Skip to content

Feat: declare the supported Node.js range (engines.node >=20) and pin the toolchain via .nvmrc - #508

Open
AmaadMartin wants to merge 5 commits into
mainfrom
feat/node-engines-range
Open

Feat: declare the supported Node.js range (engines.node >=20) and pin the toolchain via .nvmrc#508
AmaadMartin wants to merge 5 commits into
mainfrom
feat/node-engines-range

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 2, 2026

Copy link
Copy Markdown
Owner

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

  1. Link to an existing issue (if applicable):
    N/A — no existing issue.
  2. Or, if no issue exists, describe the change:

Problem: A consumer installing @google/adk, @google/adk-devtools or @google/adk-integrations onto an unsupported Node.js runtime gets no signal at all. The only statement of the requirement was prose in README.md:

Prerequisite: ADK for TypeScript requires a current Node.js LTS release.

Prose is not machine-checkable. With no engines field on any published manifest, npm install cannot emit EBADENGINE, engine-strict users cannot fail fast, and tooling that reads engines (Renovate/Dependabot runtime detection, Cloud Run / Cloud Functions runtime inference, volta, policy scanners) has nothing to read. Separately, both actions/setup-node@v6 steps had no with: block at all — the action does not auto-detect version files, so CI ran on whatever Node the GitHub runner image happened to ship that month.

Solution: Two complementary changes, plus doc alignment and one guard test.

  1. Declare the support floor. "engines": {"node": ">=20"} on the three published manifests (core, dev, integrations), byte-identical in all three — they are versioned in lockstep and installed together, so they must not advertise different floors.
  2. Give the repo one Node source of truth. A root .nvmrc pinning 22, read by both workflows via node-version-file.

These two numbers are deliberately different. engines.node is the minimum a consumer may install onto; .nvmrc is the exact major contributors and CI build with. The pin may be stricter than the published floor, never looser.

Why >=20 and not >=22. The floor is derived from package-lock.json, not chosen. Splitting the runtime closure from the dev-only closure by the lockfile's dev markers, the strongest runtime constraints are >=20.0.0 (13 entries: @azure/core-auth, @azure/core-client, @azure-rest/core-client, @azure/core-rest-pipeline) and >=20 (2 entries: @azure/msal-node, commander). commander is a direct dependency of @google/adk-devtools; the @azure/* group reaches the runtime closure via @mikro-orm/mssqltedious@azure/identity. So >=20 is the tightest floor the runtime graph already imposes — this declares a constraint that is already true rather than inventing one. Source code imposes nothing higher: no import.meta.dirname, node:sqlite, fs.glob, process.loadEnvFile, or require(esm) anywhere under core/src, dev/src, integrations/src, and tsconfig.json targets ES2020/ES2022.

Raising the floor to >=22 would drop Node 20 — a supported LTS line that every runtime dependency works on — for no reason, and that would be a genuine breaking change.

Why .nvmrc is 22. The dev toolchain floor is genuinely higher than the published floor: vite (via vitest) declares ^20.19.0 || >=22.12.0, five copies of eslint-visitor-keys (via typescript-eslint) declare ^20.19.0 || ^22.13.0 || >=24, and lint-staged declares >=20.17. A bare Node 20.0.0 satisfies the published engines but not the dev toolchain. 22 resolves to the latest 22.x and satisfies all of them.

