feat: add option `crossOrigin` #291

pckhoi · 2020-09-28T10:20:01Z

This PR contains a:

Motivation / Use-Case

Solving #281 according to this comment

Breaking Changes

No breaking changes.

Additional Info

handle cross origin worker drawing inspiration from https://benohead.com/blog/2017/12/06/cross-domain-cross-browser-web-workers/

codecov · 2020-09-28T10:21:29Z

Codecov Report

Merging #291 (3fcc7a2) into master (2448e13) will decrease coverage by 6.17%.
The diff coverage is 30.43%.

@@            Coverage Diff             @@
##           master     #291      +/-   ##
==========================================
- Coverage   76.35%   70.17%   -6.18%     
==========================================
  Files           6        7       +1     
  Lines         148      171      +23     
  Branches       52       62      +10     
==========================================
+ Hits          113      120       +7     
- Misses         30       44      +14     
- Partials        5        7       +2

Impacted Files	Coverage Δ
src/runtime/crossOrigin.js	`0.00% <0.00%> (ø)`
src/index.js	`95.34% <100.00%> (+0.11%)`	⬆️
src/utils.js	`88.37% <100.00%> (+1.88%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2448e13...3fcc7a2. Read the comment docs.

pckhoi · 2020-09-30T07:52:55Z

@evilebottnawi how is my solution? If you think it is good then I'll try to improve code coverage.

alexander-akait

Thanks for the PR, I will look at this in near future, maybe we can avoid extra the crossOrigin option and just add a new inline: 'cross-origin'

mwanago · 2020-11-24T09:38:38Z

Hello. Any plans on going forward with this one? 💔

pckhoi · 2020-11-24T14:15:38Z

Could use my fork for now?

alexander-akait · 2020-11-24T14:28:14Z

README.md

@@ -133,6 +134,32 @@ module.exports = {
 };
 ```

+### `crossOrigin`


I am fine with this PR, but let's do better name for this option, crossOrigin sounds misleading even for me although I understand that we are trying to solve this

Maybe originPath?

This change runtime logic, so I think we should point it out

Maybe workerChunkUrl? Hard for me too, here we have two changes, using import-scripts and pass filename like URL, it can be misleading with inline, I think ideally we should have chunkLoadingType: 'inline-fallback' | 'inline-no-fallback' | 'import-scripts' and set publicPath to Absolute URL, in runtime we should do publicPath + filename for import-scripts

I understand the need to use publicPath but there's a need to have different public paths for different assets. Maybe I should test whether I can change publicPath on the fly before importing worker. Also why do you refer to worker as "chunk" instead of "script"? Maybe have something like scriptType: "inline-fallback" | "inline-no-fallback" | "import-scripts"?

mwanago · 2020-11-24T15:20:00Z

@pckhoi I don't seem to be able to use a relative path here.

options: {
  crossOrigin: '/static-proxy/'
}

Uncaught DOMException: Failed to execute 'importScripts' on 'WorkerGlobalScope': The URL '/static-proxy/worker.aeb45db0.worker.js' is invalid.

Assuming my page is http://reddit.com, I would like to add /static-proxy/ as my originPath.
This would result in using http://reddit.com/static-proxy/${fileName} as the path to the worker.

Can this be achieved? Unfortunately, the URL of my page is dynamic, and therefore the originPath has to be relative.

alexander-akait · 2020-11-24T15:22:50Z

@mwanago Yep, we need thinking more about this and solve this in more general way

mwanago · 2020-11-24T15:29:25Z

@evilebottnawi Maybe allowing a function to be used as the crossOrigin (or another name we come up with) would help.
I could use ${window.location.origin}/static-proxy as the return of the function.

Instead of

options: {
  crossOrigin: '/static-proxy/'
}

do

options: {
  crossOrigin: () => (
    `${window.location.origin}/static-proxy/`
  )
}

alexander-akait · 2020-11-24T15:55:48Z

We should use currentScript here, webpack supports publicPath: 'auto'...

mwanago · 2020-11-24T16:05:21Z

@evilebottnawi My public path is set to something else and I need it to stay this way.
I need to override it only for the worker path.

alexander-akait · 2020-11-24T16:55:55Z

@mwanago Just for information, can you provide real use case?

pckhoi · 2020-11-25T01:44:14Z

@mwanago this PR is meant to solve cross origin use cases. I think you might be able to do what you want with existing options already. What have you tried so far? Would changing __webpack_public_path__ before importing worker script not work?

Also just noticed we also have a publicPath option. This is confusing. Does it only override Webpack's publicPath?

mwanago · 2020-11-25T10:43:04Z

@pckhoi This is precisely due to me wanting to solve my cross-origin issue.

@evilebottnawi
I've created a small proxy on my Node.js backend that is in the same origin and under the hood loads content from the CDN. Now I want to use this proxy to load my worker.

@pckhoi
Changing the __webpack_public_path__ before importing the worker script didn't work well for me because I wasn't able to change it back to the previous value.
Trying to change it back caused it to work like I never changed __webpack_public_path__ before importing.

