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

ReadableStream.from() static method #28316

Merged
merged 6 commits into from
Aug 21, 2023

Conversation

hamishwillee
Copy link
Collaborator

This adds docs for the new ReadableStream.from() static method, added in FF117 in https://bugzilla.mozilla.org/show_bug.cgi?id=1772772

Related docs work can be tracked in #28282

@github-actions github-actions bot added the Content:WebAPI Web API docs label Aug 1, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 1, 2023

Preview URLs

(comment last updated: 2023-08-21 00:58:16)

@hamishwillee hamishwillee marked this pull request as ready for review August 7, 2023 04:48
@hamishwillee hamishwillee requested a review from a team as a code owner August 7, 2023 04:48
@hamishwillee hamishwillee requested review from wbamberg and removed request for a team August 7, 2023 04:48

## Examples

### Convert an async iterator to a ReadableStream
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a template structure for live samples: https://developer.mozilla.org/en-US/docs/MDN/Writing_guidelines/Page_structures/Code_examples#traditional_live_samples. Although technically authors have a lot of flexibility in how to organize them, I'd prefer us to use a much more consistent approach, because it might make it possible for us to extract live samples and have them be usable in other contexts.

This would include having code blocks in a consistent order, with subheadings, and with prose in a consistent place (at the start of the example)

But maybe I'm fighting a losing battle here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But maybe I'm fighting a losing battle here.

@wbamberg I did it this way, in particular, because I understood that now the code blocks include their "type" the headings are not needed or wanted. With their removal I find the documentation easier to parse, because the isn't quite so much scrolling between the relevant bits.

If you tell me I must change, then I will, but I'm not keen :-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see you marked this as requiring approval, so you do need it changed. Bah. Will do.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't really tell you you must change. I can say that unless examples are consistently organized then it won't be possible to include them in other contexts, and any tool which tries to do that will most likely throw them out (or maybe we will just throw out the explanatory prose, and hope the example still makes sense).

But I don't know if this use case is important enough to be worth the cost of imposing more constraints on authors. I think it is, but maybe others don't.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added the sections in for the first example, unhiding the code that shows how logging works. In the second section I just have the JavaScript section: showing it once perhaps makes sense so there is no "magic" involved in the description, but showing it twice in the page when it is not central to the example seems unhelpful.

I "fairly" strongly feel that this use case is not workable, or even all that useful, and certainly not worth the constraint. Remember all the reasons we decided to remove the page inclusion macro - same problems. The "fairly" is because perhaps you have thought through some cases where content mismatch is not an issue.

I think these headings should be removed. But only after it's agree and https://developer.mozilla.org/en-US/docs/MDN/Writing_guidelines/Page_structures/Code_examples is updated :-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

```

[Using readable streams](/en-US/docs/Web/API/Streams_API/Using_readable_streams) demonstrates several ways to consume a stream.
The code below uses a `for ... await` loop, as this method is the simplest.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great to have some kind of real-world usage example. I find the whole readable streams thing so abstract. In particular it's obviously weird to create a readable stream from an array...so you can iterate it like an array. But I expect this would be quite hard to do (all the other stream examples I've seen look incredibly complicated to me).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're not wrong - they are very abstract, which is why for readable byte stream examples I mocked the spec examples to pretend I had a read data socket.

In this case I couldn't find a real example in the spec or think of one myself. I mean I kind of can "I prefer readable streams to waiting on arrays of promises" so I do it this way. But it's not "really real".

Worth asking the spec authors?

@wbamberg
Copy link
Collaborator

Thanks Hamish. I think it would be good if the examples here followed a more standard organization, and I don't think we should hide significant code. Otherwise this looks great.

Copy link
Collaborator

@wbamberg wbamberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@wbamberg wbamberg merged commit e862ea8 into mdn:main Aug 21, 2023
6 checks passed
@hamishwillee hamishwillee deleted the ff117_readablestream_from branch August 21, 2023 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:WebAPI Web API docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants