Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: note readable/writeable .toWeb()/.fromWeb() exist in webstreams docs #45381

Open
bnb opened this issue Nov 8, 2022 · 8 comments · May be fixed by #45840
Open

doc: note readable/writeable .toWeb()/.fromWeb() exist in webstreams docs #45381

bnb opened this issue Nov 8, 2022 · 8 comments · May be fixed by #45840
Labels
doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors. web streams

Comments

@bnb
Copy link
Contributor

bnb commented Nov 8, 2022

Affected URL(s)

https://nodejs.org/api/webstreams.html

Description of the problem

currently, there's no indication on the webstreams docs that .toWeb() and .fromWeb() exist. Noting that they exist somewhere on the webstreams docs would probably be a good idea. I just watched two very capable engineers spend longer than they should have figure out that these features existed while trying to use fetch and streams because they weren't in the webstreams docs - if they're tripping over this, I very much imagine it will be something that comes up again for others.

@bnb bnb added the doc Issues and PRs related to the documentations. label Nov 8, 2022
@nornagon
Copy link
Contributor

nornagon commented Nov 9, 2022

Even better, could this transformation be done automatically? There’s really only one reasonable thing to do when passing a WriteStream to something that expects a WritableStream.

@MrJithil
Copy link
Contributor

MrJithil commented Nov 9, 2022

@bnb
Copy link
Contributor Author

bnb commented Dec 6, 2022

@MrJithil yes, that's the problem. It's only documented in one place, which is notably not the place you'd be looking for context on WebStreams.

@bnb
Copy link
Contributor Author

bnb commented Dec 6, 2022

going to add the good first issue Issues that are suitable for first-time contributors. label to this because it's a relatively minor change - about a paragraph or so - and doesn't need any domain specific knowledge. If any collaborators want to remove that because they disagree with me, feel free.

@bnb bnb added the good first issue Issues that are suitable for first-time contributors. label Dec 6, 2022
@7suyash7
Copy link
Contributor

Hey, @bnb, I Just wanted to ask where the details about .toWeb and .fromWeb() will go? I'm assuming they go under Class: ReadableStream, Class: WritableStream and Class: TransformStream each. Also, should it just be a small description like how it is here or will it be good to add some implementation to it? Thanks! (PS: if the above sounds good, please assign this issue to me)

@bnb
Copy link
Contributor Author

bnb commented Dec 12, 2022

@7suyash7 I'd probably either just at the top under the overview section or as you noted in each of those sections. Either is a valid approach IMO, though PR reviewers might disagree with me. I would recommend just having whatever is implemented as a note that links to the actual API docs for those methods, rather than having code examples. However if you also want to improve those API docs, that's of course welcome as well 😊

@7suyash7
Copy link
Contributor

@bnb PTAL. Thanks!

jayshreek3 pushed a commit to jayshreek3/node that referenced this issue Oct 18, 2023
@jayshreek3
Copy link

Hey, @bnb! Kindly review this fix #50255 for issue #45381
Thanks!

jayshreek3 pushed a commit to jayshreek3/node that referenced this issue Oct 22, 2023
jayshreek3 pushed a commit to jayshreek3/node that referenced this issue Oct 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors. web streams
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants