Repository navigation
chore: consolidate open Dependabot updates (typescript 6, undici 8) - #360
Conversation
TypeScript 6 requires an explicit rootDir when outDir is set (TS5011), so tsconfig.json now declares the './src' root the compiler previously inferred. Emitted dist/ (JS and .d.ts/.d.cts) is byte-identical to the build produced by TypeScript 5.9.3.
Node's built-in fetch reaches the global dispatcher through a wrapper that forces `allowH2: false`. From undici 8.0.3 the agent then looks up an `<origin>#http1-only` client key that MockAgent never registers, so the request escapes to the real network and the test fails with a JSON parse error on live HTML rather than on a missed interceptor (nodejs/undici#5036 -- fix merged, unreleased as of 8.10.0). Calling `install()` puts undici's own fetch on globalThis, which reads the unwrapped dispatcher slot and is intercepted correctly. `disableNetConnect()` makes any future interception failure loud instead of silently hitting the network. Both work on the currently pinned undici 7.29.0, so this commit stands on its own.
Setting rootDir changed where tsc -b computes the default .tsbuildinfo path, moving both files from dist/cjs/ and dist/esm/ up to dist/. Since package.json ships "files": ["dist"], that silently changed the layout of the published tarball. Pinning the paths keeps the published file set byte-for-byte what it was; the only remaining delta is the compiler version string recorded inside the two incremental-build files.
SociableSteve
left a comment
There was a problem hiding this comment.
Approving. Consolidating two long-red Dependabot branches into one branch built on current master, rather than rebasing either, is the right call here, and the body does the thing that makes a consolidation reviewable: it says what each half was, why each was red, and what was deliberately left out.
The check that carries the most weight is the emit comparison, and it holds. I confirmed independently that all eight files under dist/ are byte-identical to the base build, so a TypeScript 5 to 6 major produced no change in shipped output at all. That is what makes this a devDependency bump rather than a release, and it is the claim a green suite would not have proved on its own.
The one item I want to name plainly, because it is the substantive one and the body already does not hide it, is test/index.test.ts:32. Calling install() routes globalThis.fetch through undici's own implementation so MockAgent can intercept it, which means the suite no longer exercises the Node built-in fetch path that the library actually runs on in production. src/index.ts calls global fetch, so from this commit the tested path and the shipped path are different implementations.
I am not treating that as a blocker, for three reasons that are visible in the code rather than inferred. The underlying cause is a real upstream defect, not a shortcut: from undici 8.0.3 Node's fetch wrapper forces allowH2: false, the agent then looks for an <origin>#http1-only client key MockAgent never registers, and the request escapes to the live network. The alternative to the workaround is therefore not "keep testing the real path", it is "tests silently make real HTTP calls", which is strictly worse. The 21-line comment above the call states the mechanism, cites nodejs/undici#5036, and says the fix is merged but unreleased as of 8.10.0. And #359 is open and records the analysis and the follow-up, so it is tracked rather than absorbed. The agent.disableNetConnect() comment at line 17 is likewise careful to say what it does not cover, which is the kind of accuracy that stops a workaround from being mistaken for a guarantee later.
What I would ask is only that #359 stays live until undici ships the fix, since the cost of this workaround is invisible from the test output: the suite will keep passing whether or not the built-in path still works.
There is no prior review history on this PR, so nothing carries forward.
Three non-blocking notes, none of them defects:
-
There is still no
engines.nodefield, andundici@8.10.0declares>=22.19.0. It is a devDependency, so nothing reaches consumers and no published constraint is wrong. The effect is on contributors: a freshnpm installon Node 20 or 22.x below 22.19 now warns rather than being caught, and this bump is what raised the floor. One"engines": { "node": ">=22.19.0" }would surface it at install time instead of at test time. -
The added
tsBuildInfoFilepins intsconfig.cjs.json:5andtsconfig.esm.json:5point insidedist/, andfilesis["dist"], so the two.tsbuildinfofiles ship to npm. I checked the published1.2.0tarball and they are already in it, both at 70 bytes, so this PR preserves existing behaviour rather than introducing it. Worth mentioning only because the pin is a new line in this diff, so this is the moment where pointing them outsidedist/would cost nothing. -
The body opens with "both of the repository's open Dependabot pull requests". #272 and #347 were closed about forty seconds after this PR was opened, so that sentence is out of date rather than wrong: it was accurate when written. Worth a small edit because this repo squash-merges, so the body becomes the commit message and the staleness becomes permanent.
On provenance: the builds, the emit comparison, npm ci, lint and the test runs were the review pass's executions. What I verified in my own hands is the head SHA, both tsconfig files against base, package.json at head and base, test/index.test.ts including the comment quoted above, undici@8.10.0's declared engines, the state of #359, #272 and #347, and the contents of the published tarball.
Consolidates both of the repository's open Dependabot pull requests into one verified change.
Closes #358
What is in here
typescript(devDep)undici(devDep)Nothing is left out. Both bumps are
devDependencies;dependencies(jsrsasign ^11.1.0) isuntouched and the package declares no
peerDependencies, so the consumer contract of the publishedpackage does not change.
Exactly two packages move in
package-lock.json—node_modules/typescriptandnode_modules/undici— both at versions (and integrity hashes) matching Dependabot's own branches.Because the lockfile is
lockfileVersion: 2that shows up as five changed JSON entries: those two,the root
packages[""]devDependency specs, and the legacydependencies.typescript/dependencies.undicimirrors. The lockfile stays onlockfileVersion: 2, the entry count isunchanged at 204, and nothing moved between hoisted and nested positions (compared path-keyed, not
name-keyed). A control
npm installon an unmodifiedmastercheckout with the same npm produceda zero-line lockfile diff, so none of the diff below is npm-version churn. No in-range drift
was picked up either:
@commitlint/cli,@types/node,eslint,lint-stagedandtypescript-eslintall have newer versions available and are deliberately left for Dependabot.Both PRs have been red for a long time (#272 since April), and in neither case was the cause a
lockfile problem. Each needed a small, specific fix.
1. TypeScript 6 needs an explicit
rootDirtypescript@6fails the existing build outright:TypeScript 6 no longer infers the common source directory when
outDiris set, sotsconfig.jsonnow declares the
./srcroot the compiler previously computed.Setting
rootDirhas one side effect worth calling out: it also changes wheretsc -bcomputesthe default
.tsbuildinfopath, moving both files fromdist/cjs/anddist/esm/up todist/.Because
package.jsonships"files": ["dist"], that would have quietly changed the layout of thepublished tarball.
tsBuildInfoFileis therefore pinned to the original locations intsconfig.cjs.json/tsconfig.esm.json, in its own commit.The published output is effectively unchanged. Built with TypeScript 6.0.3 versus 5.9.3 on
master,dist/has an identical file set (npm pack --dry-runlists the same files at the samesizes), and all eight emitted files —
dist/esm/index.js,dist/esm/types.js,dist/esm/index.d.ts,dist/esm/types.d.ts,dist/cjs/index.cjs,dist/cjs/types.cjs,dist/cjs/index.d.cts,dist/cjs/types.d.cts— are byte-identical (cmp). The only differenceanywhere under
dist/is the compiler version string recorded inside the two incremental-buildfiles:
(Those
.tsbuildinfofiles being inside"files"at all is a pre-existing packaging wart; it isleft alone here.)
TypeScript 7 was evaluated and rejected
typescript@7.0.2is the currentlatest. It actually compiles this project fine oncerootDiris set — but it cannot land yet, because
typescript-eslintwill not accept it. Itstypescriptpeer range is
>=4.8.4 <6.1.0on the current8.68.0, and across all published versions theupper bound has only ever been
<5.8.0,<5.9.0,<6.0.0or<6.1.0(<6.1.0first appears in8.58.0); thecanarytag is no different. Installing TypeScript 7 makesnpm ls --allexit 1with an invalid peer and forces the whole
@typescript-eslint/*subtree to nest, growing thelockfile from 204 to 224 entries.
6.0.3— Dependabot's own target — is the highest version thatkeeps the tree peer-clean.
2. undici 8 needs the tests to stop relying on Node's built-in
fetchsrc/index.tscalls Node's built-in globalfetch, and the tests intercept it withMockAgent+setGlobalDispatcher. undici 8 routes the legacy global dispatcher slot that Node'sbundled fetch reads through a
Dispatcher1Wrapper; in 8.0.3 that wrapper started forcingallowH2: false(nodejs/undici#4989), which collides with the`${origin}#http1-only`clientkey
Agent[kDispatch]gained in 8.0.2.MockAgent.get(origin)still registers itsMockPoolunder the plain
origin, so the lookup misses, a real client is created, and the request escapesto the public internet — which is why the failure looks like a data bug
(
SyntaxError: Unexpected token '<', "<!DOCTYPE "...) rather than a missing interceptor.Measured locally on Node 24 with
npm testand the unmodified test file:lib/global.jsguarded the legacy-slot write withif (nodeMajor === 22)allowH2collision described aboveThe fix here is two lines in
test/index.test.ts: call undici'sinstall()before the suite runs.That routes the globals through undici's own implementation, which reads the unwrapped dispatcher
slot and is intercepted correctly.
install()is public, documented andStability: 2 - Stable(
docs/docs/api/GlobalInstallation.md, added in undici 7.11.0). It is version-neutral — the testchange passes on the currently pinned 7.29.0 as well, which is why it is a separate commit from the
bump.
Honest tradeoff:
install()overwrites ten globals process-wide for the lifetime of theprocess —
fetch,Headers,Response,Request,FormData,WebSocket,CloseEvent,ErrorEvent,MessageEvent,EventSource— and the suite therefore exercises userland undici'sfetchrather than Node's bundled one. Onlyfetchmatters to this library today, but it is areal reduction in fidelity and should be reverted once an undici 8.x release carries the upstream
fix: nodejs/undici#5648 was merged on 2026-08-05 and closed nodejs/undici#5036, but 8.10.0 was
published on 2026-08-03 and therefore predates it — and 8.10.0 still reproduces the bug locally.
#359 records the analysis and the follow-up.
agent.disableNetConnect()is also added so that an ordinary unmatched request fails as aMockNotMatchedErrorinstead of silently going to the network. It does not guard against thedispatcher-level bypass above — with
install()removed anddisableNetConnect()left in place,undici 8.10.0 still reaches the live host — and the code comment says so.
One thing to be aware of: undici 8's own
engines.nodeis>=22.19.0, up from>=20.18.1on7.29.0. This repo declares no
enginesand.nvmrcislts/*, so CI is unaffected, but acontributor still on Node 20 will now get
EBADENGINEon install.Note on #272's age
#272 was opened on 2026-04-20 and last touched on 2026-05-25, so its branch predates four months of
master. Merging it would not have regressed anything — a merge only applies what the branchchanged, and it changed nothing outside the
typescriptlines — but its tree is far enough behindthat the bump was rebuilt from
masterhere rather than rebased.Verification
Run on Node 24.18.0 / npm 11.16.0 locally. CI resolves
.nvmrc(lts/*) to the current Node 24LTS — the most recent CI run on this repo used 24.19.0 — so this is the same major and the same
npm 11 line, not the shell default of Node 22 / npm 12.
masternpm cinpm run buildnpm run lintnpm testnpm ls --allnpm installfromgit archiveThat is the exact command sequence the
Lint and testjob runs. All four commits are alsoindependently green.
What the tests do and do not prove
This is a Node library — it calls Node's global
fetchand is consumed from Node — not SuiteScriptrunning inside NetSuite's runtime, so the suite does exercise the real shipped code path. But it is
two tests, and they cover the request path narrowly. Mutation checks confirm what is genuinely
asserted: changing the request URL, changing the HTTP method, or disabling access-token reuse each
turns the suite red. Changing or removing the
Authorizationheader does not — the suiteproves the token is minted once, not that it is ever sent.
Everything below the client — the actual NetSuite RESTlet contract, a real OAuth token exchange,
and
jsrsasign's PS256 signing, which the tests mock out — is not exercised by CI, on thisbranch or on
master. For the TypeScript bump the stronger signal is the byte-identicaldist/comparison above; for the undici bump it is that those assertions pass against a genuinely
intercepted request, which
mastercannot do at all on 8.10.0.