stream: rename Duplex.toWeb() type option to readableType#61632
stream: rename Duplex.toWeb() type option to readableType#61632Renegade334 wants to merge 2 commits intonodejs:mainfrom
Duplex.toWeb() type option to readableType#61632Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #61632 +/- ##
==========================================
- Coverage 89.73% 89.73% -0.01%
==========================================
Files 673 675 +2
Lines 203948 204531 +583
Branches 39193 39308 +115
==========================================
+ Hits 183022 183528 +506
- Misses 13240 13301 +61
- Partials 7686 7702 +16
🚀 New features to boost your workflow:
|
|
cc @nodejs/streams – would be grateful for eyeballs. |
|
This also came up during the initial implementation. But I agree, |
|
Can you do it in a non-breaking way? Add the new option and keep the old one. |
As stands, this silently accepts the old option as an alias to the new. Deprecation could follow, or we can keep this behaviour in perpetuity. |
I believe @mcollina means that the old |
Would this be particularly useful? The last precedent for this that I can recall was 68dc15e, and the documentation was just straight-up changed to reflect the renaming, even though the old name was initially left as a deprecated alias. |
jasnell
left a comment
There was a problem hiding this comment.
LGTM. A couple of nits. I would recommend just going ahead and adding a docs-only deprecation for types in deprecated.md
6ed587d to
c16967a
Compare
The webstreams standard uses the name
readableTypefor a source property which specifies thetypeof the readable component of a constructed readable-writable pair.1 Although this is only future-proofed nomenclature at the moment, we should probably follow this convention here, for consistency and just in case we also need to support WritableStream types in the future.This paradigm would also be consistent with
Duplex.toWeb()takingreadableStrategyandwritableStrategyoptions in the future, if so desired.Because
Duplex.toWeb()is no longer marked as experimental, I believe that removingoptions.typeentirely would need a full deprecation cycle, so I've just turned it into a silent alias for simplicity.Refs: #58664
Footnotes
https://streams.spec.whatwg.org/#dictdef-transformer ↩