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

web-sys: Added [Throws] attributes to Streams API #3052

Closed
wants to merge 2 commits into from

Conversation

rozbb
Copy link
Contributor

@rozbb rozbb commented Sep 1, 2022

Following the comment in #3046 (comment), I went through Streams.webidl and added a [Throws] attribute on every method and member that Mozilla places a [Throws] attribute on.

cc @Liamolucko

Comment on lines -13 to +23
constructor(optional object underlyingSource, optional QueuingStrategy strategy = {});
[Throws] constructor(optional object underlyingSource, optional QueuingStrategy strategy = {});

readonly attribute boolean locked;

Promise<undefined> cancel(optional any reason);
ReadableStreamReader getReader(optional ReadableStreamGetReaderOptions options = {});
ReadableStream pipeThrough(ReadableWritablePair transform, optional StreamPipeOptions options = {});
[Throws] ReadableStreamReader getReader(optional ReadableStreamGetReaderOptions options = {});
[Throws] ReadableStream pipeThrough(ReadableWritablePair transform, optional StreamPipeOptions options = {});
Promise<undefined> pipeTo(WritableStream destination, optional StreamPipeOptions options = {});
sequence<ReadableStream> tee();
[Throws] sequence<ReadableStream> tee();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Although this is correct, it's a breaking change, since ReadableStream is already stable with methods which don't return Result. So, I think we'll have to leave these as being incorrectly infallible.

This isn't a problem for the other types because they were all previously unstable.

@alexcrichton
Copy link
Contributor

Thanks for catching this @Liamolucko and helping to fix it @rozbb! If you're up for it @rozbb one thing that might be helpful is to land a revert of the stream changes and then have a second PR which re-applies it with the [Throws] attribute. That way it'll be easy in the diff to confirm that there are no breaking changes to preexisting APIs.

@rozbb
Copy link
Contributor Author

rozbb commented Sep 1, 2022 via email

@rozbb
Copy link
Contributor Author

rozbb commented Sep 4, 2022

Done! Latest two PRs should resolve this.

@hamza1311
Copy link
Collaborator

@rozbb can you also update this PR? There are merge conflicts

Done! Latest two PRs should resolve this.

I assume this PR is still needed in addition to those, right?

@Liamolucko
Copy link
Collaborator

I assume this PR is still needed in addition to those, right?

No, it isn't - #3065 included all the changes from this PR.

@Liamolucko Liamolucko closed this Sep 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants