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

Minimum viable ReadableStream. #1761

Merged
merged 1 commit into from
Sep 12, 2019

Conversation

quasicomputational
Copy link
Contributor

No methods or attributes are mapped (yet).

This is mostly useful for constructing a Response from another one's
body in a streaming fashion.

--

The Streams spec doesn't currently use WebIDL (whatwg/streams#963) and a quick lock didn't find a ready-made ReadableStream.webidl in Firefox or Chromium, so this is doing the smallest thing that could possibly be useful (and would, in fact, be useful to me).

No methods or attributes are mapped (yet).

This is mostly useful for constructing a `Response` from another one's
body in a streaming fashion.
@quasicomputational
Copy link
Contributor Author

The two CI failures also seem to be happening on master and other PRs, so I don't think they're related to this change.

@alexcrichton
Copy link
Contributor

Thanks for the PR! I suspect though this is probably the same as #1746 in that it may be blocked on us gating access to these unstable APIs? I suspect ReadableStream is still in development for browsers and isn't stage3+ yet?

@quasicomputational
Copy link
Contributor Author

It's a WHATWG spec; I don't think they do stages.

It's supported out of the box (i.e., without flags) in Firefox and Chromium, though, so I'd be very surprised if there were any changes in the pipeline that would break this MVP without also impacting Web compatibility. The individual methods may be a bit iffier, but just passing it about as an opaque object should be fine, IMO.

@alexcrichton
Copy link
Contributor

Ah ok makes sense! If this doesn't add any methods though, is this buying us much today?

@quasicomputational
Copy link
Contributor Author

Yes - it allows constructing a Response from another Response in a streaming fashion, where the new response can have a different status code. This is a very useful thing for service workers to be able to do.

@alexcrichton
Copy link
Contributor

Ah ok, nice!

To confirm, have you verified that these edits to the webidl aren't API breaking changes for the generated code?

@quasicomputational
Copy link
Contributor Author

Is there a way to do that mechanically? I've run the tests locally and they pass, but, as some famous guy said, testing can only prove the presence of bugs, not their absence.

Using the Mark I Eyeball (human):

  • BodyInit only occurs in two situations: in negative position as an argument, so adding a term to the sum can't break things; and as a field of RequestInit, which treats it as an opaque JsValue in its one relevant method.
  • The change to Response's signature is a no-op: the first argument's type was equivalent to the new BodyInit.
  • Response still has a body attribute via the Body mixin; losing GetterThrows actually brings it in line with the Fetch spec, which doesn't specify that it'll ever throw.

@alexcrichton
Copy link
Contributor

Ok, sounds reasonable to me!

@alexcrichton alexcrichton merged commit f5f9467 into rustwasm:master Sep 12, 2019
@MattiasBuelens
Copy link
Contributor

Hi! 👋 I'm interested in helping bridge the gap between JavaScript ReadableStream/WritableStream and Rust's (upcoming) Streams. My idea was to define the JavaScript types in web-sys, and then add conversions from/to the Stream trait in wasm-bindgen-futures.

I know it's still very early since the Stream trait is not yet part of the standard library, but I already started experimenting with it a bit. I've got a streams branch up on my fork of wasm-bindgen, where I've added a couple of methods and types to the WebIDL files so Rust can create and manipulate JS streams. It can already do the basic stuff: create a ReadableStream with an underlying source, acquire a reader for it and read chunks from the reader.

Unfortunately, since the streams spec does not yet have an official WebIDL definition, I can't really open a PR for this just yet. Although the classes already have well-defined names (ReadableStream, ReadableStreamDefaultReader, WritableStream,...), many of the "dictionary-like" types (such as the underlying source API or pipeTo's second argument) do not yet have an official name. These dictionary types will also appear in web-sys's public API, and we probably don't want to end up picking the wrong name and having to do a breaking change later on to fix them. For now, I'll wait for the official WebIDL first.

If anyone else is interested in this kind of stuff or wants to continue from my work-in-progress branch, feel free to let me know. 😄

@alexcrichton
Copy link
Contributor

Thanks for thie @MattiasBuelens!

FWIW the WebIDL is just generating #[wasm_bindgen] annotations and they can all be written by hand as well! In that sense it's not required that support be added to web-sys to work with this.

Additionally I think given the stability of the Future trait we'd probably want to put the streams suport in a separate crate, does that sound ok?

@MattiasBuelens
Copy link
Contributor

MattiasBuelens commented Oct 18, 2019

FWIW the WebIDL is just generating #[wasm_bindgen] annotations and they can all be written by hand as well! In that sense it's not required that support be added to web-sys to work with this.

Right, of course! 😅 I can make a crate that defines the JS bindings internally, so they don't have to be part of any public API just yet. I'll give that a try.

Additionally I think given the stability of the Future trait we'd probably want to put the streams suport in a separate crate, does that sound ok?

Yeah, I'll first start working on my own crate for streams interoperability. If it works out, we can always look into merging it into another crate later on.

Thanks for the input! 😄

@Pauan
Copy link
Contributor

Pauan commented Oct 19, 2019

@MattiasBuelens That sounds like a good crate to add to gloo.

@MattiasBuelens
Copy link
Contributor

Quick update: I've just published the first version of wasm-streams! 🎉 To quote the docs:

This crate provides wrappers around ReadableStream, WritableStream and TransformStream. It also supports converting from and into Streams and Sinks from the futures crate.

The API is still somewhat experimental, so I'd love to hear your input. I want to make this a part of gloo at some point (and deprecate my crate), but I think it's better to iterate a bit more first before I draft up a proper RFC.

In other news: whatwg/streams#963 has landed recently, and the streams specification now has an official WebIDL definition! I'm currently working on a PR to add these definitions to web-sys, so I can replace the manual bindings in wasm-streams with the auto-generated ones from web-sys.

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.

4 participants