Fix: declare @types/node in the CommonJS TypeScript build_setup fixtures - #383
Open
AmaadMartin wants to merge 1 commit into
Open
Conversation
The ts_commonjs and ts_commonjs_native_addon fixtures use the CommonJS ambient globals require and module, which TypeScript only knows about when @types/node is present, but neither manifest declared it. They typecheck today only because @types/node reaches them by accident: the fixture install hoists a copy through unrelated transitive edges, and tsc's automatic @types inclusion also walks up to the repo root's node_modules/@types. Declare the build-time type dependency each fixture actually consumes, at the ^20.12.7 the repo root and dev/package.json already pin. devDependencies rather than dependencies: type definitions are build-time only, matching 59b99b9 ("Move openapi-types to devDependencies").
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
N/A — no existing issue.
Problem: The
tests/integration/build_setup/*fixtures are miniature standalone npmprojects, and
build_setup_test.tsruns a realnpm installplus a realtscbuild in eachone, so a fixture's
package.jsonis meant to be the complete declaration of what it needs.Two of them are incomplete.
ts_commonjsandts_commonjs_native_addonuse the CommonJSambient globals
requireandmodule, which TypeScript only knows about when@types/nodeis present, and neither manifest declares it:
ts_commonjs/agent.tsrequire(...)ts_commonjs/agent.tsmodule.exportsts_commonjs/db_init_check.tsrequire(...)ts_commonjs_native_addon/agent.tsmodule.exportsNeither fixture
tsconfig.jsonsetscompilerOptions.types, sotscauto-includes everypackage it can find under a visible
node_modules/@types. Today that succeeds for two reasons,neither of which the fixture manifest controls:
@types/nodein through unrelated transitive edges.Measured in this checkout with
npm ls @types/nodebefore the change:@types/node@26.1.2at the fixture root, reachable via
@google/genai@2.15.0 -> protobufjs@7.6.5,@google/adk-devtools -> @mikro-orm/{mssql,mysql} -> tedious/mysql2, and@google/adk -> @google-cloud/storage -> retry-request -> @types/request.tsc's automatic@typesinclusion also walks ancestornode_modules/@typesdirectories, so while the fixtures live inside this repo they additionally see the repo
root's own
@types/nodedevDependency.Correction to the original problem statement. The task I was given asserted that deleting
the fixture-local
node_modules/@types/nodebreaks the build. I verified that and it is nottrue in-tree: with only the fixture-local copy removed,
npm run buildstill printedBuild complete, andtsc --noEmit --listFilesshowed it resolving the repo root's copyinstead. I am not shipping that claim. The real, measurable defects are the two below.
Solution: declare the build-time type dependency each fixture actually consumes, adding
only a
devDependenciesblock to the two CommonJS TypeScript manifests:This fixes two things that are real:
project. Extracted from this repo — which is the scenario they simulate — the ancestor
@typesfallback disappears and the manifest is the only source. The negative control belowreproduces exactly that and shows the build failing without the declaration.
against
@types/node@26.1.2, i.e. whatever major the transitive graph happened to hoist.After it they compile against
20.19.43, deduped with every transitive consumer.devDependenciesrather thandependenciesbecause type definitions are build-time only,following commit
59b99b9b("Moveopenapi-typesto devDependencies": "This package providestype definitions and is not needed at runtime, only during development and build processes.").
^20.12.7is not a new fact — it is the exact range already pinned in the rootpackage.jsonand in
dev/package.json, which are the only two@types/nodedeclarations in the repo, so afuture bump stays a single consistent sweep.
Deliberately not done, per scope: no
.tssource is touched (require/module.exportsisthe CommonJS behaviour under test), no
types/typeRootsarray is added to the fixturetsconfigs (that would additionally exclude every other
@typespackage), and the fouruntouched fixtures (
ts_esm,ts_esm_native_addon,js_commonjs,js_esm) are left alone —the ESM TypeScript fixtures use no Node globals, and the JS fixtures are never compiled by
tsc.Collision check.
gh pr list --repo AmaadMartin/adk-js --state open --limit 100plusgh pr diff --name-onlyon every plausibly adjacent PR. One overlap: PR #329 ("declare@google/genaiin the build_setup integration fixture manifests") edits these same two files,adding an entry to
dependencieson the line immediately above my insertion point. It does notadd
@types/node, so it does not land this change, but the two diffs would conflict textually.This PR is therefore stacked on
fix/build-setup-fixtures-declare-genairather than branchedfrom
main, and its diff is exactly the six added lines below. PR #327 also mentions@types/nodebut is a different package (core/package.json) and does not touch these fixtures.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:
[ ] I have added or updated unit tests for my change.
[x] All unit tests pass locally.
No new test file. This change adds zero lines of executable code — it adds a JSON key to two
test fixtures — and
tests/is outside the vitest coverageinclude(
core/src,dev/src,integrations/src), so there is nothing to cover. The existingtests/integration/build_setup/build_setup_test.tsalready is the test: itsbeforeAllrunsa real
npm installthennpm run build(tsc && echo Build complete) in eachts_*fixtureand asserts
stderr === ''and that stdout containsBuild complete. That hook is preciselythe assertion this change concerns.
Integration suite (
npx vitest run --project integration tests/integration/build_setup/build_setup_test.ts):beforeAll)All six
beforeAllbuild hooks pass — including both fixtures this PR touches. The remainingtest failures are pre-existing and environmental in my sandbox, not caused by this change: they
are
npx @google/adk-devtools --versionfailing withnpm ERR! could not determine executable to runand the spawned agent not emittingtest-llm-model-response, and they hit fixtures thisPR never touches (
js_esm,ts_esm,ts_esm_native_addon). I diffed the failing sets: theset with the change is a strict subset of the baseline's, so no test regressed.
Two environment notes for anyone reproducing this locally. The repo configures no
hookTimeout, so vitest's 10s default applies to abeforeAllthat performs a coldnpm install; on a cold npm cache all six suites time out in the hook regardless of thischange. Pre-installing the six fixtures once makes the hooks fit the budget. Passing
--hookTimeouton the CLI does not help — it is not propagated into the project config.Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
Negative control — proof the declaration is load-bearing. The existing test passes both
with and without this change (that is the whole point of the defect), so a green run alone
proves nothing. Because the in-tree build is also satisfied by the repo root's
@types/nodevia ancestor
@typesresolution, the control has to remove both sources — which is exactlythe standalone-project situation these fixtures model:
ts_commonjs,npm run buildwith no@types/nodereachable — exit code 1:ts_commonjs_native_addon, same procedure — exit code 1:Then, with the repo root's copy still hidden,
npm installin each fixture restored@types/node@20.19.43from the fixture's own manifest andnpm run buildprintedBuild completein both. Before this change that restoration was an accident of the transitivegraph; now it is what the manifest asked for.
Local validation in place of CI. This PR is stacked, and the validation workflow triggers on
pull_request: branches: [main], so no test job runs against a non-mainbase. On the exactpushed commit I ran:
npm run build— exit 0npm run lint— exit 0npx vitest run --project integration tests/integration/build_setup/build_setup_test.ts— results in the table abovenpx tsc --noEmit --listFilesints_commonjs— confirms the fixture-local@types/node@20.19.43is what the fixture now compiles againstgit status --shortafter every run shows exactly the two modifiedpackage.jsonfiles: thefixture
node_modules/,dist/, andpackage-lock.jsonare all git-ignored and are removed bythe test's
afterAll. The rootpackage-lock.jsonis untouched — the fixtures are not npmworkspace members.
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.