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

Upgrade to Node LTS (16) and Webpack 5 #5619

Merged
merged 28 commits into from
Aug 3, 2022
Merged

Conversation

netpro2k
Copy link
Contributor

@netpro2k netpro2k commented Jul 27, 2022

This PR builds on work done in #5485 and #5240 to make long overdue updates to our build tooling. This should help alleviate all the build and install issues people have been having attempting to use Hubs with non ancient versions of node, reduce the number of build-server-only surprises, and reduce build time and bundle size. This also cleans up some strange bundling hacks we had to use and sets the stage nicely to add support for using typescript, and eventually breaking up hubs UI and "engine" code. Lastly it should allow us to finally close a number of the depndbot PRs, since most of them are for libs used at build time, not runtime.

This updates almost all of our dev dependencies so it definitely needs some testing, but no runtime dependencies should have changed (at least not in a major way, I did have to wipe out the package-lock file so we may have lost some exact pinnings)

Note this does not yet remove/update @babel/polyfill which is deprecated and has had a rather scary warning about core-js

Because of the V8 engine whims, feature detection in old core-js versions could cause a slowdown up to 100x even if nothing is polyfilled.

That we have been ignoring for quite some time... We should do that in it's own PR as it feels a bit more dangerous and hard to detect issues with (since they will likely only surface on random older browsers). Alongside that we might want to just hand-craft support.js to be usable in old engines (like IE11) without polyfills, so that we can ditch the confusing multi-target babel setup.

Also, I have updated eslint and prettier but several new rules mean some work needs to be done to make them happy. The work should be trivial but will touch a lot of files, so I also want to leave that for another PR. I have made the new eslint rules warnings and temporarily disabled the prettier eslint rule.

Copy link
Contributor

@brianpeiris brianpeiris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few things I've spotted while skimming. Nothing major stands out though.

webpack.config.js Outdated Show resolved Hide resolved
webpack.config.js Outdated Show resolved Hide resolved
webpack.config.js Show resolved Hide resolved
@keianhzo
Copy link
Contributor

keianhzo commented Jul 29, 2022

Hardware: MacBook Pro (16-inch, 2019)
OS: MacOs Monterrey 12.4
Node: 16.13.1
NPM: 6.14.15

I'm getting a warning when runining:

WARNING in ./node_modules/three-ammo/index.js 5:26-36
export 'default' (imported as 'ammoWorker') was not found in './src/ammo.worker' (module has no exports)
 @ ./src/components/body-helper.js 2:0-39 4:25-51 5:13-27
 @ ./src/hub.js 205:0-34

and some errors in the console when running against dev:
Screen Shot 2022-07-29 at 16 54 03
Screen Shot 2022-07-29 at 16 59 31
Screen Shot 2022-07-29 at 16 54 52

@netpro2k netpro2k marked this pull request as ready for review August 1, 2022 21:59
@netpro2k
Copy link
Contributor Author

netpro2k commented Aug 1, 2022

Re the ammo errors @keianhzo was seeing. Those were both actually harmless but incidentally have gone away re-introducing worker-loader. If we remove that again we will likely encounter them again, so noting for future:

  • The "world not initialized" error is the ammo worker failing to correctly handle postmessages it is not expecting. In this case, messages from webpack de server.
  • The build error about ammoWorker. three-ammo doesn't really correctly export its worker, it expects the thing using it to bundle it, but then exports it. Webpack doesn't know how to handle this. We circumvented it for our code, but the internal include (which nobody uses) still gets confused.

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

Successfully merging this pull request may close these issues.

None yet

4 participants