feat: construct RpcPromise from a Promise - #242
Merged
Conversation
🦋 Changeset detectedLatest commit: bf3c7be The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
commit: |
This comment was marked as outdated.
This comment was marked as outdated.
ndisidore
force-pushed
the
feat/from-promise
branch
3 times, most recently
from
August 12, 2026 21:23
0535521 to
1ecfa82
Compare
ndisidore
marked this pull request as ready for review
August 12, 2026 21:24
ndisidore
force-pushed
the
feat/from-promise
branch
from
August 12, 2026 21:35
1ecfa82 to
1065dbf
Compare
kentonv
reviewed
Aug 12, 2026
Collaborator
Author
|
@ask-bonk review this |
There was a problem hiding this comment.
Review — LGTM
Re-reviewed at 33c0122, which addresses all four of @kentonv's review comments. Verified locally: build succeeds, test:types passes, and all 178 tests in index.test.ts pass on node, chromium, and workerd.
Review feedback resolution
- Type overloads (
src/index.ts) — the two overloads are collapsed into a singlenew <T extends RpcCompatible<T>>(value: Promise<T | Stub<T>>): RpcPromise<T>, andPromiseLikewas dropped (non-Promisethenables no longer accepted, as agreed). Inference works without an explicit<T>— the type tests confirmnew RpcPromise(Promise.resolve(pointStub))infersRpcPromise<PointTarget>(__type-tests__/rpc-base-cases.test.ts:160-162). hookForResolutionone-liner — inlined.hookForPromiseArg— inlined into the constructor.- The bare-stub bug — the real catch. The branch is now gated on
typeForRpc(hook) === "rpc-promise"only (src/core.ts:555), so a bareRpcStubno longer has its non-promise hook adopted directly (which couldn't answerpull()); it falls through to the payload path instead. Covered by the regression test "resolves when awaited after construction from a bare local stub" (__tests__/index.test.ts).
Correctness
- Rejection is adopted as an
ErrorStubHookand the backingPromiseStubHookpromise never rejects; combined withignoreUnhandledRejections(), discarded pipelined calls, awaits, andmap()results can't leak unhandled rejections. All three "does not report an unhandled rejection…" tests pass. - Disposal is correctly ordered behind queued calls via
PromiseStubHook.dispose()— the "delivers a call initiated before disposal" and "disposes the eventual target when disposed before resolution" tests exercise exactly the ordering paths that rely on the #241 fixes, which justifies the stacking. - Adopting an existing
RpcPromisedirectly preserves laziness (only one["pull"]is sent) and brokenness — verified by the "keeps an adopted RpcPromise lazy" and "preserves brokenness" tests. - No wire-protocol change (client-side construction reusing existing representations), so no
protocol.mdupdate needed. Changeset present (minor). README documents the ownership-transfer and unbounded-queue caveats.
No logic bugs, capability leaks, or backward-compat concerns found. Ship it.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
kentonv
reviewed
Aug 13, 2026
ndisidore
force-pushed
the
feat/from-promise
branch
2 times, most recently
from
August 17, 2026 21:02
11d58ca to
8c51593
Compare
ndisidore
commented
Aug 17, 2026
kentonv
reviewed
Aug 18, 2026
kentonv
reviewed
Aug 18, 2026
kentonv
reviewed
Aug 18, 2026
kentonv
reviewed
Aug 18, 2026
ndisidore
added a commit
that referenced
this pull request
Aug 18, 2026
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.
kentonv
approved these changes
Aug 20, 2026
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.
…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.
…orStubHook Per review: PromiseStubHook already handles a rejected backing promise -- it disposes the arguments of queued calls (since #241) and surfaces the error through pull() and onBroken() -- so the constructor no longer maps rejection to an ErrorStubHook resolution. Observable change: a pipelined call whose result is neither awaited nor disposed now fires an unhandled rejection event, matching the existing behavior of local async calls. The unhandled-rejection tests now dispose the discarded results, which both silences the event and models correct usage.
ndisidore
force-pushed
the
feat/from-promise
branch
from
August 20, 2026 13:19
c7f6e09 to
bf3c7be
Compare
Merged
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
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.
You can now write
new RpcPromise(promise)(resolves the TODO). Calls made before the promise settles are queued and delivered in order once it does, and awaiting it yields the resolution. The promise can resolve to anRpcTarget, a stub, or a plain value.@kentonv mentioned this re: reconnection in cloudflare-os#172
Passing an existing
RpcPromiseadopts its hook directly rather than awaiting it. Two caveats are documented in the README: ownership of the resolution transfers to theRpcPromise(resolve with adup()if you also keep the stub), and pending calls queue unboundedly (reject the promise when giving up).Stacked because the new tests exercise rejection and dispose-before-resolution paths that only behave correctly with the
PromiseStubHookfixes in #241. Landing that first keeps this diff feature-only.