Repository navigation
Conversation
|
Since last review: 0 resolved, 1 still open, 0 new. PR #7648 consolidates TypeScript stream strategy Carried forward from the last review: tests (no author changes in their files since then; earlier findings stand) Not re-run: api-compat, docs, compat-flags, design-simplicity (no author changes in their files since the last review) Reviewed commit: 556b9c8d · github run |
| test() { | ||
| const writable = countingStrategy(2); | ||
| const readable = countingStrategy(3); | ||
| const ts = new TransformStream({}, writable.strategy, readable.strategy); | ||
| deepStrictEqual(writable.reads, { highWaterMark: 1, size: 1 }); | ||
| deepStrictEqual(readable.reads, { highWaterMark: 1, size: 1 }); | ||
| strictEqual(ts.writable.getWriter().desiredSize, 2); | ||
| }, |
There was a problem hiding this comment.
[WARNING] This only constructs with {}, which takes the elided branch at transform.ts:557; the separately changed standard branch at transform.ts:622 is never exercised. Restoring the former second readableStrategy.highWaterMark read only in that branch would still pass this test.
| test() { | |
| const writable = countingStrategy(2); | |
| const readable = countingStrategy(3); | |
| const ts = new TransformStream({}, writable.strategy, readable.strategy); | |
| deepStrictEqual(writable.reads, { highWaterMark: 1, size: 1 }); | |
| deepStrictEqual(readable.reads, { highWaterMark: 1, size: 1 }); | |
| strictEqual(ts.writable.getWriter().desiredSize, 2); | |
| }, | |
| test() { | |
| for (const transformer of [{}, { transform() {} }]) { | |
| const writable = countingStrategy(2); | |
| const readable = countingStrategy(3); | |
| const ts = new TransformStream( | |
| transformer, | |
| writable.strategy, | |
| readable.strategy | |
| ); | |
| deepStrictEqual(writable.reads, { highWaterMark: 1, size: 1 }); | |
| deepStrictEqual(readable.reads, { highWaterMark: 1, size: 1 }); | |
| strictEqual(ts.writable.getWriter().desiredSize, 2); | |
| } | |
| }, |
guybedford
left a comment
There was a problem hiding this comment.
The read-once change checks out: all five double-read sites are collapsed, readable.ts and identity.ts already read once, and in transform.ts the readable strategy is read before transformer.start() runs so user code can't observe a difference from up-front conversion. strategies, transform and writable suites all pass locally across the cpp/ts/pedantic/legacy variants.
One thing on the ledger #3 row this PR edits: "strategy first (size, hwm, sink.write; spec)". WebIDL converts dictionary members in lexicographic order, so highWaterMark is read before size; that's the order C++ uses (sink.write, strategy.highWaterMark, strategy.size) and Node (const { highWaterMark, size } = strategy). TS still reads size first in both the WritableStream and ReadableStream constructors, so "(spec)" there only holds for strategy-before-sink, not the intra-strategy order. Either swapping the rawHWM read above the sizeFn read in writable.ts/readable.ts (and updating argumentConversionOrder), or dropping "(spec)" from that cell, would make the row accurate. Fine as a follow-up.
Nit: countingStrategy is duplicated verbatim across the three test modules; acceptable given the suites are self-contained.
e5835b5 to
3869084
Compare
The TS WritableStream constructor, both TransformStream paths (the readable strategy; the writable one goes through WritableStream) and the CountQueuingStrategy and ByteLengthQueuingStrategy constructors read highWaterMark twice: once to check for undefined, once to convert it. WebIDL dictionary conversion reads each member once, and so does C++. A getter on the strategy ran twice under TS (counting getter: TS 2, C++ 1 at each site). ReadableStream already read it once. Each site now reads the member into a local and works from that. Pins (all parity): writable construction.js strategyMembersReadOnce and transform construction.js strategyMembersReadOnce (highWaterMark and size each read once, for both of TransformStream's strategies), and strategies construction.js initHighWaterMarkReadOnce (both strategy classes). The writable argumentConversionOrder pin, which recorded the TS double read, now expects a single read; the writable ledger #3 cell and comment match. Without the source change, those four fail, in the TS cells only. Compatibility: the change is behind the experimental typescript_implemented_streams flag.
3869084 to
556b9c8
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## jasnell/ts-streams-identity-nul-doc #7648 +/- ##
======================================================================
Coverage ? 38.61%
======================================================================
Files ? 868
Lines ? 268653
Branches ? 25435
======================================================================
Hits ? 103732
Misses ? 150696
Partials ? 14225 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Read highWaterMark only once. See commit for details