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

Minimal node and npm upgrade #52363

Closed
wants to merge 9 commits into from
Closed

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Jul 6, 2023

This PR includes the minimal changes to the repository to upgrade NodeJS and NPM. There are other things we could do (adopt workspaces, update the packages "engine" config... but let's leave that for a separate PR #48950)

  • Upgrade Node.js to v18
  • Upgrade npm to v9

Why

It is becoming very urgent to update node and npm. Without mentioning the benefits of the upgrade in terms of features and performance, we're now starting to be blocked as a lot of packages have updated their minimum supported versions for node and npm. (I've been trying to use yjs in a PR and got stuck because of this).

Jobs

I've fixed all the CI jobs, except one (Compressed size) but we can ignore this one, it's failing because it's running npm install in trunk with the new npm version. It will fix itself after merge.

Testing instructions

  • Clone the PR
  • Update npm and node locally
  • Just try npm run dev or any of the commands you use daily to work in Gutenberg.

@youknowriad youknowriad force-pushed the update/minimal-npm-upgrade branch 8 times, most recently from faf7769 to 73379ae Compare July 6, 2023 13:43
@github-actions
Copy link

github-actions bot commented Jul 6, 2023

Flaky tests detected in 8c91d9c.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5512940997
📝 Reported issues:

@youknowriad youknowriad marked this pull request as ready for review July 6, 2023 15:03
@youknowriad youknowriad added the [Type] Build Tooling Issues or PRs related to build tooling label Jul 6, 2023
@youknowriad youknowriad self-assigned this Jul 6, 2023
@noahtallen
Copy link
Member

noahtallen commented Jul 6, 2023

@youknowriad Since we now have access to npm dedupe and need to regenerate the lockfile anyways, we could run that to simplify the dependency tree. I tried it on this branch, and it looks the result is 36,229 - 20,998 = ~15k deletions from the lock file

@youknowriad
Copy link
Contributor Author

@noahtallen Sure, assuming it doesn't break anything (tests or whatever), I think it's important that we ship this PR quickly to avoid the constant rebases.

@noahtallen
Copy link
Member

If you like, I can push a commit to try it today, and then revert if it causes issues :)

@youknowriad
Copy link
Contributor Author

I've pushed the dedupe, let's see how it goes

@youknowriad
Copy link
Contributor Author

@noahtallen there are some very minor failures in the unit tests and one failure in the test that checks the validity of the package-lock.json. I'm certain that it's fixable and that it's probably due to the job itself but for the sake of not blocking this I'm considering reverting the dedupe to leave it to its own PR. WDYT.

@noahtallen
Copy link
Member

I was able to update the unit test snapshots and they look pretty minor -- let me see if I can also find a quick fix for the package-lock issue

@noahtallen
Copy link
Member

The package-lock issue is weird; it's changing the lockfile versions for a few of the internal react-native packages like packages/react-native-editor from v1.98.1 to v1.99.0 during normal npm install (Which I can't replicate locally even after deleting node_modules.) As far as I can tell, 1.99.0 doesn't exist anywhere since these aren't published to npm.

I wonder if this issue will happen consistently

@youknowriad
Copy link
Contributor Author

What's certain is that It didn't happen before the dedupe. I wonder if it's related to the "peers" issue in native modules.

@noahtallen
Copy link
Member

I wonder if it's related to the "peers" issue in native modules.

Hm, I feel like it'd be something else. Since there are other peer dependency issues in the repo with other packages, I don't think the react-native packages would be unique. Another point of data, the version set before the dedupe was also 1.98.0. So it feels related to something internal to npm somehow

@youknowriad
Copy link
Contributor Author

yeah maybe an npm bug.

@jsnajdr
Copy link
Member

jsnajdr commented Jul 7, 2023

Regarding regenerating the package-lock.json file, I don't think deleting the file and running npm install will suffice

I think the info about Lerna and updating dependencies is outdated. It applies to the old version of npm (6.x) which didn't support workspaces (packages) and had many bugs when it comes to maintaining the package tree and deduplicating it.

With the latest npm version, after the upgrade, npm install should safely regenerate the lockfile. The only caveat you need to be aware of is that installing from scratch will install the latest matching versions of all packages. So, if there is a dependency declared as "foo": "^1.2.3", and until now the lockfile contained foo@1.3.5, after the reinstall you can get version foo@1.4.6. In other words, the old lockfile made sure that foo version is kept stable unless you're doing some operation (install, upgrade) that directly affects it. But when regenerating the lockfile, you're getting potentially hundreds of package updates at once. That's hard to control, and can be risky because even minor upgrades can introduce bugs and unintentional breaking changes.

@youknowriad
Copy link
Contributor Author

@noahtallen Great catch on the rnmobile update, it seems like all is good now. Thanks for the update here.

@@ -32,7 +32,7 @@ exports[`DependencyExtractionWebpackPlugin Webpack \`combine-assets\` should pro
`;

exports[`DependencyExtractionWebpackPlugin Webpack \`dynamic-import\` should produce expected output: Asset file 'main.asset.php' should match snapshot 1`] = `
"<?php return array('dependencies' => array('wp-blob'), 'version' => 'c8be4fceac30d1d00ca7');
Copy link
Member

Choose a reason for hiding this comment

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

I was curious why a Node upgrade would cause change of production JS assets. That shouldn't happen. And tracked it down to caniuse-lite upgrade:

  • what actually changed in the bundle is that globalThis.webpackChunk references changed to self.webpackChunk.
  • they changed because our supported browsers no longer all support globalThis.
  • the browser that doesn's support globalThis is and_uc -- Android UC browser.
  • we support this browser because it has more than 1% usage.
  • it's only in the latest caniuse-lite data where this browser went over 1% -- it's 1.044%. It was under 1% in the old version.

A standalone PR that updates caniuse-lite (and maybe also pins it in package.json?) would be nice, but not necessary.

Copy link
Member

Choose a reason for hiding this comment

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

I merged the caniuse-lite/browserslist/core-js upgrade separately in #52420. The newly supported browser (and_uc) caused different output for webpack runtime code (the hashes in test snapshots agree!), and also new CSS autoprefixes like -webkit-margin-before for margin-block-start.

@jsnajdr
Copy link
Member

jsnajdr commented Jul 7, 2023

you're getting potentially hundreds of package updates at once. That's hard to control, and can be risky because even minor upgrades can introduce bugs and unintentional breaking changes.

And now we can see an example of this in this very PR: #52363 (comment)

The caniuse-lite package was upgraded, it has the latest data about browser usage, and the and_uc browser just reached 1% (1.044%), so Gutenberg started supporting it, and this browser doesn't support the globalThis keyword, so the webpack output has changed, and therefore asset hashes in unit test snapshots have changed 🙂

@youknowriad
Copy link
Contributor Author

Interesting. Thanks for digging there @jsnajdr

@youknowriad
Copy link
Contributor Author

So it doesn't seem like there's any blockers left. After some discussions with @kevin940726 and @desrosj We're planning on landing the upgrade by 6.3 RC1 (so in 10 days approx) to avoid too much disruptions for developers and most of us will switch to 6.4 work at that time.

Core and Gutenberg don't necessarily need to be updated at the same time but it's a good thing to try to synchronize.

@noahtallen
Copy link
Member

But when regenerating the lockfile, you're getting potentially hundreds of package updates at once. That's hard to control, and can be risky because even minor upgrades can introduce bugs and unintentional breaking changes.

Indeed -- I tried regenerating the lockfile from scratch yesterday out of curiosity, and ran into a ton of CI issues due to the updates.

@noahtallen
Copy link
Member

noahtallen commented Jul 10, 2023

I rebased to resolve conflicts and to incorporate @jsnajdr's changes.

@jsnajdr
Copy link
Member

jsnajdr commented Jul 12, 2023

I rebased to resolve conflicts and to incorporate @jsnajdr's changes.

Hope it didn't cause too much trouble with updating the package-lock.json.

@noahtallen
Copy link
Member

Hope it didn't cause too much trouble with updating the package-lock.json.

Not a problem! Turns out there's another lockfile conflicts anyways. Not sure if there's a better way to do this, but I just dropped all the package-lock changes that existed already, and ran npm install && npm dedupe again. (Thus leaving us with a single commit we could drop again if needed.)

I often rely on yarn auto-fixing the lock file in other repos... though I do see this: npm/npm#18007

@desrosj
Copy link
Contributor

desrosj commented Jul 21, 2023

Sorry if I'm missing something obvious, but what exactly does this PR unblock that #48950 doesn't? AFAIK, npm workspaces isn't a blocker. The engine bump in #48950 also targets >=14 which shouldn't block core updates either. Copying code from #48950 without citing sources could also easily lose the discussions and context. The node/npm upgrade has been blocking contributors for months, so I'll love to see it merged, but I think we should maybe ask core folks first about their progress?

I'd just like to get some clarification on this to make sure there's nothing being missed and that the reasoning is clear.

I've updated wordpress-develop#4028 to be up to date with the current state of trunk. Unfortunately, Core will unfortunately need to remain on 16.x because of a systems level blocker. There are some "unable to resolve dependency" warnings, but everything seems to proceed without an issue. If you have any thoughts around this, please follow up on that PR (which we should merge around the same time).

I don't love that the two different code bases will be using different versions. The contributor experience of having to remember to switch versions each time you change from the Core to Gutenberg repos seems like it could cause a lot of strange scenarios.

I guess, as long as we continue to run tests on 16 in the Gutenberg repository, we can manage for now though. Testing in 16.x would ensure merging code into WordPress Core will go smoothly. I'm sure we'll encounter some strangeness between 16 and 18 at some point.

It would also be great to not enforce 18.x here and still allow contributors to use 16.x if desired. I'm not sure if there are any unforeseen consequences of that, though. This would allow anyone bouncing between Core and Gutenberg to remain on 16 without having to change.

Finally, this change will need to be communicated to the development community at large. I'll draft a post for this early next week.

@noahtallen
Copy link
Member

The contributor experience of having to remember to switch versions each time you change from the Core to Gutenberg repos seems like it could cause a lot of strange scenarios.

I did want to mention using nvm to manage Node versions (https://github.com/nvm-sh/nvm). It's pretty easy to install and switches versions mostly automatically as you move between directories. I wouldn't be surprised if many JS devs already have this set up for any other projects too. Potentially worth mentioning in the post.

@desrosj
Copy link
Contributor

desrosj commented Jul 24, 2023

I did want to mention using nvm to manage Node versions (https://github.com/nvm-sh/nvm). It's pretty easy to install and switches versions mostly automatically as you move between directories. I wouldn't be surprised if many JS devs already have this set up for any other projects too. Potentially worth mentioning in the post.

Totally agree. I think detailing this in the post is an easy win. I also have my machine set up to automatically detect an .nvm file when changing directory so I almost never need to worry about it. But I guess I'm more concerned with an additional barrier being added to getting started if you're a new contributor. Just something we should consider and weigh into any decisions.

@youknowriad youknowriad closed this Aug 9, 2023
@youknowriad youknowriad deleted the update/minimal-npm-upgrade branch August 9, 2023 09:07
@simison
Copy link
Member

simison commented Aug 9, 2023

@youknowriad just reminding about earlier point and especially lack of security upgrades beyond 2023-09-11 is kind of a bummer:

Just heads up that Node.js v16 maintenance support ends in about 3 months (ref), i.e. end of security updates. So I'd assume that's when you need some kind of v18 upgrade plan regardless.

Edit: just saw https://make.wordpress.org/core/2023/08/09/updating-wordpress-to-use-more-modern-versions-of-node-js-npm/ posted, with explanation. :-) All good, thanks for your work folks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants