Skip to content

build(COMPASS-28): move to ts-morph 28 - #2729

Draft
timdawborn wants to merge 1 commit into
compass-28-spectral-corefrom
compass-28-ts-morph-28
Draft

build(COMPASS-28): move to ts-morph 28#2729
timdawborn wants to merge 1 commit into
compass-28-spectral-corefrom
compass-28-ts-morph-28

Conversation

@timdawborn

@timdawborn timdawborn commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Ticket

COMPASS-28 — Bump spot's NodeJS minimum from 18 to 22

PR 8 of 9. Staying in draft until the whole chain is verified.

Stacked on #2724#2723#2722#2721#2720#2719. This PR's own commit is 0f8a610.

What

ts-morph 18.0.0^28.0.0. That swaps @ts-morph/common 0.19 for 0.29, and with it the vendored compiler that parses every user contract: TypeScript 5.0.2 → 6.0.2. Supersedes Dependabot #2591.

Generated output does not change

The headline risk was a silent shift in what spot emits. It did not happen — verified with a wider net than the snapshots:

generated: 318 combination(s); skipped: 50     # ts-morph 18
generated: 318 combination(s); skipped: 50     # ts-morph 28
diff -r /tmp/tm-before-full /tmp/tm-after-full # empty

Every generator × language combination for test-fixtures/contract/api.ts and each generator's __spec-examples__ contracts — 318 artifacts across ~40 contracts — is byte-identical, and the same 50 combinations decline to parse as before, so nothing changed about which contracts are accepted either.

Three things TypeScript 6 broke, none of which the build or the suite reported

pnpm build passed and pnpm test looked normal at first. Every actual command failed.

1. TS 6 errors on deprecated options instead of warning.

Error: Option 'baseUrl' is deprecated and will stop functioning in TypeScript 7.0.
Error: Option 'moduleResolution=node10' is deprecated and will stop functioning in TypeScript 7.0.

baseUrl is deleted, not silenced — the single paths entry is already an absolute path, so it was doing nothing; confirmed contracts still resolve @airtasker/spot without it. node10 is kept behind ignoreDeprecations: "6.0", because the replacements (Bundler, Node16) change how a contract's own imports resolve. That is published behaviour, so it is COMPASS-31 — which also has to land before TS 7 removes the option outright.

2. TS 6 stopped including node_modules/@types automatically, so a contract importing a node builtin stopped type-checking. Confirmed against both versions with the same contract:

result
ts-morph 18 Contract is valid
ts-morph 28 Error: Cannot find name 'path'

types: ["node"] restores it. There is no way back to the old automatic behaviourtypeRoots alone does not re-enable it — so a contract relying on some other ambient @types package is a narrowing this does not cover. Flagging that explicitly rather than describing this as a clean restoration.

3. typeRoots then has to point at Spot's own copy. The default walks up from the working directory, and the image's working directory is a mounted workspace with no node_modules at all. So @types/node moves to dependencies to reach an image built with --prod — alongside typescript and eslint, which are already runtime dependencies for the same kind of reason.

That one had a false summit: with @types/node in the image but typeRoots unset, the image still failed on a clean workspace while passing when I happened to mount the repo, which has node_modules.

Both halves are covered, not asserted

Mutation Caught by
drop types: ["node"] new lib/src/ts-project.spec.ts — fails with Cannot find name 'path'
drop typeRoots existing docker-parity self-containment case — FAIL: the image could not process a contract outside a Node project

The second is the useful one: scripts/check-image-parity already runs the image against a mktemp -d it verifies has no node_modules on any parent path, so CI guards the image path without anything new. Confirmed by building a mutant image and watching parity fail on exactly that case.

Also confirms a claim from #2719

#2719 worked around @types/node@22 failing under ts-morph 18's vendored TS 5.0.2 (CompressionStream circularity) and said the skew would go away when ts-morph moved. It has: under TS 6.0.2 the same probe reports 0 diagnostics even without skipLibCheck. skipLibCheck stays for its own documented reason — a contract author cannot fix errors inside a declaration file.

How this was verified

Check Result
pnpm build 0
pnpm test 0 — 55 suites, 558 tests, 44 snapshots
pnpm lint:check 0
pnpm build-docs 0
docker build + check-image-parity 0 — all checks passed
generated-output diff, 318 artifacts empty
image against a clean mounted workspace Contract is valid, Generated .../api.yml
--prod install + bin/run validate 0 (the step that catches a misdeclared runtime dep)

@timdawborn
timdawborn changed the base branch from master to compass-28-spectral-core August 20, 2026 00:10
@timdawborn

Copy link
Copy Markdown
Contributor Author

Automated review — /pr-review-toolkit:review-pr

Agents run: code-reviewer, comment-analyzer. Scoped to this PR's own commit (0f8a610).

Both agents independently landed on the same finding: the narrowing is real, and the file doesn't say so.

🔴 Critical (1)

lib/src/ts-project.ts:53-60 — the ambient-types narrowing is a published behaviour change recorded only in the commit message.

types: ["node"] plus a typeRoots into Spot's own tree means a consumer running spot generate / ts-lint in their own repo no longer gets any of their own node_modules/@types, where TypeScript 5 auto-included all of them. Verified: nodeTypesRoot() resolves to node_modules/.pnpm/@types+node@22.20.1/node_modules/@types, whose contents are exactly ['node'] — so no other ambient package is reachable, ever.

This PR's description admits it ("a narrowing this does not cover"); the file does not, and there is no README or docs note. Per pull-requests.md a known gap needs a ticket at the site, not narrative in a merged commit — COMPASS-31 covers moduleResolution, not this. The next reader sees types: ["node"] as canonical with no way to tell it's a deliberate regression.

🟡 Important (1)

lib/src/ts-project.ts:5 — the docblock overclaims and hides a package-manager-dependent trap. "The directory holding Spot's own @types packages" is plural; under pnpm it holds exactly one. Under npm/yarn hoisting the same expression returns the shared @types dir holding everything. Identical today because types names only node — but the moment anyone adds a second entry it resolves under npm and fails under pnpm, and this comment is what tells them it should work.

🔵 Suggestions (4)

  • :48-49 — "rather than part of a dependency bump" narrates this PR and goes stale on merge. State the decision only.
  • :46 — "refuses to run" is loose; it's an options diagnostic. Also the comment says node10 while the code says ts.ModuleResolutionKind.NodeJs — someone grepping node10 finds only the comment.
  • :58-59 — the @types/node-in-dependencies rationale lives only here; nothing at the package.json entry stops a later tidy-up from moving it back to devDependencies and breaking the image.
  • ts-project.spec.ts:8 — "stops parsing" is the wrong vocabulary (it parses and fails type-checking). Worth one line noting the spec does not pin typeRoots: verified it still passes with default typeRoots from the repo root, so it must not be read as covering both halves. (The typeRoots half is covered by parity, as the description says.)

✅ Verified sound

  • require.resolve is safe here — declared runtime dependency, @types/node@22.20.1 has no exports field so the subpath resolves, and parity proves it survives pnpm symlinks under --prod. An ESM move isn't newly blocked: path.join(__dirname, "lib") already requires CJS.
  • @types/* in dependencies is justified and precedented (typescript, eslint are runtime deps for the same reason). A consumer with an incompatible @types/node major gets a nested copy their own tsc won't auto-include — harmless.
  • ignoreDeprecations never reaches the repo's own typescript@5.9 — all four callers of contractCompilerOptions run through ts-morph's vendored TS 6. No bug there.
  • The coverage claim holds: parse() throws on any pre-emit diagnostic, so a typeRoots that cannot resolve types: ["node"] fails the parity self-containment case.
  • The nodeTypesRoot() "not a fixed number of .. steps" rationale is accurate and worth keeping.

Minor note

nodeTypesRoot() is called eagerly at module level, so a throw is a bare MODULE_NOT_FOUND before any command's error handling. Only reachable on a broken install, but moving the call inside createContractProject() costs nothing.

Recommended action

  1. Put the narrowing in the file with a follow-up ticket — that's the one finding both agents raised independently, and it's the difference between a documented trade-off and a silent regression.
  2. Fix the plural in the nodeTypesRoot() docblock and note the npm/pnpm divergence.
  3. Then the comment tidy-ups.

`ts-morph` 18.0.0 to ^28.0.0, which swaps `@ts-morph/common` 0.19 for 0.29
and with it the vendored compiler that parses every contract: TypeScript
5.0.2 to 6.0.2.

Generated output does not change. Every generator and language combination
for the contract fixture and each generator's spec-example contracts — 318
artifacts across 40 contracts — is byte-identical to ts-morph 18, and the
same 50 combinations decline to parse as before.

Getting there needed three changes to the contract compiler options, none
of which the type checker or the suite would have reported on their own.

TypeScript 6 refuses to run against two deprecated options rather than
warning, so every command failed outright: `baseUrl` and
`moduleResolution: node10`. `baseUrl` is deleted, not silenced — the single
`paths` entry is already absolute, so it was doing nothing, and contracts
still resolve `@airtasker/spot` without it. `node10` is kept behind
`ignoreDeprecations: "6.0"`, because moving off it changes how a contract's
own imports resolve. That is published behaviour and belongs in COMPASS-31,
which also has to land before TypeScript 7 removes the option.

TypeScript 6 also stopped pulling in `node_modules/@types` by itself, which
made a contract importing a node builtin stop type-checking. Confirmed
against both versions: the same contract is valid on ts-morph 18 and fails
on 28 with "Cannot find name 'path'". Naming `types: ["node"]` restores
that case, and `typeRoots` has to point at Spot's own copy, because the
default walks up from the working directory and the image's is a mounted
workspace with no `node_modules`. `@types/node` moves to `dependencies` so
it reaches an image built with `--prod`.

`types` and `typeRoots` together are the whole of what a contract can rely
on ambiently now, where TypeScript 5 gave it every resolvable `@types`
package. No option restores that, and under pnpm the type root holds only
`node`, so the narrowing is structural rather than a policy setting. The
options object says so and points at COMPASS-32, where whether to give the
reach back is decided.

Both halves of the fix are covered rather than asserted. Dropping `types`
fails the new `ts-project` spec; dropping `typeRoots` fails the parity job's
self-containment case, which runs the image against a directory with no
`node_modules` on any parent path.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@timdawborn
timdawborn force-pushed the compass-28-ts-morph-28 branch from 0f8a610 to 2e76e4f Compare August 20, 2026 03:39
@timdawborn

Copy link
Copy Markdown
Contributor Author

Automated review, round 2 — /pr-review-toolkit:review-pr

Agents: code-reviewer, comment-analyzer. Scoped to the revised commit 2e76e4f.

Round 1 only touched comments here, so the code got a fresh look — and it found three things round 1 missed, including one I introduced without noticing.

🔴 Critical (1)

lib/src/ts-project.ts:67 — overclaim that would mislead badly. "These two are the whole of what a contract can rely on ambiently" is false. With types and typeRoots* both deleted, Promise, MapandSymbol.asyncIteratorstill resolve — thetarget: ESNextlibs are ambient and untouched by either option. A reader taking it literally concludes ES globals are gone. Suggested: "…the whole of the@typesreach a contract has; the ES libstarget` pulls in are unaffected."

🟡 Important (3)

1. I loosened a deliberately-exact pin, silently. master had "ts-morph": "18.0.0" — no caret. This PR makes it "^28.0.0", and the commit message explains every other decision but not that one. The whole PR is about the vendored compiler determining parse behaviour, and the caret drops the guarantee that pinned it.

Worse, nothing holds the line for npm consumers: files lists npm-shrinkwrap.json, but prepack never generates it and the file does not exist (verified). The image is built from pnpm-lock.yaml; the npm package is not. So the two artefacts docker-parity compares can drift apart after release. Only 28.0.0 exists in 28.x today, so there is no live drift — but the protection is gone. Restore 28.0.0 exact, or say why a caret is now safe.

2. typeRoots resolves to the consumer's @types, not Spot's. Under npm/yarn hoisting, require.resolve("@types/node/package.json") lands in the consumer's hoisted tree, so contracts type-check against their @types/node version, not Spot's ^22.20.1. My docstring notes the directory differs across package managers but not that the version does. A contract can pass in the image and fail on a dev machine — precisely the hermeticity the paths entry above it exists to provide.

3. @types/node in dependencies leaks node globals into consumer projects. An npm/yarn consumer with no @types/node of their own gets Spot's hoisted to top-level node_modules/@types/node, which their tsc then includes ambiently — process and Buffer silently available in, say, a browser-targeted project. (The two-copies case is benign: nested, not picked up.)

Both 2 and 3 disappear if @types/node stays a devDependency and the Dockerfile COPY --from=builders the type root into the image. Worth weighing against the simplicity of the current approach.

🔵 Suggestions (4)

  • :52 overclaims — "refuses to run without this". The compiler starts; it emits an error diagnostic on every program, which is why every command fails. State the mechanism.
  • :55 narrates this PR — "rather than part of a dependency bump" is dead on merge. Cut it and let COMPASS-31 carry it.
  • :12-14 is unverified for yarn Berry — under PnP there is no node_modules, require.resolve returns a zip path, and dirname² is not a type root at all. Say "a hoisting installer (npm, yarn classic)".
  • ts-project.spec.ts:9 states location where the mechanism matters: it passes because jest's cwd is the repo root and default roots walk up from cwd. The claim dies silently if the spec is ever run from elsewhere.

Sibling claim not addressed

README.md:80 — "all you need is to write valid TypeScript code. Your editor will immediately tell you when your code is invalid." The editor uses the repo's own tsconfig with every @types hoisted; Spot now accepts only node. A contract can be green in the editor and fail spot validate. COMPASS-32 option 1 proposes documenting exactly this, but nothing in the repo points there.

✅ Verified sound

  • Every rewritten comment claim was checked by running it: pnpm type root holds only ["node"]; a temp npm install holds ["node","semver"]; a second types entry fails under pnpm ("Cannot find type definition file for 'jest'"); dropping types gives 0 parse diagnostics and 1 semantic; the spec does pass under default typeRoots.
  • COMPASS-31 and COMPASS-32 are correctly attached and each disclaims the other's scope.
  • ignoreDeprecations fails loudly if it ever becomes invalid — TS 6.0.2 emits Invalid value for '--ignoreDeprecations' plus the node10 error, and parser.ts throws on any pre-emit diagnostic, so CI fails too.
  • Nothing else in the options object is being silenced — with ignoreDeprecations deleted, exactly one deprecation (5107, node10) is emitted under 6.0.2.
  • nodeTypesRoot() is not reachable in a normal install, including --prod. When it does fail it throws at module load with a bare MODULE_NOT_FOUND naming nodeTypesRoot and no mention of contracts — a wrapped message would be cheap.
  • The 44 committed snapshots, authored under ts-morph 18, pass unchanged under 28.

Note

ts-lint now type-checks under vendored TS 6.0.2 while its ESLint pass parses under typescript 5.9.3 — the vendored compiler is now newer than the lint parser, reversing the previous skew. Syntactic-only config, so impact is small, but the direction changed.

Recommended action

  1. Restore the exact pin, or justify the caret — and note npm-shrinkwrap.json is promised but never built.
  2. Fix the ambient overclaim at :67.
  3. Decide on findings 2 and 3 together: consumer-resolved type roots and leaked globals share one fix.

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