-
-
Notifications
You must be signed in to change notification settings - Fork 26.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
WIP: Add support for WebWorker with worker-loader #5886
base: main
Are you sure you want to change the base?
Conversation
@@ -170,6 +170,7 @@ module.exports = function(webpackEnv) { | |||
// We inferred the "public path" (such as / or /my-project) from homepage. | |||
// We use "/" in development. | |||
publicPath: publicPath, | |||
globalObject: '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.
This is required due to what seems to be a bug/regression in webpack 4. More details can be found here: webpack/webpack#6642
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.
Hello @iansu , can i ask a question . Why globalObject value is this , not self ?
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.
+1
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.
@ri7nz -- as far as I can tell, the eslint config inside create-react-app
bugs out on self
keyword saying it's invalid, and hence the empty production build when using worker-loader yourself.
// Process WebWorker JS with Babel. | ||
// The preset includes JSX, Flow, TypeScript, and some ESnext features. | ||
{ | ||
test: /\.worker\.(js|jsx|mjs)$/, |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This is dope, but FYI eslint marks any use of "self" inside workers as error because of the rule "no-restricted-globals". |
Yes, that's true. If we decide to go forward with this we'll want to relax that rule (hopefully only in |
Also, we'd probably want to do something similar for typescript? Some Ideas of how to do this. Idea 1When compiling typescript for a webworker, add the I'm not sure how well this would work if both the worker and main bundle require the same file. They would probably be compiled twice. Maybe the default Idea 2Make the user explicitly reference the ambient declaration file for workers at the top of This would look something like: /// <reference lib="webworker" />
self.addEventListener((message) => console.log(message)); I think idea 2 is better but it does push some work onto the end user of create react app. What do you think? |
This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs. |
Any updates for this PR? |
No updates at the moment. There's still some more work to do to get this ready. I've added a couple of to do items to the description. |
@iansu Right, thanks 👍 |
Thanks for all your hard work guys. Any idea of date/time when this will be released? We've had to revert back to an older Webpack 3 non-PWA version of our product as the worker worked flawlessly then. Thanks |
This pull request has been automatically marked as stale because it has not had any recent activity. It will be closed in 5 days if no further activity occurs. |
Is this PR still being worked on? |
I'm not actively working on it at the moment but I am still planning on getting it into Create React App one of these days. |
@iansu, I've taken a look at your PR and am not seeing any ESLint issues relating to I'm also happy to take a look at TypeScript support (though this isn't an area I know very well). |
Thx for the pr and I stealed it to my project first. To whom may use worker-loader in CRA, please be aware of this issue webpack-contrib/worker-loader#176, |
Gotcha. Yeah, I think the way you get rid of a worker is to let it get garbage collected, but I don't think that's possible with workerize. You might ask in the issues on workerize. |
https://developer.mozilla.org/en-US/docs/Web/API/Worker/terminate |
Worker loader eslint support and documentation
Thanks for the merge @iansu. What's the status of this PR now, given workerize-loader now works with CRA. Is the intent to still support workers "natively" in react-scripts at some point? Or will this be dropped in favour of using workerize-loader? |
Would be great if the actual description of what you did wouldn't be paywalled / restricted to your workshop @kentcdodds |
@kentcdodds nice work with the workerize workaround. Seems to work well in dev, however when including a worker, the production build doesn't seem to output any files. Have you seen this? Might raise an issue in CRA about this. I've got a reproducible case here if anyone wants to take a look https://github.com/mattdarveniza/worker-cra-test |
here is a workaround developit/workerize-loader#48 (comment) for unit testing react components that import workers using workerize-loader |
@mattdarveniza, I'm not sure what's wrong with yours (didn't have time to look at it), but I can tell you that mine works fine in production: https://react-performance.netlify.com/isolated/final/02.extra-1.js |
I copied the diff into my source tree and was able to use the workers with Typescript just fine. |
Still hoping this gets merged eventually. I'm working on a project that uses Since I'm using I tried to ignore it using an override in my .eslintrc like so:
This override works if I'm ejected but it doesn't work if I'm un-ejected, even if I have
Any other type of pattern in The error I see get when I add in the
Here's my patch-package patch: index 350d424..7afc84b 100644
--- a/node_modules/react-scripts/config/webpack.config.js
+++ b/node_modules/react-scripts/config/webpack.config.js
@@ -307,6 +307,7 @@ module.exports = function(webpackEnv) {
'scheduler/tracing': 'scheduler/tracing-profiling',
}),
...(modules.webpackAliases || {}),
+ 'src': paths.appSrc,
},
plugins: [
// Adds support for installing with Plug'n'Play, leading to faster installs and adding
@@ -375,6 +376,13 @@ module.exports = function(webpackEnv) {
],
include: paths.appSrc,
},
+ {
+ test: [/\.worker\.(js|ts)$/],
+ include: paths.appSrc,
+ use: [
+ { loader: 'worker-loader' }
+ ]
+ },
{
// "oneOf" will traverse all following loaders until one will
// match the requirements. When no loader matches it will fall
@@ -668,6 +676,9 @@ module.exports = function(webpackEnv) {
// https://github.com/jmblog/how-to-optimize-momentjs-with-webpack
// You can remove this if you don't use Moment.js:
new webpack.IgnorePlugin(/^\.\/locale$/, /moment$/),
+ // dtrace-provider is an optional dependency that gives us build time warnings
+ // this makes webpack ignore this file, so we don't see unnecessary warnings.
+ new webpack.IgnorePlugin(/dtrace-provider/),
// Generate a service worker script that will precache, and keep up to date,
// the HTML & assets that are part of the webpack build.
isEnvProduction &&
@@ -692,6 +703,7 @@ module.exports = function(webpackEnv) {
typescript: resolve.sync('typescript', {
basedir: paths.appNodeModules,
}),
+ memoryLimit: 99999,
async: isEnvDevelopment,
useTypescriptIncrementalApi: true,
checkSyntacticErrors: true,
diff --git a/node_modules/react-scripts/scripts/build.js b/node_modules/react-scripts/scripts/build.js
index fa30fb0..fe09f9f 100644
--- a/node_modules/react-scripts/scripts/build.js
+++ b/node_modules/react-scripts/scripts/build.js
@@ -163,6 +163,7 @@ function build(previousFileSizes) {
const compiler = webpack(config);
return new Promise((resolve, reject) => {
compiler.run((err, stats) => {
+ console.log(stats.toString())
let messages;
if (err) {
if (!err.message) { |
+1 on this PR, would love to see this become something that's supported by default |
Wrote a small post for myself if you want to use web-workers in CRA without unmounting - you may find it interesting -> https://dev.to/nicolasrannou/web-workers-in-create-react-app-cra-without-unmounting-4865 |
Is this PR still beeing implemented or is it aborted already? |
@NicolasRannou Nice post man, thanks ! I was also thinking about integrating For those who handle the harsh IE11 support, you might need some polyfills on the way
Edit : I am trying comlink-loader, which ships all my requirements together 😍 |
FWIW Webpack 5 has added support for bundling workers that are initialized with a module-relative path: new Worker(new URL ('./foo.js', import.meta.url)) |
Any updates on this PR? 😅 |
+1 would be very nice to have indeed. |
Hola, back in this thread after a successful year using workerize-loader just with the It looks like the most sensible solution here would be a separate loader for workers in the This seems to have dropped off the list of things you're (understandably) prioritising @iansu, I might have a crack at this fresh given the changes in react-scripts@4, although I suppose if there's native worker support in webpack 5 and that's in the pipeline for 4.1 or something, it might just be easier to wait? |
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.
Just leaving this review since I saw it in my pending requests! Best of luck with the PR! Seems to be a GitHub bug since I no longer have rights.
This is an updated version of #3934
Use worker-loader to turn any file that ends with .worker.js into a WebWorker.
Closes #3660
Here is the sample WebWorker code I used to test this:
To Do
self
in workers