alexander-akait · 2020-11-25T11:52:17Z

sounds resonable, I will thinking about this deeply in near future, right now I am focused on webpack-dev-server updating, so ping me at this Friday and we try to solve this

mwanago · 2020-11-27T09:50:54Z

ping me at this Friday and we try to solve this

@alexander-akait I am pinging you 👀

mwanago · 2020-12-01T17:04:50Z

ping me at this Friday and we try to solve this

@alexander-akait Is there any hope for tackling this issue?

alexander-akait · 2020-12-02T11:26:55Z

Sorry for delay, deadline + a lot of issues, I remember about this, try to return to this in near future, sorry again

mwanago · 2020-12-18T13:34:00Z

Sorry for delay, deadline + a lot of issues, I remember about this, try to return to this in near future, sorry again

Is there any way I can help you with that?

alexander-akait · 2020-12-18T13:35:55Z

As minimum we need better name for this option

mwanago · 2020-12-29T14:50:07Z

@alexander-akait
I think workerChunkUrl sounds fine, as long as we allow it to be a relative path.

feat: add option crossOrigin

3fcc7a2

handle cross origin worker drawing inspiration from https://benohead.com/blog/2017/12/06/cross-domain-cross-browser-web-workers/

alexander-akait reviewed Oct 2, 2020

View reviewed changes

alexander-akait reviewed Nov 24, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add option `crossOrigin` #291

feat: add option `crossOrigin` #291

pckhoi commented Sep 28, 2020

codecov bot commented Sep 28, 2020 •

edited

Loading

pckhoi commented Sep 30, 2020

alexander-akait left a comment

mwanago commented Nov 24, 2020

pckhoi commented Nov 24, 2020

alexander-akait Nov 24, 2020

mwanago Nov 24, 2020

alexander-akait Nov 24, 2020 •

edited

Loading

alexander-akait Nov 24, 2020 •

edited

Loading

pckhoi Nov 25, 2020

mwanago commented Nov 24, 2020 •

edited

Loading

alexander-akait commented Nov 24, 2020

mwanago commented Nov 24, 2020 •

edited

Loading

alexander-akait commented Nov 24, 2020

mwanago commented Nov 24, 2020

alexander-akait commented Nov 24, 2020

pckhoi commented Nov 25, 2020 •

edited

Loading

mwanago commented Nov 25, 2020

alexander-akait commented Nov 25, 2020

mwanago commented Nov 27, 2020

mwanago commented Dec 1, 2020 •

edited

Loading

alexander-akait commented Dec 2, 2020

mwanago commented Dec 18, 2020

alexander-akait commented Dec 18, 2020

mwanago commented Dec 29, 2020 •

edited

Loading

feat: add option crossOrigin #291

Are you sure you want to change the base?

feat: add option crossOrigin #291

Conversation

pckhoi commented Sep 28, 2020

Motivation / Use-Case

Breaking Changes

Additional Info

codecov bot commented Sep 28, 2020 • edited Loading

Codecov Report

pckhoi commented Sep 30, 2020

alexander-akait left a comment

Choose a reason for hiding this comment

mwanago commented Nov 24, 2020

pckhoi commented Nov 24, 2020

alexander-akait Nov 24, 2020

Choose a reason for hiding this comment

mwanago Nov 24, 2020

Choose a reason for hiding this comment

alexander-akait Nov 24, 2020 • edited Loading

Choose a reason for hiding this comment

alexander-akait Nov 24, 2020 • edited Loading

Choose a reason for hiding this comment

pckhoi Nov 25, 2020

Choose a reason for hiding this comment

mwanago commented Nov 24, 2020 • edited Loading

alexander-akait commented Nov 24, 2020

mwanago commented Nov 24, 2020 • edited Loading

alexander-akait commented Nov 24, 2020

mwanago commented Nov 24, 2020

alexander-akait commented Nov 24, 2020

pckhoi commented Nov 25, 2020 • edited Loading

mwanago commented Nov 25, 2020

alexander-akait commented Nov 25, 2020

mwanago commented Nov 27, 2020

mwanago commented Dec 1, 2020 • edited Loading

alexander-akait commented Dec 2, 2020

mwanago commented Dec 18, 2020

alexander-akait commented Dec 18, 2020

mwanago commented Dec 29, 2020 • edited Loading

feat: add option `crossOrigin` #291

feat: add option `crossOrigin` #291

codecov bot commented Sep 28, 2020 •

edited

Loading

alexander-akait Nov 24, 2020 •

edited

Loading

alexander-akait Nov 24, 2020 •

edited

Loading

mwanago commented Nov 24, 2020 •

edited

Loading

mwanago commented Nov 24, 2020 •

edited

Loading

pckhoi commented Nov 25, 2020 •

edited

Loading

mwanago commented Dec 1, 2020 •

edited

Loading

mwanago commented Dec 29, 2020 •

edited

Loading