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 full Streams WebIDL #3046

Merged
merged 4 commits into from Aug 31, 2022
Merged

Conversation

rozbb
Copy link
Contributor

@rozbb rozbb commented Aug 29, 2022

Why this is needed

The Streams living standard is currently broken up into enabled/ReadableStream.webidl and unstable/{ReadableStreamBYOBReader.webidl, ReadableStreamDefaultReader.webidl, ReadableStreamGenericReader.webidl}. There's two issues with this.

  1. This does not encompass the entire standard. For example, ReadableStreamDefaultController does not appear here, and it happens to be the only way to support fetch() progress in Safari (MDN currently lists it as unsupported but I've verified that it works on this site).
  2. I don't think it accurately represents the stability of any component. The Streams spec is a Living Standard, and it makes no claim about ReadableStream's API being more solidified than any other interface's.

So my thought is to take all of the Streams standard and put it in enabled/. If you'd rather it all be in unstable/ that'd be okay too.

Blocking problem

This currently fails to regenerate the Rust bindings with the following error:

Error: compiling WebIDL into wasm-bindgen bindings in file "webidls/enabled/Streams.webidl", line 12 column 1

Caused by:
    failed to parse webidl at byte position 75182

I think it's because the ReadableStream interface is decorated with [Exposed=*, ..., which the parser does not know how to parse. I don't know the best solution here. There is

  1. Just modify the IDL to expand * into whatever namespaces it expands into. This is probably the simplest.
  2. Update weedle to accept the new syntax
  3. Update to weedle2, though I haven't checked if it accepts this syntax It doesn't looks like weedle2 supports wildcards in extended attribute lists.

@alexcrichton
Copy link
Contributor

Sorry I don't know how best to update weedle for the webidl syntax here. Otherwise are all of these APIs supported in major browsers at this time? If so this seems fine to land with the webidl bits updated but otherwise I don't think this is the right time to land it.

@rozbb
Copy link
Contributor Author

rozbb commented Aug 30, 2022

Just made a PR to weedle. I tried it on my local machine and it fixes the build.

Regarding stability: this API is supported in all major browsers except for Safari (see, e.g., this support matrix). Would you rather it all be in unstable/? Your call

@alexcrichton
Copy link
Contributor

Ok that seems reasonable enough to me to support 👍

I'd be happy to merge with weedle updated here

@rozbb
Copy link
Contributor Author

rozbb commented Aug 31, 2022 via email

@alexcrichton
Copy link
Contributor

I did that and it looks like it picked up the changes, but it appears that web-sys still needs regenerating.

@rozbb
Copy link
Contributor Author

rozbb commented Aug 31, 2022

Ope, yup forgot about that. Pushed.

@alexcrichton
Copy link
Contributor

It looks like CI may not be happy with the new crate?

@rozbb
Copy link
Contributor Author

rozbb commented Aug 31, 2022

Didn't notice there were more files from the same API lying around. Fixed.

@alexcrichton alexcrichton merged commit e8a499c into rustwasm:main Aug 31, 2022
@rozbb
Copy link
Contributor Author

rozbb commented Aug 31, 2022

Thank you for the quick turnaround! And also ofc thank you for maintaining this crate ❤️

@Liamolucko
Copy link
Collaborator

There's a problem here - the WebIDL you pulled in doesn't include any of the [Throws] attributes that web-sys relies on to mark functions as throwing (because [Throws] is non-standard). That means that none of these functions return Result when some of them should. (You can see which in Mozilla's WebIDL, which does include [Throws].)

This also happened when ReadableStream was first added (in #2478), which is already stable, so I'm not sure what to do about that.

rozbb added a commit to rozbb/wasm-bindgen that referenced this pull request Sep 3, 2022
Liamolucko pushed a commit that referenced this pull request Sep 4, 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

3 participants