Repository navigation
Conversation
The CLI already needs Node 22: `scan` uses `fs.globSync` and `npm test` relies on glob patterns in `node --test`, and neither works in Node 20. `yargs@18` also declares `^22.12.0` for the 22 line. Without an `engines` field, npm installs the package on Node 20 without a warning and `scan` fails at runtime with `fs.globSync is not a function`.
- Add `"engines": { "node": ">=22.12.0" }` to package.json.
- Run CI on Node 22 and 24.
- Add a glob `scan` smoke test to CI so the Node 22 API runs in CI.
- Document the requirement in the README.
Refs ChromeDevTools#9
mangeshraut712
left a comment
mangeshraut712
left a comment
There was a problem hiding this comment.
Good catch on the yargs floor pushing the 22.x minimum to 22.12, and adding the glob scan call to the smoke test means CI now exercises the Node 22-only code path. One suggestion on what the matrix actually tests.
| contents: read | ||
| strategy: | ||
| matrix: | ||
| node-version: ['22', '24'] |
There was a problem hiding this comment.
'22' resolves to the newest 22.x on the runner (22.18+ today), so CI never runs the version the engines field promises, 22.12.0. If something later uses an API or behaviour that only arrived in a later 22 minor, CI stays green while >=22.12.0 users break. Adding the floor explicitly, e.g. node-version: ['22.12.0', '22', '24'], would keep the declared minimum honest.
Related, and worth checking on that floor version: as far as I recall, fs.globSync was still marked experimental for part of the 22.x line (I believe it was stabilised around 22.17 / 24.0). If so, scan on 22.12–22.16 would print an ExperimentalWarning to stderr on every run. That's harmless for exit codes, but noisy for a CLI whose output people pipe. A 22.12.0 matrix leg would show it right away. Optional: fail-fast: false on the matrix so one leg failing doesn't cancel the other.
There was a problem hiding this comment.
Good catch, the warning is real. On 22.12.0 and 22.16.0, scan with a glob prints ExperimentalWarning: globSync is an experimental feature and might change at any time to stderr. On 22.17.0 it does not. I raised the floor to >=22.17.0 in engines and the README, added 22.17.0 as a matrix leg and set fail-fast: false in f13a460. Tests, check and build pass on 22.17.0.
fs.globSync is experimental before Node 22.17.0, so scan with a glob prints an ExperimentalWarning to stderr on 22.12–22.16. Raise the engines floor to 22.17.0, add 22.17.0 as a CI matrix leg so CI runs the declared minimum, and set fail-fast to false.
Part of #9.
The CLI needs Node 22, but
package.jsondoes not say so. On Node 20.18.2:trace-engine-cli scan 'fixtures/*.json' --name RunTaskfails withError: fs.globSync is not a function.npm testfails withCould not find '.../test/**/*.test.ts', becausenode --testdoes not expand globs in Node 20.yargs@18declares^20.19.0 || ^22.12.0 || >=23.fs.globSyncis experimental before 22.17.0, so on 22.12–22.16 everyscanwith a glob prints anExperimentalWarningto stderr. The effective minimum is 22.17.0.Changes
"engines": { "node": ">=22.17.0" }topackage.jsonand the lockfile.fail-fast: false, so CI runs the declared minimum.scancall to the CI smoke test, so CI runs the code path that needs Node 22.Verification
npm run check,npm run buildandnpm test(135 tests) pass on Node 22.17.0, 22.18.0 and 25.3.0.npm packand installed the tarball in a clean project:EBADENGINE ... required: { node: '>=22.12.0' }(before the floor moved to 22.17.0).scanworks.scanon 22.12.0 and 22.16.0: both print theglobSyncExperimentalWarning. 22.17.0 does not.Note: with the matrix, the CI checks are named
check-test-build (22.17.0),check-test-build (22)andcheck-test-build (24). Ifmainrequires a status check namedcheck-test-build, that setting needs an update.