-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Update NodeJS to v18 #72245
Update NodeJS to v18 #72245
Conversation
I've been using nodejs 18.x.x for the longest time to run everything, so I'm pretty sure it works 😆 |
Love hearing that! |
I get this error locally, which then lead me to this answer: https://stackoverflow.com/a/73027407. The gist is that we'll probably need to update some dependencies before we can merge this. We get a similar issue in the ICFY build. ERROR in ../node_modules/moment-timezone/index.js 2:15-51
Module not found: Error: error:0308010C:digital envelope routines::unsupported
@ ../node_modules/moment-timezone/moment-timezone-utils.js 12:33-46
@ ../node_modules/@wordpress/date/build-module/index.js 6:0-47
@ ../packages/components/src/gm-closure-notice/gm-closure-notice.tsx 2:0-41 36:17-23 37:18-24 42:18-24 55:12-18 56:13-19
@ ../packages/components/src/index.ts 26:0-72 26:0-72
@ ./landing/stepper/declarative-flow/internals/index.tsx 1:0-53 137:120-131
@ ./landing/stepper/index.tsx 32:0-60 92:14-26 See also webpack/webpack#14532 Edit: this was fixed by bumping the moment-timezone packages. |
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~16 bytes added 📈 [gzipped])
Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~7 bytes added 📈 [gzipped])
React components that are loaded lazily, when a certain part of UI is displayed for the first time. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
This appears to work for me great locally! Nice work @noahtallen! I've run |
I see |
That means that 3670 bytes were added not to the actual @sgomes implemented this smart calculation method in Automattic/icfy#27. A detailed view of the chunk deltas is visible on the ICFY page for the branch: http://iscalypsofastyet.com/p/branch?branch=update-to-node-18 The GitHub comments report only the "chunk groups" deltas (chunk group, in webpack lingo, is the set of "physical" chunks needed to load one "user-visible" chunk), and only in the "chunk delta" section you can see the actual offending shared chunk: it's the JSON file for the It's strange that the |
@anomiex set up a Jetpack PR to update our side to v18, which looks good so far. I think Jetpack will be in position to merge ours when the Calypso one is ready to help developers not have to Node switch. Automattic/jetpack#28450 |
Ahh that makes sense. Thanks for clarifying! That's very useful to know about.
We're updating it from 0.5.31 to 0.5.40, because the older version doesn't support Node 18. (Seemingly related to compatibility in the crypto library. See this error message.) Looking at the changelog, I think the only thing which would impact us is the IANA TZDB updates. There are several of these. The old version was on TZDB version 2020a, and now it's on 2022g. From what I can tell, the other changes are mostly internal (like fixes in their grunt build or types). However, I did find this issue: moment/moment-timezone#999, with the comment here being kinda helpful. They're exploring compression to bring down the size. And this points back to this other issue about the package being too small, which I see @jsnajdr commented on at the time. 😆 From what I can tell, it sounds like the older version was missing data which now exists in the current version. Buuuut, this doesn't explain why the moment timezone webpack plugin is including more data now. My theory is that we're now including data from two more years of timezones, due to the database updates. But I don't understand why that's 300+KB of data.
Added! See this PR: #72308 |
a390e85
to
6489068
Compare
6489068
to
593aecb
Compare
@jsnajdr Let me know what you think about the moment timezone change. I'm not sure how to proceed with that! |
I just wanted to add a small note that even though it compresses well and thus isn't much of a problem on the network side, this can still be a large problem on the CPU side. CPU time scales with uncompressed code size, so this could potentially be introducing significant extra init time on slower systems with underpowered CPUs. If you find that to be the case, let me know and I'll see what I can do to help! 👍 |
My conclusion is that the increase in size is expected and we can proceed with this Node 18 upgrade PR as is. It upgrades 0.5.31 (trunk):
0.5.40 (this branch)
The In the newer But there's one path to make our
When importing the
For WordPress, the |
Ideally, there would be a version of |
Supposedly, this is what it does: https://momentjs.com/timezone/docs/#/using-timezones/guessing-user-timezone/:
It seems to use this for the
Interestingly, we do set the wp-calypso/client/webpack.config.js Lines 333 to 336 in 665f60e
This is supposed to "Only include data from this year onwards." Does this mean the webpack plugin isn't cutting out enough data? If so, how would we use one of those builds? Just change our import statements everywhere? I'm thinking at this point, given we won't deploy this today and tomorrow is Friday, we'll probably land this next week :) |
Starting with year 2000 seems fine to me. WordPress history starts in 2003, so it should be format all WordPress dates. But starting in 2012 or 2013 is too late. |
Right... but this is the current behavior of the branch including the big size increase 🤔 |
Hiya, drive-by comment from the person somewhat responsible for your First up, the
I think the problem here is that most of the added data in the latest
Given how much of the internals would need to change to rely on |
Thank you @gilmoreorless, this is a great suggestion. I pushed a commit setting |
Cool, this is super helpful to know! I saw the error coming from moment-timezone in the build, and decided to update the related packages.
We're definitely looking forward to Temporal!
Fantastic. I think we should be ready to merge this Monday then! |
713eb50
to
7545978
Compare
Proposed Changes
NodeJS 16 is now in maintenance, and v18 is the current LTS. This attempts to update to v18. Unsure what, if anything, will break, but figured I'd at least put up a PR and see what happens.
Testing Instructions
yarn install
plusyarn start
after switching to node 18)