Skip to content
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

Conversation

piotr-oles
Copy link

@piotr-oles piotr-oles commented Nov 8, 2021

Motivation
Hi! 👋 I would like to use the new worker import syntax:

const worker = new Worker(new URL('./worker', import.meta.url));

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 and crossOriginLoading entry option (as advanced - not recommended to change).

@webpack-bot
Copy link
Contributor

For maintainers only:

  • This needs to be documented (issue in webpack/webpack.js.org will be filed when merged)
  • This needs to be backported to webpack 4 (issue will be created when merged)

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 8, 2021

CLA Signed

The committers are authorized under a signed CLA.

lib/RuntimePlugin.js Outdated Show resolved Hide resolved
@piotr-oles piotr-oles marked this pull request as draft November 8, 2021 20:16
@webpack-bot
Copy link
Contributor

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

@piotr-oles piotr-oles marked this pull request as ready for review November 9, 2021 10:15

const baseUriTemplate = `JSON.stringify(${RuntimeGlobals.scriptUrl})`;
const scriptUrlTemplate = `JSON.stringify(new URL(${workerUrlTemplate}, ${RuntimeGlobals.scriptUrl}))`;
const crossOriginLoaderTemplate = `"__webpack_base_uri__=" + ${baseUriTemplate} + ";importScripts(" + ${scriptUrlTemplate} + ");"`;
Copy link
Member

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

Copy link
Author

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 :)

Copy link
Author

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) :/

@piotr-oles piotr-oles force-pushed the feature/add-worker-cross-origin-loading-option branch from 2e2998e to 20d56b1 Compare November 12, 2021 22:26
@piotr-oles
Copy link
Author

could someone take a look? :)

@CraigglesO
Copy link

I'd love to see this pass through so I can have a more flexible/cleaner build & code setup.
@alexander-akait - are you available?

])}`
]);
}
}
Copy link
Member

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

Copy link
Author

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?

Copy link
Member

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

Copy link
Member

@alexander-akait alexander-akait Dec 7, 2021

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)

Copy link
Author

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?

Copy link
Author

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).

Copy link
Member

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?

Choose a reason for hiding this comment

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

@sokra bump.

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.

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?

@piotr-oles
Copy link
Author

I don't think it's going to be merged so I'm closing the PR :)

@piotr-oles piotr-oles closed this Jul 15, 2022
@gigadie
Copy link

gigadie commented Dec 4, 2023

and this issue still is present nowadays unfortunately

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants