Support `Map` over the RPC by srikrsna · Pull Request #232 · cloudflare/capnweb

srikrsna · 2026-08-06T12:11:46Z

Adds serialization support for Map. Doesn't special case .map() like we do for arrays, uses the default.

Just like #229, we support reject lazy values in keys.

Closes #230

pkg-pr-new · 2026-08-06T12:12:28Z

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

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

commit: 25c3a7b

srikrsna · 2026-08-06T12:12:46Z

/bonk review this

teamchong · 2026-08-06T15:29:34Z

/bonk review this

ask-bonk

Reviewed the Map serialization support. The implementation is well thought out and the test coverage is genuinely impressive — the promises inside a Map suite exercises the tricky bits (key/value slots resolving in either order, key collapse under Map semantics, insertion-order preservation, Blob round-trips, and the local deepCopy path). I ran the full node suite (165 passed) and test:types (clean) locally against this branch.

The core design decision — using entries as the source of truth and rebuilding the whole Map on each promise resolution via one-time-use setters — is a clever way to reuse the existing parent[property] = resolved delivery machinery for a structure that has no addressable positions. Nicely mirrors the Set-less-ness reasoning in the comments.

A few non-blocking observations below. None of them are correctness bugs — all tests pass — but the O(n^2) rebuild is worth at least acknowledging.

Notes

O(n^2) rebuild on resolution. Each resolved promise slot calls map.clear() and re-inserts every entry, so a Map with n promise slots does O(n^2) work total. For typical payloads this is fine, but a large Map full of pipelined promises could be pathological. Given the constraint (Map has no stable index to write into), this is a reasonable tradeoff — just flagging it. A future optimization could defer the rebuild until all slots for a given batch have resolved, but that adds bookkeeping complexity that probably isn't worth it now.
Type coverage for serialization. The Map<K,V> types already exist in src/types.d.ts (Stubify/Unstubify/RpcCompatible), so this PR correctly fills the runtime gap. Worth confirming there's a round-trip/type assertion somewhere in __type-tests__ that a Map value survives — roundTripHeaders(new Map(...)) exists but that's Headers-typed, not a Map-typed round trip. Optional.

Overall this looks good to merge. Nice work on the tests.

ask-bonk · 2026-08-06T15:58:47Z

Review submitted successfully. Here's a summary of what I reviewed and posted on PR #232:

Review Summary

The PR adds runtime serialization for Map over RPC. I read the full diff, traced the delivery/disposal machinery in src/core.ts and src/serialize.ts, and ran the tests locally.

Verification I did:

Full node test suite: 165 passed
Type tests (test:types): clean
Confirmed Map<K,V> types already existed in src/types.d.ts — this PR fills the missing runtime piece.

Assessment: looks good to merge. The design is sound:

Uses entries as the source of truth and rebuilds the Map on each promise resolution via one-time-use setters, cleverly reusing the existing parent[property] = resolved delivery path for a structure with no addressable positions.
Handles key/value promise slots resolving in either order, key collapse under Map semantics, and insertion-order preservation.
Test coverage is thorough (the promises inside a Map suite is excellent).

Non-blocking notes I posted (3 inline + summary):

O(n²) rebuild — map.clear() + full re-insert on every slot resolution; fine for normal payloads but quadratic for large Maps of pipelined promises. Reasonable tradeoff given the constraints, just flagged.
Error precision in the evaluate path — malformed ["map", ...] shapes fall through to the generic "unknown type" error rather than a Map-specific one. Optional.
Disposal invariant — suggested a clarifying comment noting that the disposeImpl map branch only runs for "return" payloads (where slot setters are never installed), so walking the collapsed Map is safe.

No correctness bugs found.

github run

Co-authored-by: teamchong <teamchong@users.noreply.github.com>

changeset-bot · 2026-08-06T15:58:51Z

🦋 Changeset detected

Latest commit: 25c3a7b

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

teamchong · 2026-08-10T16:22:22Z

recheck

github-actions · 2026-08-10T16:22:33Z

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

dimitropoulos · 2026-10-07T12:02:42Z

 | ------------------------------------------- | --------- | ----------- |
-| `Map` and some other built-ins              | Not yet   | Yes         |
-| `Set`                                       | Yes       | Yes         |
+| Some other built-ins                        | Not yet   | Yes         |


AbortSignal comes to mind but we can remove the line too

dimitropoulos · 2026-10-07T12:03:31Z

 // RpcPayload

-export type LocatedPromise = {parent: object, property: string | number, promise: RpcPromise};
+export type LocatedPromise = {parent: object, property: unknown, promise: RpcPromise};


why was this necessary?

Map keys can be anything

I think this is probably the right general approach, but it also makes me nervous that we might mishandle properties somewhere. I worry about what happens e.g. if someone uses undefined as a map key, or if they somehow manage to encode a value that happens to stringify to __proto__ and then we accidentally do object[propertyName] somewhere and oops.

What if we required arbitrary-typed map keys to be boxed? Like:

type PropertyName = string | number | {mapKey: unknown}

What if we required arbitrary-typed map keys to be boxed? Like:

We will allocate for every key but apart from that it will work. I can see that this flips the accidental case to do .set instead of object[propertyName] but the check that prevents this is the same. Instead of an instance check on Map we will check for the key shape to match. I am not sure it is worth the allocations.

dimitropoulos · 2026-10-07T12:07:57Z

+        }
+
+        let result = new Map();
+        for (let [key, val] of entries) {


why do we have to loop over all the entires a second time? can't we just check the key here, as we encounter them, and then just throw immediately if we hit one that's not valid?

To avoid a potential deepCopy of a value

dimitropoulos · 2026-10-07T12:23:08Z


+      case "map": {
+        let map = <Map<unknown, unknown>>value;
+        let mapEntries = [...map];


as far as I can tell (I didn't measure it) we should prefer map.entries() here because the destructure creates eagerly allocates memory for a whole new array immediately, while .entries() lazily makes an interable zero-allocation structural pointer.

It will create the iterator twice, in this case it will just create the array once and loop over the same one twice

dimitropoulos · 2026-10-07T12:23:22Z

+        // rebuilding itself. Values may be promises or Blobs. As with `Set`, validate all keys
+        // before encoding anything, since encoded streams and Blobs (including in values) create
+        // pipes that cannot be rolled back.
+        for (let [key] of mapEntries) {


same comment as above, why do we need to loop twice?

dimitropoulos · 2026-10-07T12:25:27Z

          }
          break;
+        case "map":
+          if (value.length === 2 && value[1] instanceof Array) {


I see all the others work this way, but it's strange to me that we silently (right?) drop any error resulting from not hitting this condition. seems like we should protect against malformed stuff here, no? we can handle separately in a new PR for test coverage for this for all types if you think - I'd be happy to do it.

We don't need to surface a specific error because users are unlikely to hit this case. The only ones that will likely see this could be authors of capnweb ports to other languages.

dimitropoulos

some things

ask-bonk Bot reviewed Aug 6, 2026

View reviewed changes

Comment thread src/core.ts Outdated

Comment thread src/serialize.ts

Comment thread src/core.ts

github-actions Bot pushed a commit to srikrsna/capnweb that referenced this pull request Aug 6, 2026

Reviewed Map RPC PR cloudflare#232: LGTM

3c36658

Co-authored-by: teamchong <teamchong@users.noreply.github.com>

teamchong force-pushed the sk/map branch from 3c36658 to 4668368 Compare August 12, 2026 15:32

Support Map over the RPC

71fb6d0

srikrsna force-pushed the sk/map branch from 4668368 to 71fb6d0 Compare October 7, 2026 06:22

srikrsna requested review from dimitropoulos and teamchong October 7, 2026 06:24

dimitropoulos reviewed Oct 7, 2026

View reviewed changes

Comment thread __tests__/index.test.ts

dimitropoulos reviewed Oct 7, 2026

View reviewed changes

Comment thread src/core.ts

dimitropoulos reviewed Oct 7, 2026

View reviewed changes

srikrsna added 2 commits October 8, 2026 12:19

Remove unnecessary comments

8fea1cd

fmt

25c3a7b

Conversation

srikrsna commented Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pkg-pr-new Bot commented Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

srikrsna commented Aug 6, 2026

Uh oh!

teamchong commented Aug 6, 2026

Uh oh!

ask-bonk Bot left a comment

Choose a reason for hiding this comment

Notes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ask-bonk Bot commented Aug 6, 2026

Review Summary

Uh oh!

changeset-bot Bot commented Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

teamchong commented Aug 10, 2026

Uh oh!

github-actions Bot commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srikrsna Oct 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimitropoulos left a comment

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

srikrsna commented Aug 6, 2026 •

edited

Loading

pkg-pr-new Bot commented Aug 6, 2026 •

edited

Loading

changeset-bot Bot commented Aug 6, 2026 •

edited

Loading

github-actions Bot commented Aug 10, 2026 •

edited

Loading

srikrsna Oct 8, 2026 •

edited

Loading