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

WebPack 4 breaks Web Worker #12

Closed
tvogels opened this issue Dec 12, 2018 · 31 comments
Closed

WebPack 4 breaks Web Worker #12

tvogels opened this issue Dec 12, 2018 · 31 comments
Labels
bug Something isn't working

Comments

@tvogels
Copy link
Collaborator

tvogels commented Dec 12, 2018

Hi,

I'm unable to use the NPM version of JERI as expected. See this test case (run ./run_test.sh) (requires Docker).

RUN npx create-react-app test
WORKDIR test

# try "npm install --save" as well
ARG INSALLER="yarn add"

RUN ${INSALLER} jeri
RUN ${INSALLER} worker-loader

COPY App.js src/App.js
COPY graphs.png public/
COPY matrix_avg.exr public/

EXPOSE 3000

CMD [ "npm", "run", "start" ]

When using npm install --save, the script doesn't run, and errors Cannot read property 'context' of undefined. When using yarn add, the viewer loads and can display PNGs but is stuck on 'Downloading ...' in a tab featuring an EXR image.

@cbreak-black
Copy link
Contributor

cbreak-black commented Dec 12, 2018

There seems to be a bug in your EXR Worker, running your example I get the error ReferenceError: window is not defined exr.worker.js:2:15. Your version shows /******/ var parentHotUpdateCallback = window["webpackHotUpdate"]; while the version on jeri.io doesn't try to access window at all.

@tvogels
Copy link
Collaborator Author

tvogels commented Dec 12, 2018

So maybe the cause of this issue is a newer version of webpack that does things differently?

@cbreak-black
Copy link
Contributor

Maybe... when I let the build run in the node debugger I get pointed to this:

exception in node_modules/jeri/node_modules/worker-loader/dist/index.js:73
71
72   var filename = _loaderUtils2.default.interpolateName(this, options.name || '[hash].worker.js', {
73     context: options.context || this.options.context,
74     regExp: options.regExp
75   });

> options
{ name: 'exr.worker.js' }
> this.options
undefined

@tvogels
Copy link
Collaborator Author

tvogels commented Dec 13, 2018

@cbreak-black, what should I read from the debugger output?

@cbreak-black
Copy link
Contributor

It looks as if the worker-loader tries to get a context member of this.options, which is undefined. I have no idea why it does any of this. But it doesn't look like a bug in JERI, more like something in worker-loader or an other part of the build chain. (Or maybe we don't set options... but I'd expect some form of sane diagnostic from that).

@tvogels
Copy link
Collaborator Author

tvogels commented Dec 13, 2018

Hm okay. I noticed that jeri.io uses webpack 3 and worker-loader 1, and the current verisons are 4 and 2 respectively. Maybe the rules changed ...

@cbreak-black
Copy link
Contributor

Yes, that seems to be the culprit. worker-loader 1.1.1 (instead of 1.1.0) seems to fix the bug we encounter here, but it doesn't seem to be enough to work with webpack 4. Some of your dependencies seem to force the new version onto us.

I'll look into what is required to update our code to use webpack 4 / worker-loader 2, that should circumvent the problem.

@cbreak-black
Copy link
Contributor

This might also give us some trouble later: webpack/webpack#7352

@cbreak-black
Copy link
Contributor

Check out https://github.com/disneyresearch/jeri/tree/experiments/webpack4, this seems to work enough with webpack 4 to work in the examples, but integrating in your test doesn't work yet. But at least it doesn't fail to compile anymore.

@ja-no
Copy link

ja-no commented Dec 14, 2018

FYI: I was able to build this version and install it locally into a different project (react-app) using npm link. It works but EXR images are not being displayed. They are downloaded successfully but then nothing appears in the window. Probably something with exr-parser.worker.js. I'll dig into it.

@tvogels tvogels closed this as completed Dec 14, 2018
@tvogels tvogels reopened this Dec 14, 2018
@cbreak-black
Copy link
Contributor

What happens here is that "exr.worker.js" gets downloaded. I have no idea where from. And this file has an error on line 2: /******/ var parentHotUpdateCallback = window["webpackHotUpdate"];, the error is ReferenceError: window is not defined.

This file is supposed to be generated out of exr-parser.worker.js by worker-loader. Chances are worker-loader is doing something wrong.

@ja-no
Copy link

ja-no commented Dec 14, 2018

Yes, I'm getting the window is not defined as well.
I'm getting a positive HTTP response so I believe the file is downloaded.

@cbreak-black
Copy link
Contributor

Seems to be related to webpack/webpack#6642

@ja-no
Copy link

ja-no commented Dec 14, 2018

Yep. I tried adding globalObject: 'this' to the webpack config of JERI, but that didn't remove the error.

@cbreak-black
Copy link
Contributor

The issue is not JERI related I think, but caused by the webpacking done by create-react-app. Our own webpack config isn't considered, it wasn't even bundled in the npm pack.

@ja-no
Copy link

ja-no commented Dec 14, 2018

