fix: elide stubs in RPC result types so Promise<RpcStub<T>> returns match Promise<T> - #251
Draft
ndisidore wants to merge 10 commits into
Draft
fix: elide stubs in RPC result types so Promise<RpcStub<T>> returns match Promise<T>#251ndisidore wants to merge 10 commits into
ndisidore wants to merge 10 commits into
Conversation
Resolves the long-standing TODO on the RpcPromise constructor: the application may now pass a Promise (or any other thenable) for the eventual resolution. Calls made before the promise settles are queued and delivered in order once it does, so an RpcPromise can stand in for a capability that doesn't exist yet -- for example, one that will only become available after a broken session has been re-established. The promise may resolve to an RpcTarget, a stub, or a plain value. Promise.resolve() performs thenable assimilation natively, so no hand-rolled hardening against misbehaving thenables is needed. The resolution is adopted with return semantics (the same representation used for resolutions of local async calls), so awaiting delivers the value, pipelined calls forward through it without forcing a pull, and brokenness of a stub resolution is preserved. Passing an existing RpcPromise adopts its hook directly, keeping it lazy. A rejection is adopted as an ErrorStubHook rather than left to reject the backing promise, so the promise chains behind queued calls never reject: calls land on the ErrorStubHook (which disposes their arguments) and the error surfaces only through pull() or onBroken(). Without this, a discarded pipelined call on a promise-backed stub would raise an unhandled rejection event when the promise rejects (crashing Node under its default handling), even though fire-and-forget calls on the session-backed stub it stands in for reject only on pull.
- Only adopt the hook of an existing RpcPromise; a bare stub's hook may not implement pull(), so bare stubs now take the generic path, whose resolution payload handles them correctly (await previously rejected with "Tried to resolve a non-promise stub."). Regression test added. - Inline hookForPromiseArg and hookForResolution into the constructor. - Collapse the constructor's type overloads into a single signature, narrowing the accepted type to Promise (runtime still assimilates arbitrary thenables). - Reframe the README section around the local-loopback RPC equivalence, and align the jsdoc and changeset with it.
- Adopting an existing RpcPromise now consumes the source: its hook is neutered to DISPOSED_HOOK, so disposing the source can no longer silently kill the wrapper. Using the source after wrapping reports the standard disposed error. - Restore the invariant that every RpcPromise has a defined path by defaulting pathIfPromise to [] on the internal StubHook path. - Wrap workerd-native RpcPromise/RpcProperty values (rpc-thenable) in a TargetStubHook so pipelined calls aren't eagerly assimilated. - Document ownership transfer on adoption and the dup() workaround for keeping a deferred capability lazy when resolving a native Promise with an RpcPromise.
Per review feedback on #242: the ownership-transfer note (nobody wraps an RpcPromise they already hold on purpose) and the thenable-assimilation note (not specific to this constructor) don't belong in the public docs. The behaviors themselves are unchanged and remain pinned by tests.
🦋 Changeset detectedLatest commit: bb7c807 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This comment was marked as low quality.
This comment was marked as low quality.
This comment was marked as low quality.
This comment was marked as low quality.
commit: |
ndisidore
force-pushed
the
feat/result-stub-elision
branch
2 times, most recently
from
August 19, 2026 11:23
6362382 to
05c32ea
Compare
…pped native stubs - get([]) on a thenable-backed TargetStubHook now returns dup() instead of throwing, fixing dup() and argument-passing of wrapped native promises. - onBroken() now subscribes to a thenable target's rejection, so onRpcBroken fires when a wrapped native promise rejects instead of silently no-oping. - Property promises share the source hook and path so the get() happens lazily on first use, avoiding eager wire pushes / getter side effects.
…atch Promise<T> A method declared to return Promise<RpcStub<T>> (or a property typed RpcStub<T>) previously produced RpcPromise<RpcStub<T>>, a broken type that could neither be awaited into an RpcStub<T> nor passed as a pipelined argument. Root cause: the string-keyed __RPC_TARGET_BRAND survives Provider<T>'s key mapping (which only excludes symbol | keyof StubBase), so Stub<T> of a branded or callable T structurally matches Stubable, shadowing the intended StubBase handling in both Stubify and Result. Result (and the capnweb-validate mirror, StubResult) now elides the stub wrapper for Stubable payloads, so declared-stub returns produce the exact same RpcPromise<T> as returning the payload directly — and the same type the RpcPromise constructor produces for a promised stub. Plain-interface stubs (RpcStub<PlainApi>) are deliberately NOT elided: RpcPromise<U> only awaits back to Stub<U> when U extends Stubable, so eliding those would change the awaited value from a stub to a stubified record, breaking today's working shape. Stubify's arms are reordered to [Promise, StubBase, Stubable] so existing stubs pass through instead of double-wrapping, and promise-backed stubs resolve through the Promise arm. The eliding arm intentionally omits an RpcCompatible<R> re-check: evaluating RpcCompatible<R> there recurses through its Stub<Stubable> member back into Result, tripping TS2615 circularity errors in mapped types. Anything matching our StubBase already had the constraint enforced where the stub type was formed. Documented but not fixed here: - A hand-written RpcPromise<RpcStub<T>> annotation still takes the RpcPromise alias's own Stubable arm (awaits to Stub<Stub<T>>); the library no longer produces that type, and fixing it inside the alias risks the Result/RpcPromise identity short-circuit. - Native workers-types stubs (Rpc.Stub) are unaffected: their StubBase lacks onRpcBroken, so they never match our StubBase<infer U>. This consistency gap is unchanged from before. - The RpcPromise constructor's Promise<T | Stub<T>> union also elides plain-interface stubs (new RpcPromise(Promise.resolve(plainIfaceStub)) infers RpcPromise<PlainApi>), which method returns deliberately do not. Pre-existing on the parent branch; worth raising in review. The eliding arms guard `any` payloads explicitly (IsAny): `[any] extends [Stubable]` is true, so without the guards RpcStub<any> results collapsed to UnknownResult (awaiting to unknown), and capnweb-validate's StubResult<any> collapsed to Promise<unknown> & StubBase<unknown> even for plain Promise<any> returns. Both now keep their stub surface.
…ruction with result elision - Exclude __RPC_TARGET_BRAND (and workers brands in capnweb-validate) from Provider/ValidatedStub key mapping so the string-keyed brand no longer leaks onto stub surfaces. - Extract the Result stub elision into ElideStub and apply it to the RpcPromise constructor signature, so constructing from a promised stub produces exactly the type a method returning that stub would. - Make validate's StubResult distributive so unions like ValidatedStub<T> | null elide and never stays never. - Bump the changeset to minor: explicit RpcPromise<RpcStub<T>> annotations on elided results should now be written RpcPromise<T>.
ndisidore
force-pushed
the
feat/result-stub-elision
branch
from
August 19, 2026 21:14
05c32ea to
3bd5a25
Compare
All provably behavior-preserving, each verified independently against the type-test suite (strict Equal<> assertions) and the full build: - Simplify IsUnknown: the inner bracket check was a dead branch (it never excluded `any`; call sites that care check IsAny first). - Drop MethodOrProperty's IsAny guard: Awaited<any> is `any` and Result already opens with the same check. - Drop Result's IsUnknown arm: Result<unknown> falls through to the RpcCompatible arm and yields the structurally identical RpcPromise<unknown>. - Drop Result's Stubable arm: Stubable is a member of the RpcCompatible union, so stubable returns produce the same RpcPromise<R> one arm later. - Delete UnknownResult in favor of RpcPromise<unknown>, which expands to the same type and keeps Result's arms on a single alias for identity short-circuiting. Also document in capnweb-validate why MaybeCallableStub deliberately repeats StubMethodOrProperty's function arm: delegating tips ValidatedStub's recursive instantiation over TS's depth limit (TS2589). Compiler diagnostics improve slightly: types 50,773 -> 50,194, instantiations 199,986 -> 193,650.
…parameter The inline `T | NoInfer<Stub<Extract<T, Stubable>>>` union was doing three jobs at once (accept stubs, restrict to stubable payloads, stay out of inference) with no room to explain any of them. Name it and document why each piece is load-bearing. NoInfer's inference suppression survives the alias: the strict Equal<> constructor-inference tests (bare, callable, plain-interface, and union payloads) are unchanged and pass.
ndisidore
marked this pull request as ready for review
August 19, 2026 22:39
ndisidore
marked this pull request as draft
August 19, 2026 22:50
ndisidore
commented
Aug 19, 2026
| let stub = new NativeRpcStub(new RpcStub(new JsCounter())); | ||
| // Cast: now that the `__RPC_TARGET_BRAND` no longer leaks onto stub surfaces, a userspace | ||
| // stub doesn't statically match workers-types' `Stubable` (runtime interop still works). | ||
| let stub = new NativeRpcStub(<any>new RpcStub(new JsCounter())); |
Collaborator
Author
There was a problem hiding this comment.
ugh this is probably a non-starter
- capnweb stub → new NativeRpcStub(...): needs now
but its because runtime suffers from the same poised branded stub issue
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
1 participant
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.
Declaring a method as
viaStub(): Promise<RpcStub<T>>now yields the sameRpcPromise<T>as declaringPromise<T>, matching whatnew RpcPromise(promise)infers. This resolves @kentonv's| Stub<T>question on #242. The two forms had to converge on eliding because the declared-stub form is broken on main:RpcPromise<RpcStub<T>>can't be passed as a pipelined argument and its awaited value isn't assignable toRpcStub<T>. Only pipelining typechecks, which is why nobody noticed.The culprit is the string-keyed
__RPC_TARGET_BRAND. It survivesProvider<T>'s key mapping, so a stub of anRpcTarget(or function) matchesStubableand gets wrapped a second time. Fixed by reorderingStubify's arms and adding eliding arms toResult, mirrored incapnweb-validate.Plain-interface stubs (
RpcStub<PlainApi>) are deliberately not elided:RpcPromise<U>only awaits back to a stub whenUis stubable, so eliding those would break the shape that already works. There's a pinning test for it.Stacked on #242 because the equivalence test needs the constructor from that PR.