Skip to content
This repository was archived by the owner on Sep 9, 2021. It is now read-only.

feat: add option crossOrigin#291

Open
pckhoi wants to merge 1 commit into
webpack-contrib:masterfrom
pckhoi:master
Open

feat: add option crossOrigin#291
pckhoi wants to merge 1 commit into
webpack-contrib:masterfrom
pckhoi:master

Conversation

@pckhoi

@pckhoi pckhoi commented Sep 28, 2020

Copy link
Copy Markdown

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

Solving #281 according to this comment

Breaking Changes

No breaking changes.

Additional Info

@codecov

codecov Bot commented Sep 28, 2020

Copy link
Copy Markdown

Codecov Report

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

Impacted file tree graph

@@            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

pckhoi commented Sep 30, 2020

Copy link
Copy Markdown
Author

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

@alexander-akait alexander-akait left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

mwanago commented Nov 24, 2020

Copy link
Copy Markdown

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

@pckhoi

pckhoi commented Nov 24, 2020

Copy link
Copy Markdown
Author

Could use my fork for now?

Comment thread README.md
};
```

### `crossOrigin`

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe originPath?

@alexander-akait alexander-akait Nov 24, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@alexander-akait alexander-akait Nov 24, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

mwanago commented Nov 24, 2020

Copy link
Copy Markdown

@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

Copy link
Copy Markdown
Member

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

@mwanago

mwanago commented Nov 24, 2020

Copy link
Copy Markdown

@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

Copy link
Copy Markdown
Member

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

@mwanago

mwanago commented Nov 24, 2020

Copy link
Copy Markdown

@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

Copy link
Copy Markdown
Member

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

@pckhoi

pckhoi commented Nov 25, 2020

Copy link
Copy Markdown
Author

@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

mwanago commented Nov 25, 2020

Copy link
Copy Markdown

@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

Copy link
Copy Markdown
Member

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

mwanago commented Nov 27, 2020

Copy link
Copy Markdown

ping me at this Friday and we try to solve this

@alexander-akait I am pinging you 👀

@mwanago

mwanago commented Dec 1, 2020

Copy link
Copy Markdown

ping me at this Friday and we try to solve this

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

@alexander-akait

Copy link
Copy Markdown
Member

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

@mwanago

mwanago commented Dec 18, 2020

Copy link
Copy Markdown

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

Copy link
Copy Markdown
Member

As minimum we need better name for this option

@mwanago

mwanago commented Dec 29, 2020

Copy link
Copy Markdown

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants