search

agilgur5 / physijs-webpack

PhysiJS port for bundlers with out-of-the-box support for Webpack and Browserify
Other
9 stars 3 forks source link

Fix webpack test to work without bugs #7

Open agilgur5 opened 5 years ago

agilgur5 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 https://github.com/webpack/webpack/issues/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 commented 5 years ago

Might be related to https://github.com/webpack-contrib/worker-loader/issues/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 commented 5 years ago

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)