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 5 🚀 #1060

Merged
merged 10 commits into from
Feb 1, 2022
Merged

Webpack 5 🚀 #1060

merged 10 commits into from
Feb 1, 2022

Conversation

marcelgerber
Copy link
Member

Live on: tufte

Updates our infrastructure to finally be using Webpack 5, the new webpack cli, the new dev server, updated loaders and plugins, and all the good stuff. Everything works.

@@ -232,6 +232,9 @@
"tsc-watch": "^4.2.9",
"xhr-mock": "^2.5.1"
},
"resolutions": {
"**/webpack": "^5.0.0"
Copy link
Member Author

@marcelgerber marcelgerber Dec 30, 2021

Choose a reason for hiding this comment

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

I don't love this, but this one line lets us get rid of >1000 lines of dependencies in one sweep.
We can remove it once Storybook 7 is released, which will no longer depend on Webpack 4 (storybookjs/storybook#13491)

Base automatically changed from papaparse to master December 30, 2021 12:14
@danielgavrilov
Copy link
Contributor

danielgavrilov commented Jan 4, 2022

Awesome Marcel!

I checked out the branch, ran yarn and then yarn startWebpackServer and it crashed:

yarn run v1.22.11
$ webpack serve --mode development --no-live-reload --config ./itsJustJavascript/webpack.config.js

<--- Last few GCs --->

[38270:0x158008000]      402 ms: Scavenge 33.9 (52.6) -> 24.7 (56.3) MB, 1.7 / 0.0 ms  (average mu = 1.000, current mu = 1.000) allocation failure 
[38270:0x158008000]      474 ms: Scavenge 40.1 (58.1) -> 29.8 (61.1) MB, 1.5 / 0.0 ms  (average mu = 1.000, current mu = 1.000) allocation failure 


<--- JS stacktrace --->

==== JS stack trace =========================================

Security context: 0x00010f5c08d1 <JSObject>
    0: builtin exit frame: new constructor(aka Module)(this=0x0001125abe59 <Object map = 0x113169569>,0x0001125abd09 <Uint8Array map = 0x10f5a58c9>,0x0001125abe59 <Object map = 0x113169569>)

    1: ConstructFrame [pc: 0x1050264b8]
    2: StubFrame [pc: 0x10510ef64]
    3: /* anonymous */ [0x1125a5b61] [/Users/danielg/Projects/owid-grapher/node_modules/webpack/lib/util/hash/md4.js:11] [bytecode=...

FATAL ERROR: wasm code commit Allocation failed - process out of memory

I also tried yarn install --force and still got the same. Trying yarn buildWebpack failed too. I was using Node v12.22.3.

Then I switched to Node v14.17.3, and it worked!

Does it work for you with v12? (I can see staging is still on v12, so I'm guessing it does, and maybe it has something to do with macs only?)

@marcelgerber
Copy link
Member Author

Oh no! That's interesting, but in a bad way 😞

I currently have Node 12.22.7 installed, and with that it works just fine. But that's also on the weird Windows/Linux setup mixture that I'm running on.
Staging, however, is also running on Node 12. So maybe it's to do with Macs after all?
Could you maybe try installing Node 12.22.8 and see if it works fine there?

Copy link
Contributor

@danyx23 danyx23 left a comment

Choose a reason for hiding this comment

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

Works very nicely for me! Seems to also be a bit faster. Thanks a lot for pushing this! 🙌

@danyx23
Copy link
Contributor

danyx23 commented Jan 4, 2022

(I'm on 12.22.3 on WSL 2 on windows with lots of RAM)

@danyx23
Copy link
Contributor

danyx23 commented Jan 4, 2022

@marcelgerber @danielgavrilov In the crashlogs it mentions wasm - maybe something changed regarding that and the Mac processor architecture? When Lars is back we can ask him to try it with node 12 and then if it fails with node 16

@marcelgerber
Copy link
Member Author

Great that you could test this @danyx23, thank you! And yes, it should be quite a bit faster than Webpack 4, especially with a warm cache.

After reading pmmmwh/react-refresh-webpack-plugin#259, I'm now also pretty much sure that this is an issue with all Node 12 versions on macOS (especially since they only receive security fixes at this point).
But maybe we should upgrade to Node 16 soon-ish anyway :)

@danielgavrilov
Copy link
Contributor

Could you maybe try installing Node 12.22.8 and see if it works fine there?

It doesn't unfortunately. 🙁

@larsyencken
Copy link
Contributor

Also fails for me on node 12.22.3, but works just fine on 16.13.2, anecdotally. Any obstacles to bumping node whilst we're here?

@danyx23
Copy link
Contributor

danyx23 commented Jan 24, 2022

@larsyencken upgrading the servers and verifying they end up using the right node version is the main (not huge) obstacle. Described in more detail here: https://www.notion.so/Update-to-Node-16-04bd3b117bd64f19b85a914e40aa1bad

@larsyencken
Copy link
Contributor

I migrated the issue to Github now: #1096

I don't know what exactly is causing the code move from vendors.js to
owid.js, but as long as it's down overall and everything works that's
probably fine?
@marcelgerber
Copy link
Member Author

This is now deployed to tufte again, and all is looking good! 🎉

@marcelgerber marcelgerber merged commit 65117be into master Feb 1, 2022
@marcelgerber marcelgerber deleted the webpack5 branch February 1, 2022 13:07
mlbrgl added a commit that referenced this pull request Feb 4, 2022
This reverts commit 65117be, reversing
changes made to 92e9036.

# Conflicts:
#	.storybook/webpack.config.ts
mlbrgl added a commit that referenced this pull request Feb 4, 2022
Revert "Merge pull request #1060 from owid/webpack5"
@marcelgerber marcelgerber mentioned this pull request May 16, 2022
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.

4 participants