Skip to content

chore: declare a supported Node.js range on the published packages - #445

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

chore: declare a supported Node.js range on the published packages#445
AmaadMartin wants to merge 2 commits into
mainfrom
feat/declare-engines-node-range

Conversation

@AmaadMartin

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: None of the three published manifests declares an engines field. Verified on this checkout:

$ grep -rn '"engines"' --include=package.json --exclude-dir=node_modules .
(no matches)

Without engines.node, npm has no machine-readable minimum Node version for @google/adk, @google/adk-devtools or @google/adk-integrations, so installing on an end-of-life Node release produces no EBADENGINE warning at all. The packages are silently accepted on runtimes they are neither built nor tested against.

Solution: Declare "engines": {"node": ">=22.12.0"} on exactly those three manifests, plus one guard test that keeps the three values from drifting apart. This is consumer-facing metadata only — it adds no executable code path, no runtime Node-version check and no startup assertion.

Why >=22.12.0. Two separate numbers matter here and they are not the same:

  1. Technical minimum for consumers. Across the production (non-dev) dependency closure recorded in package-lock.json, the two binding constraints are @opentelemetry/* (27 packages) at ^18.19.0 || >=20.6.0 and @google/genai / @azure/* / @typespec/ts-http-runtime (13 packages) at >=20.0.0. Their intersection is Node >= 20.6.0.
  2. Policy floor actually chosen: 22.12.0. Node 20 reaches end of life on 2026-04-30. Declaring support for it would defeat the purpose of the change. Within the supported LTS lines, 22.12.0 is the lowest release the repository's own toolchain accepts — vite@7.3.5 (pulled in by vitest) declares engines.node: "^20.19.0 || >=22.12.0", whose 22-line floor is exactly 22.12.0. Verified from the lockfile, not from memory.

This is a visible behaviour change, called out deliberately so it can be accepted or rejected consciously. Node 18 and Node 20 consumers will now see an EBADENGINE warning where they previously saw nothing. That is the intended outcome, not an oversight. It is not breaking on the default install path — engines is advisory and the install still succeeds (demonstrated below, exit code 0). It is breaking only for a consumer who has explicitly opted into engine-strict=true and is on an out-of-range Node, which is the documented semantics of that opt-in. No exported symbol, type or signature moves. If a maintainer would rather keep Node 20 alive, the fix is a one-token change to "^20.19.0 || >=22.12.0" in the three manifests plus the test constant.

CI is unaffected. Both .github/workflows/validation.yaml and .github/workflows/cross-language-integration.yml invoke actions/setup-node@v6 with no with: block at all. node-version-file has no default, so the action does not read package.json, .nvmrc or .node-version on its own; adding engines.node cannot change which Node version CI runs on. This change deliberately does not repoint the workflows and does not add a .nvmrc.

Scope. Deliberately limited to the three published manifests. Not touched: the root package.json (workspace container, not published), README.md, .nvmrc/.node-version, either CI workflow, .npmrc (introducing engine-strict would turn a warning into a hard failure for everyone), dev/src/cli/deploy/deploy_utils.ts (FROM node:lts-alpine already resolves to a satisfying version), and the tests/**/package.json fixtures. The package-lock.json hunk is only the three engines entries npm writes for the workspace packages — no dependency versions change.

Collision check. Per process, open PRs on this fork were checked before writing any code:

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

This found #133 "Feat: Establish Node.js version source of truth (.nvmrc, engines.node, CI pins)" (branch feat/node-version-source-of-truth), which overlaps: it also adds engines to these three manifests. It differs in three ways that matter. (a) It declares >=22.0.0, which blesses 22.0–22.11 — releases the repository's own toolchain cannot run on, since vite@7.3.5 needs >=22.12.0 on the 22 line. (b) It has no drift guard. (c) It also creates .nvmrc, repoints both workflows at node-version-file: .nvmrc, and adds engines to the root workspace container — all three explicitly out of scope here, and (b)/(c) are why this is not simply stacked on that branch. These two PRs conflict and only one should land. If #133 is preferred, this PR should be closed and #133 amended to >=22.12.0 plus the guard test; if this one is preferred, #133 should be narrowed to its CI/.nvmrc half. Adjacent PRs checked and cleared by file list: #443 (README.md only), #428/#416/#406 (CI workflow Node pinning only).

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.

One new file, tests/integration/package_manifests/engines_test.ts, guarding the invariant that all three published manifests declare the same range — three copies of one value across three files is the realistic way this rots. It lives under tests/integration/ because it spans all three workspaces rather than belonging to any one of them, following the tests/integration/build_setup/ precedent of resolving paths off process.cwd().

$ npx vitest run --project integration tests/integration/package_manifests/engines_test.ts
 ✓ |integration| tests/integration/package_manifests/engines_test.ts (3 tests) 8ms
 Test Files  1 passed (1)
      Tests  3 passed (3)

No existing test was modified, skipped or deleted. The change adds zero lines of production source (core/src/, dev/src/, integrations/src/ are untouched), so the vitest.config.ts coverage thresholds are unaffected — its coverage.include only covers those three source trees. No any, no @ts-expect-error, no eslint-disable, no coverage suppression was added anywhere in this diff.

Proving the guard test can fail (two mutations). Both were run against the real runner and both failed as intended; the manifest was restored and re-verified green afterwards.

  1. Missing field — deleted the engines block from dev/package.json only:
FAIL tests/integration/package_manifests/engines_test.ts > published package manifests > dev declares the shared engines.node range
AssertionError: expected undefined to be '>=22.12.0' // Object.is equality
  1. Drifted value — set dev/package.json to >=22.0.0 while the other two stayed at >=22.12.0. This is the mutation that matters, because divergence (not absence) is the invariant being guarded, and a test that only caught a missing field would pass on a drifted one:
FAIL tests/integration/package_manifests/engines_test.ts > published package manifests > dev declares the shared engines.node range
AssertionError: expected '>=22.0.0' to be '>=22.12.0' // Object.is equality

Regression check — the existing build-setup suite, which really runs npm install (and npm run build) against these manifests through file: links, still passes:

$ npx vitest run --project integration tests/integration/build_setup/build_setup_test.ts
 ✓ |integration| tests/integration/build_setup/build_setup_test.ts (24 tests | 4 skipped)
 Test Files  1 passed (1)
      Tests  20 passed | 4 skipped (24)

Manual End-to-End (E2E) Tests:

Machine used for all of the below: node -vv22.22.2, npm -v9.2.0.

  1. Confirm the declared values:
$ npm pkg get engines.node -w core -w dev -w integrations
{
  "@google/adk": ">=22.12.0",
  "@google/adk-devtools": ">=22.12.0",
  "@google/adk-integrations": ">=22.12.0"
}
  1. Confirm the range admits and rejects the right versions:
$ npx --yes semver@7 -r ">=22.12.0" 20.19.0 22.11.0 22.12.0 24.0.0
22.12.0
24.0.0
  1. In-range install emits no warning. node -v here is 22.22.2, i.e. inside the declared range, so the correct result is no EBADENGINE line naming our packages:
$ cd tests/integration/build_setup/ts_esm && npm install 2>&1 | tee /tmp/adk_install.log
added 606 packages in 2m
$ grep -i EBADENGINE /tmp/adk_install.log
(no match)
  1. Out-of-range install does emit the warning — the failure path, exercised for real. No second Node runtime was available on this machine, so rather than assert npm's behaviour from documentation, the declared range on core/package.json was temporarily set to >=99.0.0 (a range the current runtime genuinely violates) and the same fixture install was re-run. This proves npm actually reads and reports the field from these manifests end to end:
npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: '@google/adk@1.5.0',
npm WARN EBADENGINE   required: { node: '>=99.0.0' },
npm WARN EBADENGINE   current: { node: 'v22.22.2', npm: '9.2.0' }
npm WARN EBADENGINE }

The install still exited 0 — confirming the warning is advisory and the default install path is not broken. core/package.json was then restored to >=22.12.0, re-verified with npm pkg get, and the fixture's node_modules/package-lock.json removed; that temporary edit is not part of this diff.

  1. Formatting is stable under the lint-staged pre-commit hook (which runs prettier --write over staged **/*.{json,md}), so the committed hunks do not get reshaped on someone else's commit:
$ npx prettier --check core/package.json dev/package.json integrations/package.json tests/integration/package_manifests/engines_test.ts
All matched files use Prettier code style!
  1. Build and lint:
$ npm run build   # exit 0
$ npm run lint    # exit 0
$ npx eslint tests/integration/package_manifests/engines_test.ts   # exit 0, no output

npm run ts:check reports pre-existing errors across core/test/** and two tests/integration/** files on this checkout; that is unchanged by this PR, and grep package_manifests over its output returns zero — the new file contributes no type errors.

  1. The published tarballs carry the field (each workspace's files array already lists package.json):
$ npm pack --dry-run -w core -w dev -w integrations
# package.json present in all three: google-adk-1.5.0.tgz,
# google-adk-devtools-1.5.0.tgz, google-adk-integrations-1.5.0.tgz
  1. Scope containment — git diff --stat against the branch base is exactly the intended files:
 core/package.json                                  |  3 ++
 dev/package.json                                   |  3 ++
 integrations/package.json                          |  3 ++
 package-lock.json                                  |  9 +++++
 .../integration/package_manifests/engines_test.ts  | 41 ++++++++++++++++++++++
 5 files changed, 59 insertions(+)

Note on the PR title. It is intentionally prefixed chore: rather than feat:/fix:, so that release-please does not cut a release for a metadata-only change. Since this repository squash-merges, the PR title becomes the commit message, so please preserve the prefix when merging.

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 2 commits August 1, 2026 02:21
Declare "engines": {"node": ">=22.12.0"} on the three published
manifests (@google/adk, @google/adk-devtools, @google/adk-integrations)
so npm tells a consumer at install time whether their runtime is
supported. Previously no manifest declared engines, so installing on an
end-of-life Node produced no EBADENGINE warning at all.

22.12.0 is the policy floor: the technical minimum across the production
dependency closure is 20.6.0 (@opentelemetry/* declare
"^18.19.0 || >=20.6.0"), but Node 20 reaches end of life on 2026-04-30,
and warning people off EOL runtimes is the point of the change. Within
the supported LTS lines 22.12.0 is the lowest release the repository's
own toolchain accepts (vite@7.3.5 declares "^20.19.0 || >=22.12.0").

The lockfile hunk is the matching engines entry npm writes for each
workspace; no dependency versions change.
… drift

The same range is now written in three files, so the realistic way this
rots is one manifest being bumped and the other two left behind. Assert
all three published workspaces declare the identical range.

Lives under tests/integration/ because it spans all three workspaces
rather than belonging to any one of them, following the
tests/integration/build_setup/ precedent of resolving fixture paths off
process.cwd().
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