Repository navigation
Conversation
|
Since last review: 0 resolved, 0 still open, 0 new. Not re-run: tests, api-compat, docs, compat-flags, design-simplicity (no author changes in their files since the last review) Reviewed commit: bb41075d · github run |
guybedford
left a comment
There was a problem hiding this comment.
Verified the change end to end: the Encoding Standard's writable chunk type is AllowSharedBufferSource, and the downstream TextDecoder::decode(kj::Array<const byte>) already unwraps a SharedArrayBuffer via jsg's tryUnwrap (value.h), so the cast-through is sound and growable SABs take the same path. All five encoding variants pass locally, and reverting only encoding.ts makes encoding-ts@ fail on decoderSharedArrayBufferChunks, so the pin is a real regression guard.
Two non-blocking nits:
- In the
decode()helper inchunk-types.js, in the C++ branch thereader.read()loop throws beforeawait writesis reached, so thePromise.allinwritesis left as an unhandled rejection. The harness swallows it, but awrites.catch(() => {})(ortry { ... } finally { await writes.catch(() => {}) }) would keep it clean and make explicit that the read-side error is the one asserted. - The TS message "chunk must be a BufferSource" and the AGENTS.md "Accepts
BufferSourcechunks only (...)" are now slightly imprecise, since a SAB isn't aBufferSource. The message is pinned under ledger #4 so I wouldn't change it here;AllowSharedBufferSourcewould be the exact term in the doc.
7b0c1da to
6d064fd
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## jasnell/ts-streams-identity-write-edges #7643 +/- ##
========================================================================
Coverage 38.59% 38.59%
========================================================================
Files 868 868
Lines 268548 268548
Branches 25414 25414
========================================================================
Hits 103635 103635
Misses 150701 150701
Partials 14212 14212 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
0dc54bc to
a138a5d
Compare
The Encoding Standard's TextDecoderStream writable accepts an
AllowSharedBufferSource, and Node decodes a bare SharedArrayBuffer
chunk. TS rejected it ("TextDecoderStream: chunk must be a
BufferSource") and errored the stream, though it already decoded views
over shared memory, and TextDecoder.decode() accepts a bare one. C++
rejects it too, as not a BufferSource.
The TS transform now accepts a SharedArrayBuffer, growable ones
included, and decodes it like an ArrayBuffer; a character split across
two of them decodes whole. C++ is unchanged, so this is a new
implementation divergence (encoding ledger #8).
Pins: encoding chunk-types.js decoderSharedArrayBufferChunks (parity
for shared-memory Uint8Array, subview and DataView chunks with a split
character; ledger #8 for bare, growable and split bare chunks: TS
decodes 'CD€', C++ rejects with its invalid-chunk TypeError). Without
the source change, it fails, in the TS cells only.
Compatibility: the change is behind the experimental
typescript_implemented_streams flag. C++ is unchanged.
a138a5d to
bb41075
Compare
The Encoding Standard's TextDecoderStream writable accepts an
AllowSharedBufferSource, and Node decodes a bare SharedArrayBuffer
chunk. TS rejected it ("TextDecoderStream: chunk must be a
BufferSource") and errored the stream, though it already decoded views
over shared memory, and TextDecoder.decode() accepts a bare one. C++
rejects it too, as not a BufferSource.
The TS transform now accepts a SharedArrayBuffer, growable ones
included, and decodes it like an ArrayBuffer; a character split across
two of them decodes whole. C++ is unchanged, so this is a new
implementation divergence (encoding ledger #8).
Pins: encoding chunk-types.js decoderSharedArrayBufferChunks (parity
for shared-memory Uint8Array, subview and DataView chunks with a split
character; ledger #8 for bare, growable and split bare chunks: TS
decodes 'CD€', C++ rejects with its invalid-chunk TypeError). Without
the source change, it fails, in the TS cells only.