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.

Sign up for GitHub

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

Jump to bottom

Update to node 20 #33818

Merged
merged 11 commits into from
Nov 14, 2023
Merged

Update to node 20 #33818

merged 11 commits into from
Nov 14, 2023

Conversation

anomiex
Copy link
Contributor

@anomiex anomiex commented Oct 26, 2023 •
edited
Loading

Proposed changes:

Calypso has updated to node 20, which means it's time for us to do so as well.

Also, it has been two years since we switched from yarn to pnpm. Let's clean up the dummy .engines.yarn and remove the doc it references.

Also, since pnpm is the only thing (as far as I know) we use that checks package.json .engines and it's happy to check the monorepo root for everything, let's remove the engines from all the projects' package.jsons so we don't have to have such a giant PR in the future. That means we can also get rid of the linting that syncs all the projects' package.jsons and the special logic in the mirroring that strips it out of the mirrors.

Other information:

  • Have you written new tests for your changes, if applicable?
  • Have you checked the E2E test CI results, and verified that your changes do not break them?
  • Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Jetpack product discussion

None specifically, but p4TIVU-aOV-p2 is relevant.
Announcement post: pdWQjU-Ab-p2

Does this pull request change what data or activity we track or use?

No

Testing instructions:

  • Does everything still work?

@anomiex
Update to node 20
05264e5 
Calypso has updated to node 20, which means it's time for us to do so as
well.

Also, it has been two years since we switched from yarn to pnpm. Let's
clean up the dummy `.engines.yarn` and remove the doc it references.

Also, since pnpm is the only thing (as far as I know) we use that checks
package.json `.engines` and it's happy to check the monorepo root for
everything, let's remove the engines from all the projects'
package.jsons so we don't have to have such a giant PR in the future.
That means we can also get rid of the linting that syncs all the
projects' package.jsons and the special logic in the mirroring that
strips it out of the mirrors.
@anomiex anomiex self-assigned this Oct 26, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Oct 26, 2023 •
edited
Loading

Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.

  • To test on WoA, go to the Plugins menu on a WordPress.com Simple site. Click on the "Upload" button and follow the upgrade flow to be able to upload, install, and activate the Jetpack Beta plugin. Once the plugin is active, go to Jetpack > Jetpack Beta, select your plugin, and enable the update/node-20 branch.

    • For jetpack-mu-wpcom changes, also add define( 'JETPACK_MU_WPCOM_LOAD_VIA_BETA_PLUGIN', true ); to your wp-config.php file.

  • To test on Simple, run the following command on your sandbox:

    bin/jetpack-downloader test jetpack update/node-20

    bin/jetpack-downloader test jetpack-mu-wpcom-plugin update/node-20

Interested in more tips and information?

  • In your local development environment, use the jetpack rsync command to sync your changes to a WoA dev blog.
  • Read more about our development workflow here: PCYsg-eg0-p2
  • Figure out when your changes will be shipped to customers here: PCYsg-eg5-p2

@anomiex
Copy link
Contributor Author

anomiex commented Oct 27, 2023 •
edited
Loading

Ugh, should have gone to bed an hour ago. Noting down what I've found so far to not lose context:

  • Boost tests are failing early with a weird error, "Expected array buffer, or typed array to be returned for the "source" from the "transformSource" function but got null"
  • This seems to have started with node 20.6.0, 20.5.1 works fine. I suspect https://href.li?https://github.com/nodejs/node/pull/47999
  • Problem seems to be when you have a load hook where you have esm -> import cjs that gets a non-null source -> require cjs that tries to return a (default) null source, it blows up with that error.
  • The simple fix of never returning a null source (see the "Here is an example hook that would opt-in to using the non-default behavior" bit here) doesn't work either, eventually it hangs for some reason. Haven't worked out why yet.
  • Never returning a non-null source for commonjs does work, at least for us. OTOH, nodejs's docs say that's going away at some point. 🤷
  • Overall, not sure whether all the bugs are in nodejs or if playwright has some too. Playwright's bug may just be "don't try to use this broken feature yet".
  • I wonder if we can work around all this by just converting more of the e2e config to esm (e.g. tools/e2e-commons/config/playwright.config.default.cjs)

@ice9js ice9js requested a review from a team October 31, 2023 15:49
@anomiex
Copy link
Contributor Author

anomiex commented Nov 7, 2023

I wonder if we can work around all this by just converting more of the e2e config to esm (e.g. tools/e2e-commons/config/playwright.config.default.cjs)

Did that in #33846.

zinigor
zinigor previously approved these changes Nov 9, 2023
View reviewed changes
Copy link
Member

@zinigor zinigor left a comment

Choose a reason for hiding this comment

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

I really like the cleanup, although the PR needs a conflict resolve again.

zinigor
zinigor previously approved these changes Nov 9, 2023
View reviewed changes
Copy link
Member

@zinigor zinigor left a comment

Choose a reason for hiding this comment

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

Resolved the conflict, let's get it in!

@anomiex
Copy link
Contributor Author

anomiex commented Nov 9, 2023

The thing is that Calypso decided to wait on re-trying their update due to sickness and AFK (p4TIVU-aOV-p2#comment-11982), so they're still on v18. Are we ok with getting ahead of them?

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 9, 2023

Support References

This comment is automatically generated. Please do not edit it.

  • p4tivu-aov-p2#comment-11982

@github-actions github-actions bot added the Customer Report Issues or PRs that were reported via Happiness. aka "Happiness Request", or "User Report" label Nov 9, 2023
@anomiex
Copy link
Contributor Author

anomiex commented Nov 9, 2023

Looks like their new PR is Automattic/wp-calypso#83585

@zinigor
Copy link
Member

zinigor commented Nov 9, 2023

I'd say it's a good idea to wait for them. Their process is much more reliant on Node, and if they find regressions, we'll end up staying on Node 20 ahead of them longer than we expect, potentially causing workflow disruptions for people who work on both environments.

@noahtallen
Copy link
Contributor

noahtallen commented Nov 10, 2023 •
edited
Loading

While we ran out of time to ship this week due to some other priorities, we hope to have that PR shipped early next week. Other than the testing issues we ran into at the last minute, we aren't expecting any major issues hopefully!

@anomiex anomiex added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Blocked / Hold labels Nov 14, 2023
@anomiex
Copy link
Contributor Author

anomiex commented Nov 14, 2023

Calypso updated, so we're good to go here.

@zinigor zinigor enabled auto-merge (squash) November 14, 2023 16:07
zinigor
zinigor approved these changes Nov 14, 2023
View reviewed changes
Copy link
Member

@zinigor zinigor left a comment

Choose a reason for hiding this comment

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

Poor labelling action has died, RIP.

@zinigor zinigor merged commit d3136e5 into trunk Nov 14, 2023
66 of 68 checks passed
@zinigor zinigor deleted the update/node-20 branch November 14, 2023 16:08
@github-actions github-actions bot removed the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zinigor zinigor zinigor approved these changes

Assignees

@anomiex anomiex

Labels
[Action] Repo Gardening Github Action: manage PR and issues in your Open Source project [Action] Required Review [Action] Test Results to Slack Actions GitHub actions used to automate some of the work around releases and repository management Customer Report Issues or PRs that were reported via Happiness. aka "Happiness Request", or "User Report" Docs E2E Tests [Feature] Photon aka "Image CDN". Feature developed in the Image CDN package and shipped in multiple plugins [JS Package] AI Client [JS Package] Analytics [JS Package] API [JS Package] Babel Plugin Replace Textdomain [JS Package] Base Styles [JS Package] Boost Score Api [JS Package] Components [JS Package] Config [JS Package] Connection [JS Package] Eslint Changed [JS Package] Eslint Config Target Es [JS Package] I18n Check Webpack Plugin [JS Package] I18n Loader Webpack Plugin [JS Package] IDC [JS Package] Image Guide [JS Package] Licensing [JS Package] Partner Coupon [JS Package] Publicize Components [JS Package] React Data Sync Client [JS Package] Remove Asset Webpack Plugin [JS Package] Shared Extension Utils [JS Package] Storybook [JS Package] Svelte Data Sync Client [JS Package] Videopress Core [JS Package] Webpack Config [Package] Action Bar This package no longer exists in the monorepo. [Package] Ad aka WordAds [Package] Admin Ui [Package] Assets [Package] Backup [Package] Blaze [Package] Boost Core [Package] Chatbot [Package] Connection [Package] Forms [Package] Google Fonts Provider This package no longer exists in the monorepo. [Package] Identity Crisis This package no longer exists in the monorepo. It was merged into [Package] Connection. [Package] Image CDN [Package] Import [Package] Jetpack mu wpcom WordPress.com Features [Package] Jitm [Package] Lazy Images This package no longer exists in the monorepo. [Package] My Jetpack [Package] Plans [Package] Plugin Deactivation [Package] Publicize [Package] Search Contains core Search functionality for Jetpack and Search plugins [Package] Stats Admin [Package] Transport Helper [Package] VideoPress [Package] WP JS Data Sync [Package] Yoast Promo [Plugin] Boost A feature to speed up the site and improve performance. [Plugin] CRM Issues about the Jetpack CRM plugin [Plugin] Inspect [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Plugin] Migration [Plugin] mu wpcom jetpack-mu-wpcom plugin [Plugin] Protect A plugin with features to protect a site: brute force protection, security scanning, and a WAF. [Plugin] Search A plugin to add an instant search modal to your site to help visitors find content faster. [Plugin] Social Issues about the Jetpack Social plugin [Plugin] Starter Plugin [Plugin] Super Cache A fast caching plugin for WordPress. [Plugin] VideoPress A standalone plugin to add high-quality VideoPress videos to your site. [Pri] Normal RNA [Tests] Includes Tests [Tools] Development CLI The tools/cli to assist during JP development. [Type] Janitorial
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@anomiex @zinigor @noahtallen