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

Throw a TypeError in ReadableStreamBYOBReader constructor if stream isn't a byte stream #638

Merged
merged 6 commits into from Jan 6, 2017

Conversation

3 participants
@tschneidereit
Contributor

tschneidereit commented Jan 5, 2017

No description provided.

@ricea

This comment has been minimized.

Show comment
Hide comment
@ricea

ricea Jan 6, 2017

Collaborator

This looks correct to me. I'm going to wait for someone else's opinion on whether we should remove the check from getReader() since it is now redundant. @domenic ?

We will also need the same change made in the reference implementation and a test adding to the web-platform-tests but I can do that if you don't want to.

Collaborator

ricea commented Jan 6, 2017

This looks correct to me. I'm going to wait for someone else's opinion on whether we should remove the check from getReader() since it is now redundant. @domenic ?

We will also need the same change made in the reference implementation and a test adding to the web-platform-tests but I can do that if you don't want to.

@domenic

This comment has been minimized.

Show comment
Hide comment
@domenic

domenic Jan 6, 2017

Member

Yeah I was planning to push some commits to this PR to remove the getReader check, add till to the acks, and update at least the reference implementation. Till says he's working on converting the tests to WPT and will add a test to those when he's done but it wouldn't hurt to update the existing suite too.

Feel free to do so yourself if you have time before I next wake up (at least 12 hours from now) :)

Member

domenic commented Jan 6, 2017

Yeah I was planning to push some commits to this PR to remove the getReader check, add till to the acks, and update at least the reference implementation. Till says he's working on converting the tests to WPT and will add a test to those when he's done but it wouldn't hurt to update the existing suite too.

Feel free to do so yourself if you have time before I next wake up (at least 12 hours from now) :)

@ricea

This comment has been minimized.

Show comment
Hide comment
@ricea

ricea Jan 6, 2017

Collaborator

@domenic PTAL

Collaborator

ricea commented Jan 6, 2017

@domenic PTAL

@tschneidereit

This comment has been minimized.

Show comment
Hide comment
@tschneidereit

tschneidereit Jan 6, 2017

Contributor

@ricea thank you for the additional fixes and tests!

Contributor

tschneidereit commented Jan 6, 2017

@ricea thank you for the additional fixes and tests!

tschneidereit and others added some commits Jan 5, 2017

Test ReadableStreamByobReader constructor
Add a test to verify that the ReadableStreamByobReader constructor
rejects non-byte streams.

Also add tests for normal construction of ReadableStreamByobReader and
other error conditions. All error conditions throw a TypeError so it is
important to ensure that we are triggering the right one.
Fix capitalisation of ReadableStreamBYOBReader
It was capitalised as ReadableStreamByobReader in the new tests.
Reference impl: Move type check for BYOB reader
To match the change to the standard text in the reference
implementation, move the check that the ReadableStream is of 'bytes'
type to the ReadableStreamBYOBReader constructor.
Remove type check from getReader().
Now that the ReadableStreamBYOBReader constructor tests that the
ReadableStream type is 'bytes', there is no longer any need for
getReader() to do it. Remove the redundant check from the standard
text.

@domenic domenic merged commit e4b97e6 into whatwg:master Jan 6, 2017

1 of 2 checks passed

continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment