Revisiting what's reasonable return value for each method of the controller #424
Assignees
Comments
Only behind a flag!! We're safe. I think I agree with all your proposed fixes. Some of the issues you point out just sound like bad inconsistencies. Your fixes generally move in the direction of adding more errors for potential misbehavior. We can always loosen them if we find out that they are problematic later (e.g. for the close case especially). |
tyoshino
added a commit
that referenced
this issue
Jan 20, 2016
…y in the closed state It was throwing the stored error but is also changed to throw a predefined TypeError. Issue: #424
24 tasks
|
Done in #418. As noted in #418 (comment), added a list of things done in the PR to the OP of the PR. The list includes the action items of this issue. |
tyoshino
added a commit
that referenced
this issue
Feb 18, 2016
This commit includes the following design changes: - sync the spec text with the new reference implementation - #423 - add byobRequest getter to the controller - port the asserts in the non-byte source version to the byte source version - remove auto release feature from the byte source version - rename Byob to BYOB - move TransferArrayBuffer to helpers.js - make the default highWaterMark of the byte source version to 0 - port the functionality that the start method can delay pulling by returning a pending Promise to the byte source version - add `[[disturbed]]` feature to the byte source version - port highWaterMark mechanism to ReadableByteStreamController - merge ReadableStream and ReadableByteStream - now it behaves differently based on whether the given source has pullInto or not - bunch of renaming for merge - merge ReadableStreamReader and ReadableByteStreamReader - #424 - make controller.close() and controller.enqueue() fail when the stream is not in the readable state - make controller.enqueue() throw a predefined TypeError, not the stored error
tyoshino
added a commit
that referenced
this issue
Feb 18, 2016
This commit includes the following design changes: - sync the spec text with the new reference implementation - #423 - add byobRequest getter to the controller - port the asserts in the non-byte source version to the byte source version - remove auto release feature from the byte source version - rename Byob to BYOB - move TransferArrayBuffer to helpers.js - make the default highWaterMark of the byte source version to 0 - port the functionality that the start method can delay pulling by returning a pending Promise to the byte source version - add `[[disturbed]]` feature to the byte source version - port highWaterMark mechanism to ReadableByteStreamController - merge ReadableStream and ReadableByteStream - now it behaves differently based on whether the given source has pullInto or not - bunch of renaming for merge - merge ReadableStreamReader and ReadableByteStreamReader - #424 - make controller.close() and controller.enqueue() fail when the stream is not in the readable state - make controller.enqueue() throw a predefined TypeError, not the stored error
domenic
pushed a commit
that referenced
this issue
Mar 3, 2016
This commit includes the following design changes: - sync the spec text with the new reference implementation - #423 - add byobRequest getter to the controller - port the asserts in the non-byte source version to the byte source version - remove auto release feature from the byte source version - rename Byob to BYOB - move TransferArrayBuffer to helpers.js - make the default highWaterMark of the byte source version to 0 - port the functionality that the start method can delay pulling by returning a pending Promise to the byte source version - add `[[disturbed]]` feature to the byte source version - port highWaterMark mechanism to ReadableByteStreamController - merge ReadableStream and ReadableByteStream - now it behaves differently based on whether the given source has pullInto or not - bunch of renaming for merge - merge ReadableStreamReader and ReadableByteStreamReader - #424 - make controller.close() and controller.enqueue() fail when the stream is not in the readable state - make controller.enqueue() throw a predefined TypeError, not the stored error
domenic
pushed a commit
that referenced
this issue
Mar 25, 2016
## Background We originally designed ReadableByteStream, which included extended features to handle bytes, as a separate class from the ReadableStream, which handles a stream of general objects. While designing the details of ReadableByteStream (see #361), we noticed that we could simplify ReadableStream and ReadableByteStream by moving variables and logic for handling queuing into their controller class (see #379). This turned out to also clarify which part of the logic represents semantic requirements of readable streams, and which part of it is implementing helper logic for easier development of underlying sources. After the above refactoring, we also noticed that ReadableStream and ReadableByteStream share most of their code. So, we merged the ReadableByteStream class into ReadableStream. This has many benefits for developers who don't have to deal with two similar-but-different classes. Instead, the same class is used, with the behavior customized by the underlying source. ## Change summary The new ReadableStream class has two reader acquisition methods, getReader() and getBYOBReader(), with the latter working when BYOB reading is available. Availability of BYOB reading is determined by whether or not the underlying source passed to the ReadableStream had BYOB pulling functionality. This is indicated by a truthy `byob` property of the underlying source. The two readers are named the default reader and BYOB reader. The default reader's read() method takes no argument. The resulting promise of the read() method will be fulfilled with a newly-allocated chunk. The BYOB reader's read() method takes one argument; it must be an ArrayBuffer view. The ReadableStream will fill the passed ArrayBuffer view, and fulfill the returned promise with it. The ArrayBuffer view might be transferred several times, but the same backing memory is always written to. When the byob option is falsy, the underlying source is given a ReadableStreamDefaultController. This class provides methods to enqueue a new chunk and know the status of the queue. Its queuing strategy is configured by the parameters passed to the ReadableStream's constructor. The underlying source can subscribe to know when chunks are drained from the queue by implementing the "pull" method. Only the getReader() method will be functional on the ReadableStream. When the byob option is truthy, the underlying source is given a ReadableStreamBYOBController. In addition to the functionalities of the ReadableStreamDefaultController, this controller provides a getter named `byobRequest` which exposes the oldest outstanding BYOB reading request into which the underlying source can put bytes directly (see #423). Both the getReader() and the getBYOBReader() method will be functional on the ReadableStream. The ReadableStreamBYOBController can be configured to convert read requests from a default reader into BYOB read requests, by automatically allocating a buffer and exposing it via the byobRequest getter. This eases implementation of a reactive underlying source, as shown in one of the new examples. ## Changes included in this commit In addition to the major changes as described above, this commit includes bunch of design /logic/aesthetic changes as follows: ### Changes to existing observable features Although ReadableStream's internals were refactored immensely, its external behavior (when not providing a BYOB source) is almost identical to before, as verified by our extensive unit tests. However, we did make a few changes which are observable: - Changes to the semantics of the controller methods (see #424): - Make controller.close() and controller.enqueue() fail when the stream is not in the readable state - Make controller.enqueue() throw a predefined TypeError, not the stored error - (As a result of these changes, the tests test/pipe-to-options.js, test/readable-streams/general.js, and test/readable-stream-templated.js have been updated.) - Rename ReadableStreamController to ReadableStreamDefaultController - Rename ReadableStreamReader to ReadableStreamDefaultReader ### Changes to byte streams As explained above, byte streams were changed in fairly extensive ways to merge them into the base ReadableStream class. Here we call out a few notable changes from the previous specification text: - Remove auto release feature from the ReadableByteStream - Rename Byob to BYOB - Make the default highWaterMark of the byte source version to 0 - Port the functionality that the start method can delay pulling by returning a pending promise to the ReadableStreamBYOBController - Port the highWaterMark mechanism to ReadableByteStreamController - Rename ReadableByteStreamController to ReadableStreamBYOBController - Correctly update the [[disturbed]] slot in the byte handling logic - read(view) now checks view.byteLength before setting [[disturbed]]
tyoshino
added a commit
that referenced
this issue
Mar 28, 2016
## Background We originally designed ReadableByteStream, which included extended features to handle bytes, as a separate class from the ReadableStream, which handles a stream of general objects. While designing the details of ReadableByteStream (see #361), we noticed that we could simplify ReadableStream and ReadableByteStream by moving variables and logic for handling queuing into their controller class (see #379). This turned out to also clarify which part of the logic represents semantic requirements of readable streams, and which part of it is implementing helper logic for easier development of underlying sources. After the above refactoring, we also noticed that ReadableStream and ReadableByteStream share most of their code. So, we merged the ReadableByteStream class into ReadableStream. This has many benefits for developers who don't have to deal with two similar-but-different classes. Instead, the same class is used, with the behavior customized by the underlying source. ## Change summary The new ReadableStream class has two reader acquisition methods, getReader() and getBYOBReader(), with the latter working when BYOB reading is available. Availability of BYOB reading is determined by whether or not the underlying source passed to the ReadableStream had BYOB pulling functionality. This is indicated by a truthy `byob` property of the underlying source. The two readers are named the default reader and BYOB reader. The default reader's read() method takes no argument. The resulting promise of the read() method will be fulfilled with a newly-allocated chunk. The BYOB reader's read() method takes one argument; it must be an ArrayBuffer view. The ReadableStream will fill the passed ArrayBuffer view, and fulfill the returned promise with it. The ArrayBuffer view might be transferred several times, but the same backing memory is always written to. When the byob option is falsy, the underlying source is given a ReadableStreamDefaultController. This class provides methods to enqueue a new chunk and know the status of the queue. Its queuing strategy is configured by the parameters passed to the ReadableStream's constructor. The underlying source can subscribe to know when chunks are drained from the queue by implementing the "pull" method. Only the getReader() method will be functional on the ReadableStream. When the byob option is truthy, the underlying source is given a ReadableStreamBYOBController. In addition to the functionalities of the ReadableStreamDefaultController, this controller provides a getter named `byobRequest` which exposes the oldest outstanding BYOB reading request into which the underlying source can put bytes directly (see #423). Both the getReader() and the getBYOBReader() method will be functional on the ReadableStream. The ReadableStreamBYOBController can be configured to convert read requests from a default reader into BYOB read requests, by automatically allocating a buffer and exposing it via the byobRequest getter. This eases implementation of a reactive underlying source, as shown in one of the new examples. ## Changes included in this commit In addition to the major changes as described above, this commit includes bunch of design /logic/aesthetic changes as follows: ### Changes to existing observable features Although ReadableStream's internals were refactored immensely, its external behavior (when not providing a BYOB source) is almost identical to before, as verified by our extensive unit tests. However, we did make a few changes which are observable: - Changes to the semantics of the controller methods (see #424): - Make controller.close() and controller.enqueue() fail when the stream is not in the readable state - Make controller.enqueue() throw a predefined TypeError, not the stored error - (As a result of these changes, the tests test/pipe-to-options.js, test/readable-streams/general.js, and test/readable-stream-templated.js have been updated.) - Rename ReadableStreamController to ReadableStreamDefaultController - Rename ReadableStreamReader to ReadableStreamDefaultReader ### Changes to byte streams As explained above, byte streams were changed in fairly extensive ways to merge them into the base ReadableStream class. Here we call out a few notable changes from the previous specification text: - Remove auto release feature from the ReadableByteStream - Rename Byob to BYOB - Make the default highWaterMark of the byte source version to 0 - Port the functionality that the start method can delay pulling by returning a pending promise to the ReadableStreamBYOBController - Port the highWaterMark mechanism to ReadableByteStreamController - Rename ReadableByteStreamController to ReadableStreamBYOBController - Correctly update the [[disturbed]] slot in the byte handling logic - read(view) now checks view.byteLength before setting [[disturbed]]
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
The return value of the controller methods are kinda inconsistent/strange in the following point.
ReadableStreamController.close()when the stream is already closedReadableStreamController.enqueue()ReadableStreamController.error(). It fails if the stream is either closed or errored.ReadableStreamController.error()throws the stored error whileclose()throws a TypeError. I think both should throw a TypeError saying "you're misbehaving".Chromium has shipped it but it's not too late to fix them if necessary. They're "fixed" in RBSController being redesigned in #379 based on my thoughts. I'd like to get your opinion on this.
The text was updated successfully, but these errors were encountered: