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

Use Node 20, and add it to CI #5012

Merged
merged 12 commits into from
Aug 30, 2024
Merged

Use Node 20, and add it to CI #5012

merged 12 commits into from
Aug 30, 2024

Conversation

p2edwards
Copy link
Contributor

@p2edwards p2edwards commented Jul 15, 2024

Description

Update Node version to 20.17.0 for release builds, and run the frontend tests in Node 20 and 22 in CI.

Related issues

Part of #4703

Notes for devs

  • Current recommended version of Node for dev is now 20.17.0.
    • The easiest way to follow these Node version bumps is with a tool like fnm or nvm.
  • If you're updating npm packages (package-lock.json) yourself, please do so using Node 20.17.0 (or, in the future, the version is specified in the Dockerfile)
  • We'll consider Node 20 the minimum supported version
    • Any 20 or 22 should work too, but you may get some console warnings
    • Node 16.15.0 will continue to work for a few weeks, but we'll soon upgrade npm packages that don't support Node 16.
  • This mainly affects frontend developers (anyone running npm commands manually during their regular dev process, on their host machine not in the kpi container)
    • If you're running these commands inside the Docker container, and your Docker image is up to date, it will come with the OK version of Node.

Details for reviewer

Patches mocha-chrome to allow it to successfully run on Node >16 (see commit message)

Updates the recommended version of Node to v20.17.0 (the active LTS version, newest point release as of 2024-08-23)

  • in the Dockerfile, used by kobo-install and the production builds
  • in .node-version and .nvmrc, supporting popular tooling such as nvm and fnm, which developers may use to conveniently switch to the recommended Node version for a given project on their dev machine
  • in the helper script whch shows up for developers running a different version of Node during npm install.

Runs this version v20.17.0 in the CI npm tests, as well as any available newer Node 20 or Node 22.

  • To avoid invalid cache reuse when new versions become available, we use the actual resolved version as part of the cache key, rather than the generic "20" or "22" specifier.

Bumps various github action versions (actions/checkout, setup-node, setup-python), mainly to avoid deprecation warnings in CI output. Major changelogs look good to me.

Process notes

Like #4354 and #4554, test Node 20 upgrade.

troubleshooting steps

  • npm run watch still works locally

the mocha-chrome problem

  • Fix the broken mocha test runner in CI

Like before, the mocha test runner was in node > 16.

Run npx mocha-chrome test/tests.html --chrome-launcher.connectionPollInterval=5000
  npx mocha-chrome test/tests.html --chrome-launcher.connectionPollInterval=5000
  shell: /usr/bin/bash -e {0}
  
Promise Rejection:  Error: connect ECONNREFUSED ::1:32897
    at TCPConnectWrap.afterConnect [as oncomplete] (node:net:1606:16) {
  errno: -111,
  code: 'ECONNREFUSED',
  syscall: 'connect',
  address: '::1',
  port: 32897
}
Error: The action 'Run Tests, with mocha-chrome' has timed out after 1 minutes.

Node 17 introduced some changes to DNS.lookup, which was the root cause of this timeout. See commit message for the fix.

follow-up

  • Add newer versions of Node to the test matrix, too
  • Figure out if we want to pin a precise version of Node per release, or go with 20.
    (The 16.15.0 -> 16.15.1 breakage is probably not likely to happen again, but we like stability.)
    Conclusion: Pin it

@p2edwards p2edwards added workflow Related to development process dependencies Pull requests that update a dependency file Front end labels Jul 15, 2024
Here's the error we were encountering when running mocha-chrome
on Node >=17:

```
Promise Rejection:  Error: connect ECONNREFUSED ::1:39193
    at TCPConnectWrap.afterConnect [as oncomplete] (node:net:1606:16) {
  errno: -111,
  code: 'ECONNREFUSED',
  syscall: 'connect',
  address: '::1',
  port: 39193
}
```

Here's the patch that fixes this for us:

   // node_modules/mocha-chrome/lib/client.js
-  const client = await CDP({ port: instance.port });
+  const client = await CDP({ port: instance.port, host: '127.0.0.1' });

An alternative patch that would also succeed:

   // node_modules/chrome-remote-interface/lib/defaults.js
-  module.exports.HOST = 'localhost';
+  module.exports.HOST = '127.0.0.1';

Another option:

   // node_modules/chrome-remote-interface/lib/external-request.js
-  const {address} = await util.promisify(dns.lookup)(options.host);
+  const {address} = await util.promisify(dns.lookup)(
+    options.host,
+    {family:'IPv4'},
+  );

Thank you @nsainaney for writing this comment in nodejs/node#40702 [^1]:

> It appears to be a breaking change with how DNS.lookup works. With
> node 16, the lookup would return a IPv4 address but with node 17, it
> returns an IPv6 address which will break most REST clients that
> hardcode URLS like http://localhost:4040/api if the upstream server
> only binds to the IPv4 address (e.g. server.listen('127.0.0.1'…) etc…

[^1]: nodejs/node#40702 (comment)

Before committing to the 'patch' strategy, I checked to see if either
mocha-chrome or chrome-remote-interface had been updated later with this
workaround.

- chrome-remote-interface accepts a parameter for host, so nope.
  Makes sense, since you can specify the host manually.
- mocha-chrome doesn't parameterize host, and the author of mocha-chrome
  considers it an obsolete package and is no longer updating it.
Base automatically changed from react-query-start to beta August 12, 2024 15:29
@p2edwards p2edwards changed the title (WIP / TEST) Use Node 20.15.1, and add it to CI (WIP) Use Node 20, and add it to CI Aug 23, 2024
@p2edwards p2edwards force-pushed the node-20-upgrade-CI-testing branch 2 times, most recently from f2d7dcb to ee0d9fd Compare August 23, 2024 16:04
Use resolved version in the package cache instead of matrix specifier
@p2edwards p2edwards changed the title (WIP) Use Node 20, and add it to CI Use Node 20, and add it to CI Aug 23, 2024
Response to this warning:

> The following actions uses node12 which is deprecated and will be forced
to run on node16: actions/checkout@v2, actions/setup-python@v2. For more
info: https://github.blog/changelog/2023-06-13-github-actions-all-actions-will-run-on-node16-instead-of-node12-by-default/
@p2edwards p2edwards requested a review from jnm August 26, 2024 15:39
@jnm jnm merged commit d235ce1 into beta Aug 30, 2024
7 checks passed
@jnm jnm deleted the node-20-upgrade-CI-testing branch August 30, 2024 15:45
jnm added a commit to jnm/kobo-no-docker that referenced this pull request Sep 26, 2024
jnm added a commit to jnm/kobo-no-docker that referenced this pull request Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file Front end workflow Related to development process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants