Skip to content

simplify WritableStream example #38536

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

Merged
merged 5 commits into from
Apr 16, 2025
Merged

simplify WritableStream example #38536

merged 5 commits into from
Apr 16, 2025

Conversation

phoddie
Copy link
Contributor

@phoddie phoddie commented Mar 8, 2025

Description

The example is needlessly complex. It converts a string to a Uint8Array, then writes each byte of the array separately, and reconstructs the message later. It could simply send the string. Streams are confusing enough. The examples shouldn't add to that.

Motivation

I want developers to be able to understand essential concepts of streams and JavaScript clearly.

Additional details

The example refers to the bytes within a Uint8Array as "chunks". They are just numbers. This is confusing. The confusion is eliminated by simply writing the message as a string to the stream.

@phoddie phoddie requested a review from a team as a code owner March 8, 2025 23:59
@phoddie phoddie requested review from wbamberg and removed request for a team March 8, 2025 23:59
@github-actions github-actions bot added Content:WebAPI Web API docs size/s [PR only] 6-50 LoC changed labels Mar 9, 2025
Copy link
Contributor

github-actions bot commented Mar 9, 2025

Preview URLs

@hamishwillee
Copy link
Collaborator

Thanks @phoddie. Fly-by comment (i.e. @wbamberg is the reviewer).

FWIW The current wording and design of the example is intended. The design is about showing features of the API - it uses a text encoder so that we can chop up the sent string into chunks of data we can safely send and reconstruct.

I do slightly agree with the point that it is confusing that we're talking about sending chunks but we're just sending bytes/numbers here. As a reader I'd at least be interested in knowing that these really could be chunks of fixed/arbitrary sized data. That might be fixed with a comment about what we are sending vs what we could be sending.

Definitely "messages" is not the right terminology to use here for what we are sending

@wbamberg
Copy link
Collaborator

Yes, I didn't properly look at this yet but this was also my reaction: the point of the example is to send the data in chunks, that's why it's using streams.

I do think these examples are really hard to understand, partly because the code is inherently complex and partly because they are so abstract - having a plausible concrete use case would help a lot. But I'm not sure this PR is the right approach to fixing it.

@hamishwillee
Copy link
Collaborator

I don't have a great solution either, which is why I went "fly by mode" :-)

The minimal fix would be to explain the compromises made. For an example, maybe some kind of chat application? You could then use words as the chunks. Dunno.

@phoddie
Copy link
Contributor Author

phoddie commented Mar 11, 2025

Thanks for the notes. I understand that it is important to use the word "chunks" here because that is a term in the Streams specification:

A chunk is a single piece of data that is written to or read from a stream. It can be of any type; streams can even contain chunks of different types

Here's an idea. Go back to using the term chunk, and then change the single call to sendMessage("Hello, world.) to two, to show the use of multiple chunks.

sendMessage("Hello, ", writableStream);
sendMessage("world.", writableStream);

While we're discussing this example... it is kind of old-school to use then instead of await. Maybe this should be updated to use async functions? I think that would improve readability for most developers.

@hamishwillee
Copy link
Collaborator

While we're discussing this example... it is kind of old-school to use then instead of await. Maybe this should be updated to use async functions? I think that would improve readability for most developers.

I think moving to using await might help too.

@phoddie
Copy link
Contributor Author

phoddie commented Mar 14, 2025

How about this? It naintains much of the original but uses await instead of then() and catch(). It also eliminates TextEncode/Decode and splitting Uint8Arrays to bytes and reassembling – neither of which contributes to understanding WritableStream.

const writableStream = new WritableStream(
  // Implement the sink
  {
    write(chunk) {
      const textElement = document.getElementById('text-output');
      textElement.textContent += chunk;
    }
  }
);

const writer = writableStream.getWriter();

try {
  await writer.ready;
  writer.write('Hello, ');
  writer.write('world!\n');
  writer.write('This has been a demo!\n');

  await writer.ready; // Ensure all chunks written before closing writer
  await writer.close();
  console.log("All chunks written")
}
catch (err) {
  console.error("Stream error:", err);
}

@wbamberg
Copy link
Collaborator

wbamberg commented Mar 14, 2025

Yes, I agree that using async/await would simplify the examples a lot and has no drawbacks that I can see. I had meant to say that before when I talked about code complexity but didn't manage to articulate that.

I do think your version is a lot more readable and makes the basic usage of the API much clearer.

I suppose the reason the existing example does text decoding is that in general, with streams, you don't know where the chunk boundaries are going to be. That is, the sender just sends a continuous stream of data and the receiver has to make sense of it - which means, if it's expecting UTF-8 text, it has to use a decoder to deal with multibyte encodings. I'm not sure how much it matters that this example isn't doing that. Maybe it's OK.

Note that there are unfortunately lots of versions of this example, and other quite complex examples, in the streams documentation and they often refer to the live examples at https://github.com/mdn/dom-examples/tree/main/streams .

@phoddie
Copy link
Contributor Author

phoddie commented Mar 14, 2025

Yes, I agree that using async/await would simplify the examples a lot and has no drawbacks that I can see. I had meant to say that before when I talked about code complexity but didn't manage to articulate that.

I do think your version is a lot more readable and makes the basic usage of the API much clearer.

Thanks!

I suppose the reason the existing example does text decoding is that in general, with streams, you don't know where the chunk boundaries are going to be. That is, the sender just sends a continuous stream of data and the receiver has to make sense of it - which means, if it's expecting UTF-8 text, it has to use a decoder to deal with multibyte encodings. I'm not sure how much it matters that this example isn't doing that. Maybe it's OK.

Not all streams use chunks that are buffers of binary data. This current example shows that - it sends bytes as numbers, though it is not entirely obvious on a quick read. My feeling is that since is an example of WritableStream, it should stay focused on that. Assembling chunks of buffers of bytes into a string in a stream is something else. Perhaps that would be more appropriate in an example more focused on buffers of bytes, like ByteLengthQueuingStrategy. If it is important here, then perhaps it could be a second example, to show more advanced uses.

Note that there are unfortunately lots of versions of this example, and other quite complex examples, in the streams documentation...

There are. Trying to do them all at once is daunting. I like an incremental approach of fixing one well and then taking on another. I choose this one because it confused me when I was looking to MDN for help. ;)

@wbamberg
Copy link
Collaborator

OK, that sounds totally reasonable to me. If you'd like to update your PR with #38536 (comment) I'm happy to merge that :).

@phoddie
Copy link
Contributor Author

phoddie commented Mar 27, 2025

Cool. In the code above, I copied this bit more-or-less from the original:

  await writer.ready; // Ensure all chunks written before closing writer
  await writer.close();

The comment is incorrect. writer.ready doesn't provide information about chunks being written. It indicates there's no back pressure so more chunks may be written. Those two lines should be collapsed to :

  await writer.close();

The explanatory text below needs to be updated to reflect that. These sentences are incorrect both for the original example and the updated example. I'll do that in the updated PR.

  • "Calling close() too early can prevent data from being written. This is why the example calls defaultWriter.ready twice."
  • "The Promise returned by the sink's write() method tells the WritableStream and its writer when to resolve defaultWriter.ready."

@github-actions github-actions bot added size/m [PR only] 51-500 LoC changed and removed size/s [PR only] 6-50 LoC changed labels Mar 27, 2025
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@wbamberg
Copy link
Collaborator

wbamberg commented Apr 1, 2025

Thank you! I'll get to this but probably not this week.

@Josh-Cena
Copy link
Member

Note, this same example is in https://developer.mozilla.org/en-US/docs/Web/API/Streams_API/Using_writable_streams, all WritableStream members, and all WritableStreamDefaultWriter members. You probably want to update those as well.

@phoddie
Copy link
Contributor Author

phoddie commented Apr 1, 2025

Note, this same example is in https://developer.mozilla.org/en-US/docs/Web/API/Streams_API/Using_writable_streams, all WritableStream members, and all WritableStreamDefaultWriter members. You probably want to update those as well.

Absolutely. @wbamberg and I discussed that above. The idea is to land this change and then work through the related examples. I'd also like to include a simple example here that shows the correct use of back pressure when writing; it is an important, powerful concept that often appears to be misunderstood.

Copy link
Contributor

Preview URLs

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.

👍 thank you, I think this looks great. Sorry to be so slow to review.

@wbamberg wbamberg merged commit 84e780d into mdn:main Apr 16, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:WebAPI Web API docs size/m [PR only] 51-500 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants