Uh oh!
There was an error while loading. Please reload this page.
Share one RpcPromise alias between Result and the public export - #237
Conversation
Result's Stubable and RpcCompatible arms and the public RpcPromise built structurally-identical intersections from different aliases, forcing the checker through a member-by-member structural comparison of the mutually recursive Stubify/Provider cycles. Defining RpcPromise once in types.d.ts (conditional on Stubable, so each arm resolves to exactly the type it had before) lets same-alias-same-args comparisons short-circuit on identity. tsc 5.9: type-tests stay 0-error; instantiations drop 175,019 -> 151,466. tsgo 7.0.2: all 11 depth errors in __type-tests__ and all 12 (+6 cascades) in __tests__ at index.test.ts:808/:967/:1334 and workerd.test.ts:183 disappear; no test assertion changed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…essary The TODO at this site documented a TS2589 on TypeScript 5.9 when relying on the default type argument. With Result and RpcPromise sharing one alias the relation no longer recurses past the depth budget, so the explicit <undefined> can go -- and staying removed is a regression canary for the restructuring helping Strada, not just tsgo. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
All contributors have signed the CLA ✍️ ✅ |
commit: |
Maximo-Guk
commented
Aug 12, 2026
I have read the CLA Document and I hereby sign the CLA |
teamchong
commented
Aug 12, 2026
/bonk review this |
There was a problem hiding this comment.
Review: Share one RpcPromise alias
Nice, well-motivated change. I checked out the branch and verified the claims:
npm run test:types(TS 5.9) passes cleanly.tsc --extendedDiagnosticsreports Instantiations: 151466, exactly matching the PR description (175,019 → 151,466).- Removing the
RpcSession<undefined>workaround inindex.test.tsis safe:RpcSession<T extends RpcCompatible<T> = undefined>(src/index.ts:72) already defaults toundefined, and the type tests confirm it now compiles without the explicit arg.
On the semantics of the public alias
The old public type was Stub<T> & Promise<Stubify<T>>, whereas the new alias mirrors Result's arms (Promise<...> & Provider<T> & StubBase<T>). These are not structurally identical (e.g. for a primitive T, the new form adds Provider<T>), so this is a real change to the exported type, not a pure refactor. That said, it's a defensible — arguably better — one: RpcPromise is documented as "the result of an RPC call" (src/index.ts:45), so having the public alias resolve to the same thing Result<T> produces is more accurate, and it's what actually flows out of pipelined calls. The identity short-circuit that fixes the tsgo depth errors is a nice bonus. Worth a sentence in the PR body calling out that the exported type shape shifts slightly, for anyone relying on it structurally.
One cleanup
The Stubify import in index.ts is now unused — it was only referenced by the old RpcPromise definition. Neither tsconfig enables noUnusedLocals, so it slips through, but it's dead code now.
Overall this looks correct and is a solid improvement. Just the unused-import nit below.
Uh oh!
There was an error while loading. Please reload this page.
Review posted successfully. Here's a summary of what I did and found: Review Summary for PR #237I checked out the branch, installed deps, and independently verified the PR's claims: Verified ✅
Observations raised in the review
Overall assessment: correct and a solid improvement, with one minor unused-import nit. |
Co-authored-by: teamchong <teamchong@users.noreply.github.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ef5c64b to
cce0eb9Comparecce0eb9 to
27b5d05CompareCo-Authored-By: Claude Fable 5 <noreply@anthropic.com>
27b5d05 to
778dd8eCompareUh oh!
There was an error while loading. Please reload this page.
The typescript catalog entry moves to 7.0.2; capnweb 0.11's shared RpcPromise alias (cloudflare/capnweb#237) removed the depth errors that blocked this. Consumers of the JS compiler API keep a JS-based compiler: capnweb-validate's transform gets a pinned typescript 6.0.3 dependency via .pnpmfile.cjs (its typescript peer would otherwise resolve to 7.x, which ships no compiler API), and the configurator build script plus the mcp-shared schema test import the typescript6 alias. typed-storage gains the rootDir TS7 emit requires, workshop-backend drops an Env.BROWSER optional override TS7 rejects (use sites read BrowserRun | undefined), and three map/filter callbacks get explicit parameter types. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The typescript catalog entry moves to 7.0.2; capnweb 0.11's shared RpcPromise alias (cloudflare/capnweb#237) removed the depth errors that blocked this. Consumers of the JS compiler API keep a JS-based compiler: capnweb-validate's transform gets a pinned typescript 6.0.3 dependency via .pnpmfile.cjs (its typescript peer would otherwise resolve to 7.x, which ships no compiler API), and the configurator build script plus the mcp-shared schema test import the typescript6 alias. typed-storage gains the rootDir TS7 emit requires, workshop-backend drops an Env.BROWSER optional override TS7 rejects (use sites read BrowserRun | undefined), and three map/filter callbacks get explicit parameter types. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The typescript catalog entry moves to 7.0.2; capnweb 0.11's shared RpcPromise alias (cloudflare/capnweb#237) removed the depth errors that blocked this. Consumers of the JS compiler API keep a JS-based compiler: capnweb-validate's transform gets a pinned typescript 6.0.3 dependency via .pnpmfile.cjs (its typescript peer would otherwise resolve to 7.x, which ships no compiler API), and the configurator build script plus the mcp-shared schema test import the typescript6 alias. typed-storage gains the rootDir TS7 emit requires, workshop-backend drops an Env.BROWSER optional override TS7 rejects (use sites read BrowserRun | undefined), and three map/filter callbacks get explicit parameter types. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Defines
RpcPromiseonce in types.d.ts soResult's arms and the public export resolve to the same alias instantiation. Fixes all TypeScript 7 (tsgo) depth errors (TS2321/TS2589) with no test assertion changes, and drops 5.9 type instantiations 175,019 → 151,466 — which also makes theRpcSession<undefined>workaround in index.test.ts unnecessary.