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

Dev - Upgrade to Node v20 #9121

Merged
merged 54 commits into from
Jul 23, 2024
Merged

Conversation

danielmx-dev
Copy link
Contributor

@danielmx-dev danielmx-dev commented Jul 17, 2024

Fixes #9116

Changes proposed in this Pull Request

  • Upgrade Node version to v20
  • Upgrade packages that require a specific version of node. Most notably: @woocommerce/components and @wordpress/scripts. Other packages related to testing and linting were also updated.
  • Update test snapshots
  • Replace deprecated functions in tests
  • Perform various fixes in tests so they work properly with the new versions of the packages that were upgraded.
  • E2E Tests are being fixed in this other PR Dev - Fix E2E tests in Node v20 #9132

Testing instructions

  • Make sure npm scripts work as expected locally: npm build, npm start, npm lint, npm test:js
  • Make sure all checks are passing in the PR: css, lint, js, php
  • You can also test the build in a Jurassic Ninja site; however, since no client code was changed, we don't expect any changes in build size and functionality.

  • Run npm run changelog to add a changelog file, choose patch to leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.
  • Covered with tests (or have a good reason not to test in description ☝️)
  • Tested on mobile (or does not apply)

Post merge

@danielmx-dev danielmx-dev self-assigned this Jul 17, 2024
@botwoo
Copy link
Collaborator

botwoo commented Jul 17, 2024

Test the build

Option 1. Jetpack Beta

  • Install and activate Jetpack Beta.
  • Use this build by searching for PR number 9121 or branch name dev/upgrade-support-to-node-20 in your-test.site/wp-admin/admin.php?page=jetpack-beta&plugin=woocommerce-payments

Option 2. Jurassic Ninja - available for logged-in A12s

🚀 Launch a JN site with this branch 🚀

ℹ️ Install this Tampermonkey script to get more options.


Build info:

  • Latest commit: 4579ce1
  • Build time: 2024-07-23 16:13:31 UTC

Note: the build is updated when a new commit is pushed to this PR.

@danielmx-dev
Copy link
Contributor Author

danielmx-dev commented Jul 22, 2024

The Compressed Size check is currently failing due to node version upgrade (since the action doesn't change node versions between builds). We have a couple options:

  1. Update .npmrc options to engine-strict=false, merge to develop, then update this PR so it can perform the comparison.
  2. Move forward with the failing check. Since in the other PR I have confirmed that the action works as expected when both the base and target branches are using the same node version. See: Dev - Fix E2E tests in Node v20 #9132 (comment)

@danielmx-dev danielmx-dev marked this pull request as ready for review July 22, 2024 18:28
@danielmx-dev danielmx-dev requested review from a team and frosso and removed request for a team July 22, 2024 18:31
@danielmx-dev danielmx-dev changed the title Dev/upgrade support to node 20 Dev - Upgrade to Node v20 Jul 22, 2024
Copy link
Contributor

@frosso frosso left a comment

Choose a reason for hiding this comment

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

Awesome job!! 💯 Those snapshot updates are 🤢 I think we should move away from using snapshots.

Looks like an additional update is needed on this action - we should be able to just use the latest version: https://github.com/Automattic/woocommerce-payments/blob/bd5fa2793ec195315d3a2f21bc0157c413d6a8ba/.github/workflows/bundle-size.yml

Tested locally in settings/disputes/checkout/etc. I didn't notice any differences.

Thank you for working on this!

Comment on lines 196 to 200
} );

expect( handleHideMock ).toHaveBeenCalled();

act( () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious about this change - why did the expect get moved from the end to the middle here? Just to be closer to the click action?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, it wasn't necessary, the only necessary change was to wrap the runAllTimers call in a separate act() block. I'll revert that small change.

* External dependencies
*/
import { Status } from '@wordpress/notices';
// This type was imported from @wordpress/notices but it's not longer exported by the module.
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for adding the inline comment 👍

@danielmx-dev danielmx-dev added this pull request to the merge queue Jul 23, 2024
Merged via the queue into develop with commit ac09df8 Jul 23, 2024
24 of 25 checks passed
@danielmx-dev danielmx-dev deleted the dev/upgrade-support-to-node-20 branch July 23, 2024 16:24
lovo-h pushed a commit that referenced this pull request Aug 1, 2024
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.

Upgrade node support to version 20
3 participants