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 browserify test to work without errors in Node v10 #14

Open
agilgur5 opened this issue Nov 25, 2019 · 2 comments · May be fixed by #15 or #16
Open

Fix browserify test to work without errors in Node v10 #14

agilgur5 opened this issue Nov 25, 2019 · 2 comments · May be fixed by #15 or #16

Comments

@agilgur5
Copy link
Owner

agilgur5 commented Nov 25, 2019

So in #13 I tried to upgrade CI to Node v10 Active LTS (v8 is EOL in January, lots of new features and performance in v10, npm ci, etc, etc) and that apparently caused the browserify test to start failing. I noticed this locally too, but didn't at all realize it was related to me updating my local Node version 😕

Per the Travis build, the error I'm getting is:

  /home/travis/build/agilgur5/physijs-webpack/build/browserify.bundle.js:100
    99:     if (options && options.bare) { return blob; }
   100:     var workerUrl = URL.createObjectURL(blob);   
   101:     var worker = new Worker(workerUrl);          
  Error thrown in test:
  TypeError {
    message: 'URL.createObjectURL is not a function',
  }

This is coming directly from the webworkify code and is seems like it's because jsdom has not implemented URL.createObjectURL, so it would make this a cantfix.

I have no clue why it works on Node v8 and fails on Node v10 though, that doesn't really make sense to me as that doesn't change the jsdom version and it's not implemented in any jsdom version. I'm guessing it might be silently failing and this might be why the browserify tests have the coverage issues found in #6, but idk.

Based on all the bugs encountered with workers / jsdom, I think it would make sense for this library to move to using something like cypress or a headless browser so that all the necessary features are implemented, and hopefully then the tests will pass without errors and code coverage will actually give some results.

@agilgur5
Copy link
Owner Author

agilgur5 commented Nov 25, 2019

Ok I tried a whole ton of stuff to get all tests working and just ran into so so many issues 😢 😢 😞

In cypress the webpack test runs in about half a sec and passes. But the browserify test will run for like a full minute, wind up all the fans, and then just errors out on importing either ./vendor/ammo.js or ./physijs/physi.js (or something) with Error: Cannot find module...
(I also tried running with just the watch preprocessor, but then I couldn't import or require anything, so tried loading an HTML file with the bundle in a script tag, but still had issues and that was very convoluted)

In karmatic the webpack test similarly runs fast and passes. But the browserify test will run for a long while and then just spit out gc errors until it crashes.

With jest-puppeteer I couldn't get either test to succeed as Physijs.Scene() would error out.

I did get the browserify test to run and succeed when I ran it in cypress and imported browserify.js directly (cypress's default preprocessor is browserify), but it took like over a minute to run... adding noParse to cypress's browserify made it a bit faster but still like a minute... 😕 When I run browserify directly or in CI, it is much faster than cypress's build...

Idk why browserify has so many issues and even errors out in a browser. The long running time and gc errors make me think it may be related to the infinite loop/cycle issue found in #6 and that there's something very weird with webworkify.

@agilgur5
Copy link
Owner Author

agilgur5 commented Nov 25, 2019

Ok, once I added ignore: [/ammo/] to cypress's browserify's babelify config, it sped up exponentially to a few seconds. So I guess noParse does not affect transforms in browserify.

Given this fact, it's likely that the gc errors with karmatic were also from babel trying to parse ammo. And the infinite loop issues in #6 are probably due to istanbul-lib-instrument (which uses @babel/traverse under-the-hood) trying to parse the ammo code as well.

And yes, unfortunately babelify is needed because we get import errors without it (SyntaxError: 'import' and 'export' may appear only with 'sourceType: module' (17:0) while parsing /Users/.../physijs-webpack/cypress/support/index.js while parsing file: /Users/.../physijs-webpack/cypress/support/index.js), even if we only use requires... I assume that means some dependency is written in ESM then...
EDIT: babelify is not needed; the cypress/support/index.js file is actually not empty like most of its auto-generated files, and unlike most, it uses ESM instead of CJS. Switching it to CJS allows you to run with browserifyOptions: { ... transform: [] }, which will override the default to not use babelify.

Anyway got it to work with decent speed, so now just to reorganize stuff and see about adding coverage reporting

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