Hmm... can we npm eject? I don't see this option in scripts in package.json. Did you guys remove it manually?

@cbreak-black
Copy link
Contributor

After ejecting, you should be able to edit config/webpack.config.dev.js. I tried that, and got it to stop giving that stupid error, but it produced new ones. Why again do we use webpack? :)

@ja-no
Copy link

ja-no commented Dec 14, 2018

How did you eject? There is no script to run npm run eject. Or did you do it differently?

@cbreak-black
Copy link
Contributor

I did npm run eject in Thij's example (which ran, so it probably had a script for that). Then I tried removing the HotModuleReplacementPlugin`` and adding globalObject: (typeof self !== 'undefined' ? self : this),``` as suggested in webpack/webpack#6642, but no success. Maybe the dev server thing is just broken.

@ja-no
Copy link

ja-no commented Dec 14, 2018

After ejecting my test app and adding the globalObject option I'm getting:

exr-wrap.js:1 Uncaught TypeError: Cannot read property '../../jeri/node_modules/file-loader/dist/cjs.js?name=exr-wrap.wasm!../../jeri/build_npm/exr-wrap/exr-wrap.wasm' of undefined

I have file-loader 2.0.0 installed. Any idea?

@cbreak-black
Copy link
Contributor

So, finally got that weird react app creator thingie to work, for "release" mode. And I hope I didn't forget anything, because this was messy. (I hate webdev... :). I don't know how people can get anything done like this.

  • npm run eject
  • export NODE_ENV=production
  • Change the webpack config file to not break wasm:
    • Add defaultRules next to rules:
      	defaultRules: [
      	{
      		type: 'javascript/auto',
      		resolve: {}
      	},
      	{
      		test: /\.json$/i,
      		type: 'json'
      	}
      ],
      
    • Add '.wasm' to the exclude list for file loader (line 426 in my version of the file):
      exclude: [/\.(js|mjs|jsx|ts|tsx)$/, /\.html$/, /\.json$/, /\.wasm$/],
      
  • npm install webpack-cli
  • ./node_modules/.bin/webpack --config config/webpack.config.prod.js
  • cp public/graphs.png public/matrix_avg.exr build/
  • ./node_modules/.bin/serve build/

@ja-no
Copy link

ja-no commented Dec 14, 2018

Nice!

For me, it was sufficient to eject and make the changes you listed in bullet 3 (default rules and wasm exclude). I didn't need to install webpack-cli and do the rest of what you described.

I'm not getting any errors anymore and the EXR image appears. Thanks for looking into this!

@ja-no
Copy link

ja-no commented Dec 14, 2018

The only annoying thing is that one needs to eject. I wonder if there is a way to change webpack settings without ejecting.

@cbreak-black
Copy link
Contributor

Perfect, but I assume you still had to use the "updated" version of jeri's experimental webpack 4 branch , or did it not matter?

I don't know much about ejecting, but it seems you can also fork react-scripts: https://facebook.github.io/create-react-app/docs/alternatives-to-ejecting (To me that sounds a lot more annoying)

@ja-no
Copy link

ja-no commented Dec 14, 2018

I used your webpack 4 branch. Thanks for the link, I'll have a look.

@tvogels
Copy link
Collaborator Author

tvogels commented Dec 14, 2018

It seems to me like worker-loader etc. are very shaky. It's hard to get it to put the emscripten-related code exactly where it needs to me. Especially because this depends on the user's webpack config. My suggestion would be to let users manually copy the three related files (exr-paser.worker.js, exr-wrap.js, exr-wrap.wasm) to the root of their web server and drop the worker-loader and file-loader stuff.

@ja-no
Copy link

ja-no commented Dec 14, 2018

Would npm start find them? E.g. if I put them in the public folder of my react project?

@tvogels
Copy link
Collaborator Author

tvogels commented Dec 14, 2018

If you put them in the public folder of your react project, they will be served in the web root, so that would work. Note that if these files change, the user needs to update them manually ...

@ja-no
Copy link

ja-no commented Dec 14, 2018

Yes, that is true.
I'm currently doing so through a prestart script to get the updates if jeri changes automatically.
"prestart": "cp ./node_modules/jeri/src/utils/exr-parser.worker.js ./public && cp ./node_modules/jeri/src/exr-wrap/exr-wrap.* ./public"
This way we don't need to eject our react apps and mess with webpack scripts. I think I would prefer that.

@cbreak-black cbreak-black changed the title NPM version can't display EXRs. WebPack 4 breaks Web Worker Dec 17, 2018
@cbreak-black
Copy link
Contributor

I updated a ton of dependencies, among them lots of security fixes in 0a9f3a8, react in 21ed42a and webpack in 212f0f5.

My stand-alone tests work, so I plan to release this as 2.0 eventually.

@cbreak-black cbreak-black added the bug Something isn't working label May 14, 2019
@cbreak-black
Copy link
Contributor

Webpack 4.31 seems to work somewhat reliably.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants