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 #53426

Merged
merged 3 commits into from
Aug 9, 2023
Merged

minimal node and npm upgrade #53426

merged 3 commits into from
Aug 9, 2023

Conversation

youknowriad
Copy link
Contributor

Ok this is the third PR to attempt to upgrade node and npm. Sorry :)

Previous attempts #52363 and #48950

The difference here is that this only upgrades to:

  • node 16
  • npm 8

This matches the Wordpress-develop PR WordPress/wordpress-develop#4028
And this is due to some current limitations in the build servers of wp.org. See this post for more details https://make.wordpress.org/systems/2023/02/09/upgrade-nodejs-npm-on-the-build-server/

The systems team is aware that the EOL of node 16 is approaching but to avoid blocking things too much, we can still upgrade to 16 which will unblock the yjs dependency needed for phase 3 work for now and 18 once the build servers are ready.

@youknowriad youknowriad self-assigned this Aug 8, 2023
@youknowriad youknowriad added the [Type] Build Tooling Issues or PRs related to build tooling label Aug 8, 2023
@youknowriad
Copy link
Contributor Author

Looks like there's some typescript build issues in this PR, not sure I understand these failures.

@noahtallen
Copy link
Member

noahtallen commented Aug 8, 2023

I rebased on trunk after merging the typescript update (#52621), and put all of the package-lock.json changes into one commit. (Basically just discarded the current package-lock changes, and ran npm install followed by npm dedupe.)

I then ran both npm run build:package-types and npm run lint locally, and both passed.

One odd thing -- this PR is around +90k lines/-40k lines, but the old PR (#52363) was +55k/-55k lines. So it doesn't seem to be de-duplicating as much this time! Potentially a difference between npm 8 and 9, or some other changes since then?

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

The change looks good and tested well for me 👍

Copy link
Member

@ellatrix ellatrix left a comment

Choose a reason for hiding this comment

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

YES!

Copy link
Member

@ntwb ntwb left a comment

Choose a reason for hiding this comment

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

Looks good, though I see this n the ailing CI run:

/usr/bin/git reset --hard
HEAD is now at 1ac6aef Merge 3d16f40be7a232c0457569c5e20b8ec087dd42b6 into c94eca530c73cb06d4a73f78559ee27c0bea9ecb
[base] Checkout target branch
  /usr/bin/git fetch -n origin trunk
  From https://github.com/WordPress/gutenberg
   * branch                  trunk      -> FETCH_HEAD
   * [new branch]            trunk      -> origin/trunk
  successfully fetched base.ref
  checking out and building base commit
  /usr/bin/git reset --hard trunk
  fatal: ambiguous argument 'trunk': unknown revision or path not in the working tree.
  Use '--' to separate paths from revisions, like this:
  'git <command> [<revision>...] -- [<file>...]'
  /usr/bin/git reset --hard c94eca530c73cb06d4a73f78559ee27c0bea9ecb
  HEAD is now at c94eca530c Fix Storybook build which broke after Typescript 5 update (#53445)
[base] Cleanup via npm run distclean
  /opt/hostedtoolcache/node/16.20.1/x64/bin/npm run distclean
  
  > gutenberg@16.4.0-rc.1 distclean
  > rimraf node_modules packages/*/node_modules

@youknowriad
Copy link
Contributor Author

@ntwb The compressed size check failure is ok because that job compares with trunk and in trunk, node version need to be 14. So it's going to fix itself after merge.

@ntwb
Copy link
Member

ntwb commented Aug 9, 2023 via email

Copy link
Member

@ntwb ntwb left a comment

Choose a reason for hiding this comment

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

:shipit:

@youknowriad youknowriad merged commit a3bef32 into trunk Aug 9, 2023
50 of 51 checks passed
@youknowriad youknowriad deleted the update/node-16-npm-8 branch August 9, 2023 08:49
@github-actions github-actions bot added this to the Gutenberg 16.5 milestone Aug 9, 2023
@swissspidy
Copy link
Member

Finally 🎉 Thank you!

@desrosj
Copy link
Contributor

desrosj commented Aug 9, 2023

The changes required for Core have also been committed. Thanks again, everyone!

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.

7 participants