feat: Support ArrayBuffer and Buffer Views serialization#201
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
66c4db3 to
4231ea8
Compare
🦋 Changeset detectedLatest commit: f3ac57a 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 |
|
I have read the CLA Document and I hereby sign the CLA |
kentonv
left a comment
There was a problem hiding this comment.
Design looks good to me. I'm overloaded and don't have time to review line-by-line, maybe @dimitropoulos or @teamchong can take it.
commit: |
|
/bonk review this |
There was a problem hiding this comment.
Reviewed the ArrayBuffer / typed-array serialization support. The design is solid and I'm glad to see the earlier review feedback addressed: the misalignment validation, little-endian wire order, and the minor version bump are all in place, and the test coverage is thorough (offset views, endianness, misalignment, unknown markers).
A few things worth considering below. None are blocking, but the "Uint8Array" marker robustness gap and the swapByteOrder missing default are worth a look.
Notes / questions:
-
Incoming
"Uint8Array"marker throws. The serializer intentionally emits the 2-element legacy form forUint8Array, but a different or future implementation could legitimately send["bytes", b64, "Uint8Array"]. That currently falls through to thedefaultcase and throwsUnknown bytes type marker: Uint8Array. Since the protocol doc lists the marker set explicitly, it may be worth either (a) accepting"Uint8Array"(and"Buffer"?) as valid inbound markers, or (b) documenting in protocol.md thatUint8Arraymust always use the 2-element form. Deciding this now avoids a compatibility footgun later. -
The 12-case prototype
switchinDevaluatorduplicates the marker names already listed incore.tsand the deserializeswitch. A single prototype→name map (or reusing one shared constant) would reduce the chance of these three lists drifting apart over time. Stylistic, but this is exactly the kind of triplicated list that tends to rot.
|
Review submitted successfully. Here's a summary of what I found: Review of PR #201: ArrayBuffer and Buffer Views serializationThe PR is well-executed. The prior reviewer feedback (from ndisidore and kentonv) has all been addressed:
Non-blocking notes I posted:
No correctness bugs found in the happy path or the endianness handling; the tests are host-independent and cover the important edge cases. |
|
/bonk re-review please |
There was a problem hiding this comment.
Re-review. All the feedback from the previous rounds has been addressed cleanly:
- Misalignment validation (
byteLength % elementSize) — done, with a clear error message and test coverage. - Little-endian wire order — mandated in protocol.md and implemented via
swapByteOrder; the endianness handling is host-independent and tested. minorversion bump — correct.- Explicit type list in protocol.md — added, including the unknown-marker throw behavior and the
Uint8Arraymarkerless rule that were called out last time. swapByteOrderdefault — now throwsRangeErroron unsupported element sizes (the earlier suggestion).- Inbound explicit
"Uint8Array"marker — now accepted (and tested), closing the compatibility footgun. .slice()copy — documented as deliberate.
I ran tsc (no new errors in src/serialize.ts / src/core.ts) and a manual roundtrip of all 13 type markers, offset views, and the legacy/explicit Uint8Array forms — all correct.
Two small, non-blocking notes below. Nice work.
|
Re-review submitted. Summary of my findings: PR #201 re-review — ArrayBuffer / typed-array serializationAll prior feedback resolved. The author addressed every point from the earlier rounds (ndisidore, kenton, and the previous bonk review):
Verification I ran: Two non-blocking notes posted:
No correctness bugs found. The PR looks ready to merge modulo the maintainer's call on the two minor notes. |
Co-authored-by: kentonv <kentonv@users.noreply.github.com>
|
ughh Bonk accidentally pushed a commit. It does that sometimes and we don't know why. @ttmx can you force-push remove bonk's commit? Then I think we can merge this, no other changes needed. |
cce656c to
f3ac57a
Compare
Added serialization support for ArrayBuffer and Buffer views, like Int8Array, Uint8ClampedArray, etc.