Behaviour change. Not breaking in practice; disclosing it anyway:

  • npm treats engines as advisory by default, so the overwhelming majority of installs are unaffected and simply gain an npm WARN EBADENGINE ... required: { node: '>=20' } on Node <20.
  • For a consumer who has opted into engine-strict=true and is on Node <20, npm install @google/adk changes from succeeding to failing. That population is small and, by construction, asked to be told. Node 18 reached EOL on 2025-04-30, and the runtime graph (commander, @azure/*) already requires >=20, so such an install is already broken — this makes it fail early and legibly instead of late and cryptically.
  • A root npm install on Node <20 now prints three EBADENGINE warnings. Install still succeeds. This is intended; it is not suppressed.
  • No API, type, or export changes. Zero lines under core/src/**, dev/src/**, integrations/src/** change.
  • CI now installs Node 22 explicitly instead of using the runner image default. This is a real change to what CI executes and is the main risk carried by this PR — see the fixture run under Testing.

Deliberately out of scope (each a real thing a reviewer might expect): no engines on the root manifest (it is a workspace root, not a published artifact, and npm validates the three workspace manifests during a root install anyway); no engines.npm; no engines on the tests/integration/** fixtures or the cli_create.ts scaffold template; no engine-strict=true in a repo .npmrc (that converts a warning into a hard failure for every contributor); no node-version matrix axis in CI; dev/src/cli/deploy/deploy_utils.ts (FROM node:lts-alpine) untouched.

Collision check. Run before writing any code, per contribution triage:

gh pr list --repo AmaadMartin/adk-js --state open --limit 1000 --json number,title,headRefName

This area is contended — five open PRs on the fork touch adjacent files: #133 (.nvmrc + engines on all three manifests and the root + both workflows, floor >=22.0.0), #445 (engines >=22.12.0 + a guard test), #416 (.nvmrc + both workflows), #443 (README only, quotes 20.19), #428 (CONTRIBUTING + both workflows, pins node-version: '24' directly rather than via .nvmrc). This PR is not a duplicate of any of them and differs substantively rather than cosmetically:

Whichever of these lands first, the rest will need rebasing onto it; the guard test added here is the thing that makes a divergence between those four numbers fail loudly instead of silently.

Testing Plan

Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.

Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.

New file tests/integration/packaging/engines_test.ts (5 cases). It lives in tests/integration/ because the assertion spans all three workspaces so it belongs to no single package, and because integration is the only repo-level Vitest project actually executed by npm run test:coverage (note unit:integrations is not in that project list, so a test placed in integrations/test/ would never run). No existing test was modified or deleted — this change alters no behaviour any current test observes.

$ npx vitest run --project integration tests/integration/packaging/engines_test.ts
 ✓ published package manifests > core declares the supported Node.js engine range
 ✓ published package manifests > dev declares the supported Node.js engine range
 ✓ published package manifests > integrations declares the supported Node.js engine range
 ✓ toolchain Node version pin > pins a .nvmrc version that satisfies the published range
 ✓ toolchain Node version pin > resolves every setup-node step from .nvmrc
 Test Files  1 passed (1)      Tests  5 passed (5)

Proof each test can fail. Coverage of a declarative change is vacuous (zero new lines under the coverage include globs, thresholds untouched), so each assertion was run against a mutated input and confirmed to fail with the targeted message. Every mutation was reverted and the suite re-confirmed green afterwards.

# Mutation Result
1 core/package.json engines.node">=22" ✅ fails core onlyAssertionError: expected '>=22' to be '>=20'
2 delete the engines block from integrations/package.json ✅ fails integrations onlyAssertionError: expected undefined to be '>=20'
3 .nvmrc18 ✅ fails — AssertionError: expected 18 to be greater than or equal to 20
4 remove the with: block from cross-language-integration.yml ✅ fails and names the workflowAssertionError: cross-language-integration.yml must resolve Node from .nvmrc: expected undefined to be '.nvmrc'
5 rename both actions/setup-node@v6actions/setup-nodejs@v6 so the walk matches nothing ✅ fails — AssertionError: expected 0 to be greater than 0 (the anti-vacuity guard: the test cannot pass by finding no steps)

Mutation 5 is not in the spec; it was added because cases 1–4 all leave the walk intact, so none of them would catch the test silently degenerating into an empty loop.

Manual End-to-End (E2E) Tests:

Verified the engines field actually reaches the published artifact (npm pack depends on a built dist/, so this stays a manual gate rather than an automated test):

$ node --version
v22.22.2                      # matches .nvmrc
$ npm run build
$ npm pack --workspaces --pack-destination /tmp/adk-engines-check
$ for f in /tmp/adk-engines-check/*.tgz; do echo "== $f"; \
    tar -xzOf "$f" package/package.json | node -e '...JSON.parse(s).engines'; done
== google-adk-1.5.0.tgz
{"node":">=20"}
== google-adk-devtools-1.5.0.tgz
{"node":">=20"}
== google-adk-integrations-1.5.0.tgz
{"node":">=20"}

Full local validation on the exact pushed commit:

npm install                  # package-lock.json UNCHANGED (verified: git diff --stat package-lock.json is empty)
npm run build                # PASS
npx vitest run --project integration tests/integration/packaging/engines_test.ts   # PASS (5/5)
npx vitest run --project integration tests/integration/build_setup/build_setup_test.ts  # PASS (20 passed | 4 skipped, 566s)
npm run lint                 # PASS (clean)
npm run format:check         # PASS — "All matched files use Prettier code style!"
npm run ts:check             # see note below

Two disclosures on the checks above:

  • npm run ts:check is red on the base branch already — 281 pre-existing errors across 41 files (core/test/**, plus two tests/integration/** files), none of which this PR touches. I measured it both ways: 281 errors with the change, 281 errors with the change stashed — identical. My new test file does not appear in the error list. I did not attempt to fix the pre-existing 281; that is unrelated churn and belongs in its own PR.
  • package-lock.json is intentionally not in this diff. engines is manifest metadata and does not affect resolution; a full npm install on Node 22.22.2 / npm 10 left the lockfile byte-identical, so there is no hunk to include. (Two of the sibling PRs above do carry a lockfile hunk here; it is not required.)

CI result on this PR: all checks greenrun-tests (ubuntu-latest), run-tests (macos-latest), run-tests (windows-latest) and the cross-language run-tests all pass.

Because this PR changes the Node version CI executes on, here is the direct evidence that the pin took effect and what it changed:

  • All three matrix legs resolved .nvmrc and reported node: v22.23.1, Found in cache @ /Users/runner/hostedtoolcache/node/22.23.1/arm64 — so no download cost; the version is already in the runner tool cache.
  • The drift this PR removes was real and measurable: on a run of the same workflow without the pin, the macOS leg reported node: v24.18.0. CI was silently running Node 24 while the repo's own @types/node is ^20.12.7. That gap is exactly what node-version-file closes.

One honest disclosure about the first attempt: the macOS leg initially failed with two 40s timeouts in tests/integration/app_loader/app_loader_test.ts (a file this PR does not touch). I did not paper over it — I re-ran it on the identical commit and pin, and it passed. The numbers show why it was runner variance rather than a Node-version regression:

Run Node transform total result
pre-pin baseline (other branch) v24.18.0 52.3s 202.8s pass
this PR, attempt 1 v22.23.1 164.0s 333.4s 2 timeouts
this PR, attempt 2 (same commit) v22.23.1 67.9s 317.6s pass (225 files)

A later commit on this same branch then passed all three legs first-try (macOS in 4m10s), which is a third data point on the same pin. The spread between two runs on the same Node 22 (164s vs 68s transform, 2.4×) is far larger than the gap between Node 22 and Node 24 (68s vs 52s), so the timeout is macOS runner variance meeting a tight 40s per-test budget, not something the pin caused. I deliberately did not change .nvmrc to make this green, and did not touch that test's timeout — it is a pre-existing fragility in a file unrelated to this change and belongs in its own PR.

The build_setup fixture suite was run specifically because it is the one place the CI Node pin can actually bite: its ts_*_native_addon fixtures npm install and compile a native addon in a beforeAll hook and are therefore sensitive to NODE_MODULE_VERSION.

One limitation of this Testing Plan worth stating plainly: .nvmrc pins 22 while the published floor is >=20, so no CI job actually executes on Node 20. The declared floor is backed by the dependency-graph evidence above and pinned by the guard test, but it is not exercised by a running job. That is a deliberate trade-off — one toolchain version rather than a node-version matrix, which would multiply the job count on an already-heavy three-OS workflow — and the engines field communicates the floor to consumers either way. Adding that matrix is worth doing in its own reviewable PR, not here.

Not performed: I do not have a Node 18 toolchain available locally, so I did not empirically observe the EBADENGINE warning text a Node 18 consumer would see. The warning behaviour is npm's, driven entirely by the declared field, but I am flagging it rather than claiming a check I did not run.

Checklist

[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.

Amaad Martin added 4 commits August 2, 2026 01:13
…published packages

The three published manifests carried no engines field, so npm could not emit
EBADENGINE and nothing in the ecosystem could read the supported runtime.
>=20 is the tightest floor the runtime dependency closure already imposes
(commander requires >=20; the @azure/* group reached via
@mikro-orm/mssql -> tedious -> @azure/identity requires >=20.0.0), so this
declares a constraint that is already true rather than inventing one.
…rom it

Both actions/setup-node@v6 steps had no with: block at all. The action does not
auto-detect version files, so CI ran on whatever Node the runner image happened
to ship that month. Pinning via node-version-file gives the repo one source of
truth that nvm/fnm and CI both read.

.nvmrc is 22 rather than 20: the dev toolchain needs it (vite requires
^20.19.0 || >=22.12.0, eslint-visitor-keys ^20.19.0 || ^22.13.0 || >=24,
lint-staged >=20.17). The published floor stays >=20 -- a library's support
floor is about what consumers may run, not what its build tools need.
README quoted a vague 'current Node.js LTS release' that no tool could check;
it now names the declared floor. CONTRIBUTING never mentioned a Node version at
all, so a contributor had no way to know which one CI uses.
Pins the three values this change introduces so they cannot silently diverge:
every published manifest declares the same range, the .nvmrc pin satisfies that
range, and every setup-node step in the repo resolves its version from .nvmrc.
The last assertion is what keeps .nvmrc from degenerating into a decorative
file, and it counts the steps it found so it cannot pass vacuously.
…ing it back

SUPPORTED_NODE_RANGE is a file-local literal, so re-deriving the floor from it
with a regex could never fail -- the guard around that parse was unreachable by
construction. Making the major the source constant and building the range
string from it removes the dead branch and the duplicated number.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant