-
Notifications
You must be signed in to change notification settings - Fork 10.3k
fix: support node 22 #39349
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
fix: support node 22 #39349
Conversation
@@ -83,15 +83,19 @@ describe(`report.error`, () => { | |||
const generatedError = getErrorMessages( | |||
reporterActions.createLog as jest.Mock | |||
)[0] | |||
expect(generatedError).toMatchSnapshot() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these snapshots contained full stack traces down to the node.js level which weren't stable across node.js versions. it didn't seem like that was material to the tests, so we just simplified this.
@@ -8,7 +8,7 @@ import _ from "lodash" | |||
import { resolve } from "path" | |||
import { setFieldsOnGraphQLNodeType } from "../extend-node-type" | |||
import { generateImageSource } from "../gatsby-plugin-image" | |||
import * as gatsbyCoreUtils from "gatsby-core-utils" | |||
import * as fetchRemoteFileModule from "gatsby-core-utils/fetch-remote-file" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
matching how it's imported in the source module we're testing. this mismatch stopped working in node 20.
df870f9
to
9624f12
Compare
9624f12
to
ad01dde
Compare
bab8e48
to
5db7975
Compare
This mismatch stopped working after node 18.
Some of the connection header defaults depend on the node.js major version wip this is a fix for the headers one
See inline comment
The `lmdb-regeneration` integration test fixture had an `.npmrc` file forcing all packages to be built from source, which caused sharp to fail compilation in Node 20+ CircleCI images (Ubuntu Noble) due to... who knows what, honestly. Since this fixture specifically needs lmdb built from source but not sharp (it's testing something specific to this), this replaces the global `.npmrc` `build-from-source` flag with a targeted `postinstall` script that only rebuilds lmdb from source, allowing sharp to use prebuilt binaries. This resolves the "cannot find -l:libvips-cpp.so.42" linker errors that were blocking integration tests on Node 20+ while preserving the fixture's intended lmdb compilation testing. AFAICT this was... basically the only solution. Whew
to get this bug fix: sysgears/webpack-virtual-modules#172 causing this, unclear when: ``` TypeError: Cannot read properties of null (reading 'fileWatchers') ```
- run unit tests against all three - run integration tests against earliest and latest - make sure to separate npm cache by node version
0915f8a
to
5c86fe5
Compare
@@ -28,12 +32,6 @@ aliases: | |||
environment: | |||
<<: *e2e-executor-env | |||
|
|||
restore_cache: &restore_cache |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these two were CircleCI "aliases" which can't accept parameters (node version), so I changed them to CircleCI "commands"
- bootstrap | ||
- bootstrap-18.2.0 | ||
|
||
integration-test-workflow: &integration-test-workflow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we were using e2e-test-workflow
for both e2e and integration, which was confusing and inflexible. I split them so I could set up the node version parameter and dependency on parameterized bootstrap version.
- run: sudo apt-get update && sudo apt-get install python -y | ||
- <<: *restore_cache | ||
# python is not built in and node-gyp needs it to build lmdb | ||
- run: sudo apt-get update && sudo apt-get install -y python3 python-is-python3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is how you get a python
bin in noble apparently
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If just for node-gyp
, you might be able to get away with just installing setuptools
via pip
unit_tests_node20: | ||
executor: | ||
name: node | ||
image: "20.19" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
latest node 20. hopeful renovate will pick this up.
.circleci/config.yml
Outdated
unit_tests_node22: | ||
executor: | ||
name: node | ||
image: "22.18" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
name: node | ||
image: "22.18" | ||
<<: *test_template | ||
|
||
integration_tests_gatsby_source_wordpress: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't know why this one integration test has a whole custom setup. I didn't look into it. seems to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the main thing I remember different about this test suite from others is that it spawn wordpress app in docker (to source content from), so possibly when the test was introduced it had needs not fullfilled by setup for other test suites
@@ -1228,47 +1236,48 @@ describe(`Query schema`, () => { | |||
} | |||
`) | |||
}) | |||
;(shouldTestGC ? it : it.skip)( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
view this diff with "hide whitespace", it's mostly indentation
7854825
to
8b25061
Compare
Summary
We were previously only testing against Node.js 18 in CI, even though we've set
engines.node
to>=18.0.0
.Gatsby appears to just work with Node.js 20 and 22, with a couple caveats noted below. Most of this PR is adjusting our own tests and test infra to work with these Node.js versions.
Test Infrastructure Changes
Ubuntu Noble Numbat
-based CircleCInode
20 and 22 images (e.g. no includedpython
); the node 18 image used FocalTest Changes
sharp
vs.lmdb
compilation conflict in thelmdb-regeneration
integration test fixture (😰 this was a doozy to figure out! see commit for more)Node.js 22 Compatibility Fixes
webpack-virtual-modules
to fixCannot read properties of null (reading 'fileWatchers')
error(fix). I don't know how this is related to Node.js 22, but it appears to be.
gatsby develop
fails with Node.js 22.7.0, 22.8.0, and 22.9.0There is a critical bug in Node.js (nodejs/node#55145?) affecting versions 22.7.0, 22.8.0, and 22.9.0 that causes
gatsby develop
to fail with the error reported in #39068.Versions before 22.7.0 and version 22.10.0 and later work fine, even before this PR.
Page loads may hang in dev with Node.js ≥20.19.0 or ≥22.14.0 and experimental
DEV_SSR
enabledA change landed in Node.js 20.19.0 and 22.14.0 (and presumably 24) results in requests to the
gatsby develop
dev server to occasionally hang for 15 seconds. This can only occur if you have opted in to the experimentalDEV_SSR
flag.To avoid this, downgrade to Node.js 20.18.3 or 22.13.1 or disable
DEV_SSR
.To Do