Update Calypso to Node 20

[noahtallen]

This supersedes #81798. We had to revert that PR due to a unit test failure which appeared on trunk. We didn't catch it in our branch due to an oversight with our unit test setup. (The specific change in this branch compared to the original PR is c5c1d64)

The only reason we had to revert was tschaub/mock-fs#380, which is that certain Node FS methods don't get mocked properly with the mock-fs package under node 20. A small manual mock seems to fix the issue.

The specific reason that the unit tests build is because of our base image setup. This base image is rebuilt twice per day, and includes all the binaries and tools for building and testing Calypso. As a result, this contains the Node image. Therefore, the branch unit test build points at the current "latest" base image, which does not include Node 20. Once we merged the PR, and run the "base image" build, then the latest base image includes "node 20." After that, every build started using the new Node version, including unit tests. While we tested a few builds (such as calypso.live) using a test base image which included Node 20, we didn't do that for the unit tests.

There is definitely room for improvement with how we test these PRs, but here's what we can do now:

• Use our existing test base image (with the tag test-node-20)
• Manually run a unit test build pointing to this tag and branch

[github-actions]

Calypso Live (direct link)

https://calypso.live?image=registry.a8c.com/calypso/app:build-87502

Jetpack Cloud live (direct link)

https://calypso.live?image=registry.a8c.com/calypso/app:build-87502&env=jetpack

[matticbot]

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

[noahtallen]

I manually triggered several builds using the node 20 base image:

• docker image build with node 20: https://teamcity.a8c.com/buildConfiguration/calypso_BuildDockerImage/11095884
• unit test build : https://teamcity.a8c.com/buildConfiguration/calypso_calypso_WebApp_Run_All_Unit_Tests/11095889
• lint: https://teamcity.a8c.com/buildConfiguration/calypso_calypso_WebApp_Check_Code_Style_Branch/11095906
• desktop e2e: https://teamcity.a8c.com/buildConfiguration/calypso_calypso_WebApp_Calypso_E2E_Playwright_desktop/11095920

[noahtallen]

The new test failure is a regression in NodeJS from what I can tell. Try this code in the Node repl (e.g. just execute node by itself):

console.log(new Intl.NumberFormat('en-ZA', { style: 'currency', currency: 'ZAR' }).format(12.34));

• Node version 18: R 12,34
• Node version 20: R 12.34
• Browser (Firefox): R 12,34

From what I understand, the comma is the correct variant. I reported this to Node as it doesn't seem like an issue on our side: nodejs/node#50477.

Now, as far as I know we never render currencies using Node (in SSR), so the browser would be executing the formatter function meaning this wouldn't impact customers. Should we skip this test until they fix it, or patch our formatter to replace decimals with commas if ZAR is the currency? Curious for your thoughts @sirbrillig

[flootr]

nodejs/node#50477 (comment) links to https://unicode-org.atlassian.net/browse/CLDR-14707 which indicates this change happened on purpose so we might want to adapt our tests accordingly? cc @sirbrillig

[sirbrillig]

Should we skip this test until they fix it, or patch our formatter to replace decimals with commas if ZAR is the currency?

Interesting! For reference, on my sandbox with PHP, I see we use a comma for ZAR in en-ZA, but I'm not certain which version of CLDR that's using. On my local computer with PHP, we use a period, so it appears that this is being updated across all systems that implement CLDR formatting.

I've encountered a similar issue before on the backend and the way I resolved it was to make the currency unit tests accept multiple formats (in this case, either a comma or a period) for the specific currency which is transitioning.

client/my-sites/store/app/store-stats/test/utils.js is not a test suite I'm familiar with, so we should probably check with its maintainers but I'd say that it might be best to make the test support either both separators or whatever the current version of JS says is the separator, which can be derived from Intl.NumberFormat.

[flootr]

I'd say that it might be best to make the test support (…) both separators

👍🏻 Added via f61e50e

The tests seem to be quite old (#16035) and I'm not exactly sure what implementation they were testing back then. To me it looks like today we're defaulting to Intl.NumberFormat (via @automattic/format-currency). The tests can probably be improved by either mocking away the formatCurrency dependency or spying on it to see whether it get's called when it should be with the right params but I feel like that's out of scope for this PR.

[matticbot]

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

• happy-blocks

To test WordPress.com changes, run install-plugin.sh $pluginSlug update-node-20 on your sandbox.

[noahtallen]

I generated a new base image with the tag node-20-2: https://teamcity.a8c.com/buildConfiguration/calypso_BuildBaseImages/11194496

I triggered more builds using that image:

• Normal docker image: https://teamcity.a8c.com/buildConfiguration/calypso_BuildDockerImage/11194717
• Unit tests: https://teamcity.a8c.com/buildConfiguration/calypso_calypso_WebApp_Run_All_Unit_Tests/11194701
• Linter: https://teamcity.a8c.com/buildConfiguration/calypso_calypso_WebApp_Check_Code_Style_Branch/11194740 (compare to trunk here: https://teamcity.a8c.com/buildConfiguration/calypso_calypso_WebApp_Check_Code_Style_Branch/11194829)
• e2e: https://teamcity.a8c.com/buildConfiguration/calypso_calypso_WebApp_Calypso_E2E_Playwright_desktop/11194834

All these tests are passing. (for the linter, you can see the same number of failures in this branch and trunk.)

And here is a calypso.live image which should be running Node 20: https://container-mystifying-carver.calypso.live/