-
-
Notifications
You must be signed in to change notification settings - Fork 8.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add cross-origin worker support #14680
Add cross-origin worker support #14680
Conversation
For maintainers only:
|
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
lib/dependencies/WorkerDependency.js
Outdated
|
||
const baseUriTemplate = `JSON.stringify(${RuntimeGlobals.scriptUrl})`; | ||
const scriptUrlTemplate = `JSON.stringify(new URL(${workerUrlTemplate}, ${RuntimeGlobals.scriptUrl}))`; | ||
const crossOriginLoaderTemplate = `"__webpack_base_uri__=" + ${baseUriTemplate} + ";importScripts(" + ${scriptUrlTemplate} + ");"`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bad idea to use importScripts
, also it should respect ESM output
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I will create test cases for esm output and update the code :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added support for ESM modules and tested it manually. Unfortunately, I wasn't able to make automatic tests to work with ESM (and I didn't found any ESM tests with workers) :/
2e2998e
to
20d56b1
Compare
could someone take a look? :) |
I'd love to see this pass through so I can have a more flexible/cleaner build & code setup. |
])}` | ||
]); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure it is good solution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you elaborate?
Is it bad because of the runtime implementation?
Are you concerned about browser support?
Or is it bad because of how it interact with webpack architecture?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Firstly, we need to add blob
support to https://github.com/webpack/webpack/blob/main/lib/config/target.js#L81
, we don't need to generate extra runtime if developer provide versions.
I think we need to look at this solution a little differently, blob
loading can be added not only for workers, any file can be loaded using blob
, yes it is most often applicable to workers, but if we need to add blob support we need to add them to any types - js/web wokrers/asset modules and provide option to this.
crossOriginLoading
is some misleading, you apply several techniques that can be applied to other things to solve the cross origin problem. For example you can apply base64
here too.
I see what you want to solve and what problem you are experiencing, but I think we should think it over in more detail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already support asset/raw
, so we can have asset/blob
and refactor runtime to allow using it on javascript
files, so you can loaded bundled file as raw text or as blob (just idea)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... That's interesting, I agree that we should try to generalise the use-case.
On the other hand, we don't load the file using Blob - we only use it to create importScripts(filePath)
code (and the file itself is loaded later by the runtime).
So this wouldn't work with other file types. Do you suggest adding asset/blob
and then emitting the "importScripts(filePath)" file and importing it using asset/blob
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In some sense it's already possible - I do this here (it could be extracted to a plugin).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sokra What do you think about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sokra bump.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My team is desperate for this to be merged. We need cross-origin workers, and worker-loader
is causing all kinds of problems. Is there any way that generalizing custom public paths can be deferred until after this feature is merged? If I can assist in development or testing in any way, I am happy to offer my attention and energy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@AprilArcus
You can use a shim in the meantime that @piotr-oles created here
Works great for me. Although, I'm in the same boat because I want to have 2 separate builds for varying purposes.
Would be happy to add support where I can. @alexander-akait - is there anyone else that could take a look?
I don't think it's going to be merged so I'm closing the PR :) |
and this issue still is present nowadays unfortunately |
Motivation
Hi! 👋 I would like to use the new worker import syntax:
to share code between the main thread and the worker.
Unfortunately, the project I'm working on uses CDN on a separate domain, and I get a CORS error. I believe this is a common pattern, and I think it would be very nice if webpack would support cross-origin workers out of the box.
Related discussion: #14648
What kind of change does this PR introduce?
Feature
Did you add tests for your changes?
I added some tests, but I'm not sure if it's enough. It's a little bit hard to test because the issue is browser-specific and I don't know if it's possible to reproduce on nodejs.
Does this PR introduce a breaking change?
No
What needs to be documented once your changes are merged?
We have to add documentation for the new
workerCrossOriginLoading
output option andcrossOriginLoading
entry option (as advanced - not recommended to change).