Support Map over the RPC - #232
Conversation
commit: |
|
/bonk review this |
1 similar comment
|
/bonk review this |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 insrc/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 aMapvalue 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.
| set(resolved: unknown) { | ||
| delete (map as any)[property]; | ||
| entries[index]![slot] = resolved; | ||
| map.clear(); |
There was a problem hiding this comment.
Minor performance note (non-blocking): map.clear() + full re-insert runs on every slot resolution, so a Map with n promise slots does O(n^2) total work. Fine for normal payloads, but a large Map of pipelined promises would be quadratic. Given that a Map has no stable index to write a single slot into, this is a reasonable tradeoff — just worth a comment noting the cost so a future reader doesn't mistake it for free.
| let entries: [unknown, unknown][] = []; | ||
| for (let entry of value[1]) { | ||
| if (!(entry instanceof Array) || entry.length !== 2) { | ||
| throw new TypeError("Map entries must be serialized as key/value pairs."); |
There was a problem hiding this comment.
Nice — validating each entry is a length-2 array here is exactly the right defensive check for untrusted wire data, and the tests cover both the non-array and wrong-length cases. One small gap: when value.length !== 2 or value[1] isn't an array (e.g. ["map"] or ["map", 5]), this falls through the switch to the generic "unknown type" error rather than a Map-specific message. Not wrong, just slightly less precise diagnostically. Optional to tighten.
|
|
||
| case "map": { | ||
| let map = <Map<unknown, unknown>>value; | ||
| for (let [key, val] of map) { |
There was a problem hiding this comment.
For my own confirmation: this disposeImpl map branch only runs for source === "return" payloads (the recursive-scan path), where slot setters are never installed, so walking the final Map entries is correct. The "owned" deepCopy path disposes via the tracked this.promises list instead, so a promise key that collapsed into a duplicate is still disposed. Worth a one-line comment here mirroring the note at line 1205-1210 to make that invariant explicit for future readers — it's non-obvious that walking the collapsed Map is safe.
Co-authored-by: teamchong <teamchong@users.noreply.github.com>
|
Review submitted successfully. Here's a summary of what I reviewed and posted on PR #232: Review SummaryThe PR adds runtime serialization for Verification I did:
Assessment: looks good to merge. The design is sound:
Non-blocking notes I posted (3 inline + summary):
No correctness bugs found. |
🦋 Changeset detectedLatest commit: 3c36658 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 |
2 participants
Adds serialization support for
Map. Doesn't special case.map()like we do for arrays, uses the default. I think we don't need it is becauseMapitself doesn't have one, so it is not expected as much as it is on an Array. But I can see the use case of wanting to iterate over values of aMap.Just like #229, we support lazy values for both keys and values of a map.
If we want to revisit
.mapover map elements, I think we should block.mapsupport forMapin this.Closes #230