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

Fix webpack test to work without bugs #7

Open
agilgur5 opened this issue Jun 13, 2019 · 4 comments · May be fixed by #15 or #16
Open

Fix webpack test to work without bugs #7

agilgur5 opened this issue Jun 13, 2019 · 4 comments · May be fixed by #15 or #16

Comments

@agilgur5
Copy link
Owner

Now that we've got a test harness and runner in #5 , should be trivial to add a webpack test that would be virtually identical to the browserify side stuff.

@agilgur5
Copy link
Owner Author

agilgur5 commented Jun 13, 2019

Ok, pretty quick work so I just decided to do it anyway. The test passes, but I do get a TypeError when trying to load the Worker with jsdom-worker:

TypeError: Only absolute URLs are supported
    at getNodeRequestOptions (/physijs-webpack/node_modules/node-fetch/lib/index.js:1299:9)
    at /physijs-webpack/node_modules/node-fetch/lib/index.js:1404:19
    at Promise (<anonymous>)
    at fetch (/physijs-webpack/node_modules/node-fetch/lib/index.js:1401:9)
    at call (/physijs-webpack/node_modules/@react-frontend-developer/jsdom-worker/src/index.js:33:19)
    at new fetch (/physijs-webpack/node_modules/@react-frontend-developer/jsdom-worker/src/index.js:93:9)
    at new module.exports (webpack:///./physijs_worker.js?./node_modules/worker-loader/dist/cjs.js:2:10)
    at new Physijs.Scene (webpack:///./physi.js?:391:18)
    at t (/physijs-webpack/webpack.spec.js:6:17)
    ...

It would seem that jsdom-worker might be incompatible with Webpack's worker-loader due to how it code-splits out the worker and references URLs. Maybe possible to fix with some publicPath hacks?

Webpack is a bit slower than browserify also (probably bc 0CJS mode does more transforms) and had to set it to development mode to get it to be reasonable (bc wrapping ammo as always takes a toll on build times). Browserify doesn't do any transforms, minification, etc by default so dev mode should match up.

@agilgur5
Copy link
Owner Author

agilgur5 commented Jun 14, 2019

Well, I found the inline option to resolve this, but then I had to add a webpack config to add it (since I can't change the source file as that's actually used externally, not just internally). Adding a config ended up still outputting and using an separate worker file, even when fallback was set to false (it would output two separate worker files when not set to false). When I removed the worker-loader! prefix from the inline require (just to test it out), it worked perfectly and had no errors either 😐

Based on webpack/webpack#1747, the configuration should override the inline loader, but for some reason both end up running for me with the inlined loader being the one that's used...

I definitely don't want to force users to inline their Worker by adding the config option to the inline require, but not really sure how to override it properly 😕

@agilgur5
Copy link
Owner Author

Might be related to webpack-contrib/worker-loader#194 -- worker-loader has a ton of unresponded issues with a lot of upvotes each 😞

Since the test passes (the Worker probably just won't work under jsdom-worker), I think I'll still push out a PR and see if I can fix it in the future. It is potentially problematic to developers as they might be unable to configure their worker-loader due to this -- though that would be an upstream bug 😕

@agilgur5
Copy link
Owner Author

Looking at the source code for worker-loader, the option handling is done entirely by loader-utils and loader-utils goes straight from the loader context from webpack core.... 👀 That would suggest the issue is either in webpack core or in the way loader-utils is being used in worker-loader -- which seems more or less correct to me (supposed to be used on this, though the default export is empty for some reason) and it has tests. But the overrideability isn't something that's tested I don't think (probably because that's provided by webpack core and not individual loaders)

This was referenced Jun 14, 2019
@agilgur5 agilgur5 changed the title Add webpack test Fix webpack test to work without bugs Jun 14, 2019
@agilgur5 agilgur5 mentioned this issue Jun 14, 2019
agilgur5 added a commit that referenced this issue Jun 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant