[srikrsna]

Adds serialization support for Set. Doesn't special case .map() like we do for arrays, uses the default. I think we don't need it is because Set itself doesn't have one, so it is not expected as much as it is on an Array.

This does handle lazy values that are part of a set. Again, can't think of a use case but using a Blob as an element of Set is technically valid and also using an RpcPromise that you get back as part of the Set, so this implementation handles such cases. It handles it by not moving away from LocatedPromise which expects a parent and property accessor to replace the placeholder promise with the resolved value.

We could refactor the code to avoid the temporary setter but that felt like a bigger change, so once this and Map implementation merges we can revisit that.

If we want to revisit .map over set elements, I think we should block .map support for Set

Closes #221

[changeset-bot]

🦋 Changeset detected

Latest commit: 4fbcbe0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
capnweb	Minor

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

[github-actions]

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

[srikrsna]

I have read the CLA Document and I hereby sign the CLA

[srikrsna]

/bonk review this

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/cloudflare/capnweb@229

npm i https://pkg.pr.new/cloudflare/capnweb/capnweb-validate@229

commit: 4fbcbe0

[teamchong]

/bonk review this

[ask-bonk]

Review: Add support for sending Set over RPC

Overall this is a solid, well-tested implementation. I traced the promise-substitution path on both the wire (Evaluator → RpcPayload.forEvaluate → deliverTo → deliverRpcPromiseTo → parent[property] = value) and the local (deepCopy) side, and the one-time setter approach in defineSetPromiseSlot correctly:

• substitutes each promise at its own position (verified with multiple promises),
• re-applies Set semantics so a resolution equal to an existing element collapses,
• uses reference identity via indexOf(placeholder), which is robust even when resolved values coincide,
• covers Blob-in-Set on the return path since blobs are delivered as RpcPromises.

The test suite is thorough and the strayProps assertions are a nice way to pin down that no leftover setter properties leak. build succeeds. (I couldn't run the vitest suite in CI here because it uses the browser pool and Playwright isn't installed in this environment, but I validated the setter logic with a standalone reproduction.)

Comments below are minor — mostly whitespace/style, plus one unrelated diff line and a complexity note. None are blocking.

Notes not tied to a single line

• O(n²) resolution cost: each promise resolution does [...set] + indexOf + clear() + full rebuild. For a Set with k promises this is O(k·n). Fine for the expected small-cardinality case, but worth a comment on defineSetPromiseSlot so a future reader doesn't assume it's cheap.
• Changeset file .changeset/serialize-set.md is missing a trailing newline (\ No newline at end of file).

[ask-bonk]

Review posted successfully. Here's a summary of what I reviewed and posted to PR #229.

Review summary

I traced the full Set serialization path on both the wire and local sides and verified the core mechanism is correct:

Correctness — verified sound:

• defineSetPromiseSlot installs a one-time setter that fires when the promise-delivery machinery does parent[property] = value (used by both Evaluator/RpcPayload.forEvaluate and deepCopy).
• The setter uses reference identity (indexOf(placeholder)), rebuilds the set in order, and re-applies Set dedup semantics. I confirmed multi-promise, dedup-collapse, and same-resolved-value edge cases with a standalone reproduction.
• Blob-in-Set works on the return path because blobs are delivered as RpcPromises.
• Set is correctly treated as having no remotely-addressable properties in followPath, consistent with the author's decision to skip map().
• Depth limits apply via recursive evaluateImpl(... depth + 1).
• build succeeds. (Couldn't run vitest — it uses the browser pool and Playwright isn't installed in this env.)

Feedback posted (all minor, non-blocking):

1. src/core.ts:1002 — an unrelated whitespace-only change to the existing array case; suggested reverting.
2. Trailing whitespace on new comment/code lines in core.ts and serialize.ts (with suggestions).
3. A few missing semicolons inconsistent with file style.
4. Redundant <Set<unknown>>value re-cast in disposeImpl.
5. A note about the O(n²) resolution cost worth documenting on defineSetPromiseSlot.
6. Changeset file missing a trailing newline.

The review was submitted as a single COMMENT event with grouped inline suggestions.

github run

[srikrsna]

/bonk review this

[teamchong]

/bonk review this

[ask-bonk]

Review: Add support for sending Set over RPC

I re-reviewed at the latest commit (0942d18). The style nits from the earlier round (trailing whitespace, the unrelated array-case diff) have been addressed — src/ typechecks cleanly (tsc --noEmit), and the remaining tsc errors are pre-existing and confined to __tests__/bun.test.ts / workerd.test.ts, unrelated to this change.

Correctness — traced and looks sound:

• Wire path: Devaluator emits ["set", elements]; Evaluator rebuilds with evaluateImpl(element, set, key, depth + 1) and installs a per-element setter via defineSetPromiseSlot. Depth is threaded correctly so the recursion limit still applies to set elements.
• Local path: deepCopy builds the new Set first, then fills it, passing result as the parent so promise records point at the copy (not the caller's set). The substitutes a promise in a Set passed to a local stub test confirms the caller's set is left untouched.
• Delivery: deliverTo does parent[property] = value, which fires the one-time setter. indexOf(placeholder) uses reference identity, so multiple promises and resolutions that coincide with existing elements both behave correctly (dedup re-applied on rebuild). Verified against the multi-promise and collapse tests.
• followPath treats set as having no remotely-addressable properties, consistent with skipping map().
• disposeImpl / ignoreUnhandledRejectionsImpl recurse over set elements; disposal is driven by the promises record list, so replacing promises in-place during delivery doesn't break it.
• Empty set (["set", []]) passes the value.length === 2 && value[1] instanceof Array guard.

Non-blocking notes:

1. defineSetPromiseSlot is O(k·n) for a set with k promises (each resolution does [...set] + indexOf + clear() + full rebuild). Fine for expected small cardinalities, but worth a one-line comment so a future reader doesn't assume it's cheap.
2. If a set element promise never resolves (rejection/abort before delivery), the non-enumerable setter property persists on the copied set. It's on an ephemeral copy that gets GC'd, so this is harmless — noting it for completeness.
3. A couple of missing semicolons in the setter body (delete ... / set.clear()), inconsistent with the surrounding file style. Minor.

Nice work — the test coverage (strayProps assertions in particular) is thorough and pins down the tricky substitution behavior well.

[ask-bonk]

Review submitted successfully to PR #229.

Summary

I re-reviewed the Set-over-RPC PR at the latest commit (0942d18) and posted a single COMMENT review with two inline suggestions.

Key findings:

• Prior style nits are resolved. The trailing whitespace and the unrelated array-case diff line flagged in the earlier review round are gone. src/ typechecks cleanly via tsc --noEmit; the only tsc errors are pre-existing and confined to __tests__/bun.test.ts and workerd.test.ts, unrelated to this change.

• Correctness traced end-to-end and looks sound:

  • Wire path (Devaluator → ["set", elements] → Evaluator rebuild with per-element setters) threads depth + 1 correctly.
  • Local path (deepCopy) builds the new set first and points promise records at the copy, so the caller's set is untouched (confirmed by a test).
  • Delivery's parent[property] = value fires the one-time setter; indexOf(placeholder) uses reference identity, so multi-promise and dedup-collapse cases behave correctly.
  • followPath, disposeImpl, and ignoreUnhandledRejectionsImpl all handle the new set case consistently.

• Non-blocking notes posted:

  1. defineSetPromiseSlot is O(k·n) — worth a comment (inline suggestion posted).
  2. Missing semicolons in the setter body (inline suggestion posted).
  3. Persisted setter property on a never-resolving promise, harmless since it's on an ephemeral copy.

The implementation is solid and the test coverage (especially the strayProps assertions) is thorough.

github run

[srikrsna]

/bonk review this

[teamchong]

/bonk review this

[ask-bonk]

Review: Add support for sending Set over RPC

The implementation has evolved substantially since the earlier rounds and now follows @kentonv's guidance: instead of the property-accessor/setter hack, promises (and, on the wire, Blobs) are simply banned as direct Set elements, with clear TypeErrors on both the send and receive sides. This is a much simpler and more defensible design.

Verification performed in this review:

• npm run build succeeds.
• vitest run --project node __tests__/index.test.ts -> 202 passed (browser/workerd pools can't run in this sandbox, but the Set logic is pure and fully covered by the node run).
• Traced the full path on both sides:
  • Wire send (Devaluator, src/serialize.ts:322): validates all elements before encoding any, so a rejected element can't leave a half-created blob pipe behind. Good, and the test rejects a Blob sent inside a Set pins this down.
  • Wire receive (Evaluator, src/serialize.ts:891): rebuilds the set and rejects any element that evaluates to an RpcPromise. Since streamed Blobs arrive as pipelined imports (RpcPromise), this also blocks Blob-in-Set from a hostile/buggy peer. The forged-message test (rejects a promise arriving inside a Set from a peer) exercises this guard directly.
  • Local path (RpcPayload.deepCopy, src/core.ts:1077): builds the copy first, bans promises, but intentionally allows Blobs (no wire stream involved). Consistent with the accepts a Blob in a Set passed to a local stub test.
  • followPath, disposeImpl, and ignoreUnhandledRejectionsImpl all handle the set case consistently.

The one change worth calling out for a maintainer's eyes: src/serialize.ts:1152 changes the non-promise import branch of addStub from new RpcPromise(hook, []) to new RpcStub(hook). This is needed so the Evaluator's copy instanceof RpcPromise check doesn't reject a legitimate bounced-back stub inside a Set (RpcPromise extends RpcStub). It also looks like a genuine correctness fix — on main this branch registered the hook in this.hooks (the stub list) yet returned an RpcPromise, which contradicts both the documented "imports pass immediately as stubs" intent and the parallel logic in deepCopy (src/core.ts:1123-1125). The behavioral effect is that root-level import arguments are no longer eagerly pulled/awaited before the target runs, which is the intended behavior. Worth a maintainer confirming there's no code relying on the old (arguably buggy) behavior. The bounce tests cover the Set-specific case.

Everything below is minor / non-blocking. Nice work — the test suite (especially the strayProps assertions and the forged-peer-message test) is thorough.

[ask-bonk]

fatal: could not read Username for 'https://github.com': No such device or address

github run