Skip to content

fix: migrate four client packages to TypeScript 7 (Clients workflow) - #1583

Merged
rbuergi merged 2 commits into
mainfrom
fix/clients-ts7-migration
Aug 14, 2026
Merged

rbuergi merged 2 commits into
mainfrom
fix/clients-ts7-migration

Conversation

@rbuergi

@rbuergi rbuergi commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

The npm-major bump in 688ce589b (PR #1555) moved TypeScript 5.9 → 7 across the client packages and left six Clients jobs red. #1567 already fixed Portal-next. This fixes four of the remaining five. The two react-native jobs need a scope call — see the bottom.

Verified locally by running each job's own commands (npm ci from the committed lock, then typecheck / test / build).

The TS 7 change behind three of them

TypeScript 7 no longer auto-includes every package under node_modules/@types the way 5.x did. Reproduced in a pristine project — same tsconfig, same @types/node@26.2.0, only the compiler version varying:

typescript import { readFileSync } from "node:fs" + process.cwd()
5.9.3 clean
7.0.2 TS2591 on node:fs, node:path, process
job real error migration
@meshweaver/react 11 × TS2591 — the tests that read the server catalogs off disk (i18n/localize, render/parity, ciTrigger) lost node:fs / node:path / process "types": ["node"]
@meshweaver/client-web same TS2591 in the wire/rest contract guards, plus TS7006 implicit-any that follows from an untyped readFileSync "types": ["node"]
Portal example TS5102: Option 'baseUrl' has been removed dropped baseUrl; the paths entries are already relative to the tsconfig, which is exactly what baseUrl "." resolved them against — the same one-liner portal-next took in #1567

"types": ["node"] is the fix the compiler's own diagnostic prescribes. Nothing is silenced — the files stay fully checked, and naming one package is stricter than 5.x's include-everything.

@meshweaver/client — a real API migration

TS 7 removed ts.transpileModule. The package's "." export is now lib/version.cjs (a version string); the API moved to typescript/unstable/*, which is Project/Program/Checker shaped with no single-string transform anywhere in it (grep -r transpile over the package: zero hits). So this is not a config problem and there is no in-package replacement.

The node kernel's type-stripper now uses amaro — the SWC-based stripper Node itself bundles for --experimental-strip-types.

mode: "transform", not "strip", is deliberate: strip-only rejects the TS constructs that carry runtime semantics, which transpileModule used to compile. Node's own module.stripTypeScriptTypes is strip-only, which is why it is not enough. Verified through the real executeCode:

snippet result
const answer: number = 6*7 42
enum Color { Red, Green } Color.Green 1
namespace N { export const v = 42 } N.v 42
class C { constructor(private readonly x: number){} … } 42

typescript drops to a devDependency (it is only tsc now), so the worker no longer ships the whole compiler. amaro declares engines: node>=22, hence the engines field and that job moving to node 22 — Node 20 went end-of-life 2026-04-30.

That job also gains npm test: it previously ran typecheck only, so the js/ts execution core this PR changes had no CI coverage at all.

Nothing was suppressed

No ignoreBuildErrors, no new skipLibCheck, no @ts-nocheck / @ts-ignore / @ts-expect-error, no --legacy-peer-deps / --force, no narrowed include, no deleted or skipped test. Net change to how much is checked: more (one added test step).

Local verification

job result
@meshweaver/react typecheck clean · 261 tests in 35 files · lib build clean
@meshweaver/client-web typecheck clean · 52 tests in 6 files
@meshweaver/client clean npm ci → typecheck clean · 5/5 tests
Portal example npm ci → tsc --noEmit clean · vite build clean

The i18n catalog drift guard (15 tests) and the ciTrigger workflow guard (4 tests) both pass — the latter re-run after the clients.yml edit.

NOT fixed here — RN connector / RN web need a scope call

Dependabot bumped expo 52→57 and react 18→19 but pinned react-native at 0.76.5, whose peers are react: ^18.2.0 / @types/react: ^18.2.6. That set is genuinely unresolvable — the npm ci ERESOLVE is the drift guard working correctly, not a packaging nit.

Three candidate fixes, all rejected on evidence:

  • --legacy-peer-deps / an overrides entry — installs a combination upstream does not support (RN 0.76.5 predates React 19), and the RN web job actually renders through Playwright. Cannot be proven, so it cannot be forced.
  • Pin @types/react back to 18 for RN — tested: RN typechecks ../react/src, which resolves React 19 types from clients/react/node_modules, so two React type identities collide → TS2786 at every JSX call site.
  • Revert clients/react to React 18 as well — would re-break Portal-next, which is green on React 19.

The coherent direction is forward: react-native@0.82 peers are exactly react: ^19.1.1 / @types/react: ^19.1.1 — i.e. the Expo SDK 52→57 upgrade, six RN minors, which also has to deal with expo-av. The repo already flagged this in clients/react-native/package.json: "the real fix is the Expo SDK upgrade … which is its own migration." Tracked separately rather than half-done here.

These two jobs do not gate image publication: main-cd.yml triggers on MeshWeaver Build and Test, and its image legs build grpc-web + portal-next only — never react-native. (The #1568 stranding was the portal-next leg, fixed in #1567.)

No What's New entry: CI/build plumbing, and no workflow publishes these npm packages.

🤖 Generated with Claude Code

The npm-major bump in 688ce58 (PR #1555) moved TypeScript 5.9 -> 7 across
the client packages and left six Clients jobs red. #1567 already fixed
Portal-next. This fixes four of the remaining five; the two react-native
jobs are a separate migration (see below).

TS 7 no longer auto-includes every package under node_modules/@types the way
5.x did, so @types/node has to be named explicitly:

  - @meshweaver/react     11x TS2591 - the tests that read the server catalogs
                          off disk (i18n/localize, render/parity, ciTrigger)
                          lost node:fs, node:path and process.
  - @meshweaver/client-web same TS2591 in the wire/rest contract guards, plus
                          the TS7006 implicit-any that follows from an
                          untyped readFileSync.

Both get "types": ["node"] - the fix the compiler's own diagnostic prescribes.
Nothing is silenced: the files stay fully checked, and naming one package is
stricter than 5.x's include-everything.

  - Portal example        TS5102: baseUrl was removed in TS 7. Dropped it; the
                          "paths" entries are already relative to the tsconfig,
                          which is exactly what baseUrl "." resolved them
                          against (same one-liner portal-next took in #1567).

  - @meshweaver/client    a real API migration, not a config fix. TS 7 removed
                          ts.transpileModule: the package's "." export is now
                          lib/version.cjs (a version string) and the API moved
                          to typescript/unstable/*, which is Project/Program/
                          Checker shaped with no single-string transform
                          anywhere in it. The node kernel's type-stripper now
                          uses amaro - the SWC-based stripper Node itself
                          bundles for --experimental-strip-types.

                          mode: "transform", not "strip": strip-only REJECTS
                          the TS constructs that carry runtime semantics
                          (enum, namespace, parameter properties) that
                          transpileModule used to compile, and Node's built-in
                          module.stripTypeScriptTypes is strip-only. Verified
                          through executeCode that all of them still run.

                          typescript drops to a devDependency (it is only tsc
                          now), so the worker no longer ships the compiler.
                          amaro declares engines node>=22, hence the engines
                          field and that job moving to node 22 - Node 20 went
                          end-of-life 2026-04-30.

That job also gains `npm test`: it only ran typecheck, so the js/ts execution
core this commit changes had no CI coverage at all. Strictly more checking.

Verified locally with each job's own commands (npm ci from the committed lock,
then typecheck/test/build): react 261 tests + lib build, client-web 52 tests,
client 5 tests from a clean npm ci, portal tsc + vite build.

NOT fixed here - RN connector and RN web need a scope call. Dependabot bumped
expo 52->57 and react 18->19 but pinned react-native at 0.76.5, whose peers are
react ^18.2.0 / @types/react ^18.2.6. That set is genuinely unresolvable, which
is what the npm ci ERESOLVE reports; forcing it with --legacy-peer-deps or an
overrides entry would install a combination upstream does not support. Pinning
@types/react back to 18 does not work either: RN typechecks ../react/src, which
resolves React 19 types from clients/react/node_modules, so the two React type
identities collide (TS2786 across every JSX call site). The coherent direction
is forward - react-native 0.82 peers are exactly react ^19.1.1 - i.e. the
Expo SDK 52->57 upgrade, which also has to deal with expo-av. Tracked separately.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes multiple Clients workflow failures after the TypeScript 7 upgrade by updating client tsconfigs for explicit Node typings, adjusting a portal tsconfig for removed options, and migrating the Node worker’s TypeScript transform off the removed ts.transpileModule.

Changes:

  • Migrate the Node worker TypeScript-to-JS transform from typescript.transpileModule to amaro, and update CI to use Node 22 plus add worker tests.
  • Add explicit "types": ["node"] to affected client tsconfigs to restore Node globals/modules under TS 7.
  • Remove the baseUrl option from the portal example tsconfig to align with TS 7 behavior.

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
clients/typescript/src/worker.ts Replaces TypeScript runtime transpilation with amaro transform-based compilation for Code node execution.
clients/typescript/package.json Adds Node >=22 engines, moves typescript to devDependency, and adds amaro runtime dependency.
clients/typescript/package-lock.json Updates lockfile to reflect amaro dependency and typescript as dev-only, plus engines metadata.
clients/react/tsconfig.json Explicitly includes Node types to fix missing node:*/process typing under TS 7.
clients/grpc-web/tsconfig.json Explicitly includes Node types for contract tests reading files under TS 7.
clients/portal/tsconfig.json Drops baseUrl while keeping path aliases working under TS 7.
.github/workflows/clients.yml Bumps the Node worker job to Node 22 and adds a test step to cover the execution core.
Files not reviewed (1)
  • clients/typescript/package-lock.json: Generated file

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread .github/workflows/clients.yml
…O tests on node 20

The react job's `npm test` had NEVER RUN on main: the TS 7 typecheck failure in
the step before it short-circuited the job, so this second breakage from the
same dependabot commit (688ce58, jsdom 29 -> 30) sat latent behind it. Fixing
the typecheck un-masked it.

jsdom@30.0.1 declares engines ^22.22.2 || ^24.15.0 || >=26.0.0; jsdom 29 still
allowed node 20. On node 20 its undici dependency throws

  TypeError: webidl.util.markAsUncloneable is not a function
    at new CacheStorage (node_modules/undici/lib/web/cache/cachestorage.js:20)
    at node_modules/jsdom/lib/api.js:12

because markAsUncloneable landed in node 22. Every jsdom worker then fails to
start, so vitest collects nothing:

  Test Files  no tests
  Tests       no tests      -> exit 1

That is the dangerous shape: "no tests" is a failure that reads like an absence
of work rather than a break, which is exactly why this must be the node version
and not a swallowed error. portal-next has been on node 22 all along, which is
why it is green on the same jsdom.

Verified by running the job's own commands on both runtimes against the same
committed lock:

  node 20.20.2 -> npm ci ok, typecheck ok, 0 tests collected, exit 1
  node 22.23.2 -> npm ci ok, typecheck ok, 35 files / 261 tests PASSED, build ok

22.23.2 satisfies jsdom's ^22.22.2. Node 20 also went end-of-life 2026-04-30.
grpc-web and portal stay on 20 deliberately: neither uses jsdom (portal has no
test script at all), so neither is sitting on this trap.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@rbuergi

rbuergi commented Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

Arrived at the same four fixes independently in #1585 before spotting this — closing mine in favour of this + #1587. Two findings from that pass that are not in either PR:

1. 🚨 The worker.ts TS 7 migration will ship UNTESTED

clients/typescript depends on typescript at runtime — the Node kernel worker transpiles user TS snippets — so migrating worker.ts off ts.transpileModule changes product behaviour, not just a typecheck.

And the node-sdk job runs npm run typecheck only. The package defines test and ships test/worker.test.mjs, whose second case is literally "typescript node: types are stripped, then executed" — the one thing that would prove the migrated transpile still works — and CI has never run it. So this PR can go green with TypeScript Code-node execution broken, exactly as #1555 did.

#1588 adds - run: npm test to that job (for #1496, which is the same "a test nobody runs is documentation" defect). Worth taking the same line here, or stacking so the two do not both edit that job.

I took the other route in #1585 — pinning typescript back to ^5.9.3 for that package only, on the grounds that TS 7's "." export is now ./lib/version.cjs and the compiler moved behind typescript/unstable/*, which is explicitly unstable for a runtime dependency. Your route is better if it holds, since it keeps the bump — it just needs the test running to know that it does. With npm test wired in, worker.test.mjs answers it in one run.

2. The @meshweaver/react job can still fail at TEST time on the runner

Not a typecheck failure — the vitest forks pool fails to start its workers:

Error: [vitest-pool]: Failed to start forks worker for test files .../threadChat.test.tsx
Caused by: TypeError: webidl.util.markAsUncloneable is not a function

35 unhandled errors, job dead in 34 s. That is undici's webidl against the runner's Node 20.

It hit my branch at 15:26:16 while yours passed at 15:25:05 on a near-identical tree, so it is plausibly runner flakiness rather than something in either diff — but it is the kind that reads as "the PR broke the tests". If it recurs, node-version: 20 → 22 in clients.yml is the likely fix, and it is worth knowing about before it is attributed to a diff.

@github-actions

Copy link
Copy Markdown
Contributor

Test Results (shard 0)

728 tests  ±0   728 ✅ ±0   10m 52s ⏱️ -30s
 10 suites ±0     0 💤 ±0 
 10 files   ±0     0 ❌ ±0 

Results for commit 986a213. ± Comparison against base commit 9018929.

@github-actions

Copy link
Copy Markdown
Contributor

Test Results (shard 4)

1 925 tests  ±0   1 915 ✅ ±0   7m 12s ⏱️ +9s
   11 suites ±0      10 💤 ±0 
   11 files   ±0       0 ❌ ±0 

Results for commit 986a213. ± Comparison against base commit 9018929.

@github-actions

Copy link
Copy Markdown
Contributor

Test Results (shard 3)

   11 files  ±0     11 suites  ±0   5m 56s ⏱️ -8s
1 837 tests ±0  1 734 ✅ ±0  103 💤 ±0  0 ❌ ±0 
2 182 runs  ±0  2 079 ✅ ±0  103 💤 ±0  0 ❌ ±0 

Results for commit 986a213. ± Comparison against base commit 9018929.

@github-actions

Copy link
Copy Markdown
Contributor

Test Results (shard 5)

1 925 tests  ±0   1 733 ✅ ±0   6m 13s ⏱️ +16s
   11 suites ±0     192 💤 ±0 
   11 files   ±0       0 ❌ ±0 

Results for commit 986a213. ± Comparison against base commit 9018929.

@github-actions

Copy link
Copy Markdown
Contributor

Test Results (shard 1)

1 464 tests  ±0   1 464 ✅ ±0   7m 8s ⏱️ +32s
   11 suites ±0       0 💤 ±0 
   11 files   ±0       0 ❌ ±0 

Results for commit 986a213. ± Comparison against base commit 9018929.

@github-actions

Copy link
Copy Markdown
Contributor

Test Results (shard 2)

2 554 tests  ±0   2 553 ✅ ±0   8m 45s ⏱️ -19s
   12 suites ±0       1 💤 ±0 
   12 files   ±0       0 ❌ ±0 

Results for commit 986a213. ± Comparison against base commit 9018929.

@github-actions

Copy link
Copy Markdown
Contributor

Test Results

    66 files  ±0      66 suites  ±0   46m 7s ⏱️ -2s
10 433 tests ±0  10 127 ✅ ±0  306 💤 ±0  0 ❌ ±0 
10 778 runs  ±0  10 472 ✅ ±0  306 💤 ±0  0 ❌ ±0 

Results for commit 986a213. ± Comparison against base commit 9018929.

@rbuergi

rbuergi commented Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

Both points are correct, and I'm taking one and deliberately not taking the other. Recording why rather than resolving silently.

The duplicated compile is real. "test": "tsc && node --test test/*.test.mjs" and "typecheck": "tsc --noEmit" — so with both steps the package compiles twice. They are not identical (one emits, because the tests run against the emitted output; one doesn't), but tsc type-checks either way, so the typecheck step adds no coverage. It costs a few seconds and buys a cleaner failure attribution — a red typecheck step vs a red test step that happens to be a compile error. I'm leaving it, and I'd rather state that as a judgement call than pretend it isn't redundant.

The stale job name is a genuine defect and it gets fixed — a job labelled (typecheck) that also runs tests misreports what it did, which is the precise failure mode this whole change set exists to remove. I'm not fixing it here, because #1588 already renames it to (typecheck + test) and that PR is open now. Renaming it in this PR would burn a full CI cycle on a label and deepen the clients.yml conflict between the two branches instead of resolving it — see the overlap note I left on #1588.

So the sequence is: this merges as-is, #1588 lands the rename it already carries. If #1588 stalls for any reason, the rename comes back here as a one-liner.

Worth noting the two PRs independently added npm test to this same job within hours of each other, having each found that the package shipped test/worker.test.mjs and never ran it. That is the argument for #1568.

@rbuergi
rbuergi merged commit 850a782 into main Aug 14, 2026
27 of 29 checks passed
@rbuergi

rbuergi commented Aug 14, 2026

Copy link
Copy Markdown
Contributor Author

Runtime proof for the ts.transpileModule migration (the concern raised in review — the node-sdk CI job only typechecks): ran clients/typescript npm test locally on this branch — 5/5 pass, including "typescript node: types are stripped, then executed", "a throwing snippet is captured as an error", and the JSON round-trip. #1588 adds npm test to the CI job so this stays enforced. The two red RN jobs on this branch's Clients run are #1587's domain (mirror image: #1587's red jobs are the TS7 six fixed here); merging both restores the workflow.

rbuergi added a commit that referenced this pull request Aug 14, 2026
…kflow can go green

The new 'Clients workflow verdict' gate resolves the Clients conclusion for the
head commit. On this branch that conclusion was still 'failure' because the four
packages #1583 migrates were red here - this branch predates it. Merging main
brings the RN fix and the TS 7 migration together, which is the first tree where
all eight Clients jobs can pass.
@rbuergi
rbuergi deleted the fix/clients-ts7-migration branch August 19, 2026 10:30
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.

2 participants