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

Investigate using native streams #20

Open
MattiasBuelens opened this issue Mar 25, 2019 · 6 comments
Open

Investigate using native streams #20

MattiasBuelens opened this issue Mar 25, 2019 · 6 comments
Labels

Comments

@MattiasBuelens
Copy link
Owner

@MattiasBuelens MattiasBuelens commented Mar 25, 2019

Inspired by this tweet from @surma:

@MattiasBuelens Is it possible to offer up the polyfills for Readable, Writable and Transform individually? Most browsers have Readable, so ideally I’d only load Writable and Transform.

I've thought about this previously. Back then, I decided that it was not feasible because readable byte streams are not supported by any browser. A full polyfill would always need to provide its own ReadableStream implementation that supports byte streams. By extension, it would also need to provide its own implementations for WritableStream (that works with its ReadableStream.pipeTo()) and TransformStream (that uses its readable and writable streams).

Looking at this again, I think we can do better. If you don't need readable byte streams, then the native ReadableStream should be good enough as a starting point for the polyfill. From there, the polyfill could add any missing methods (pipeTo, pipeThrough, getIterator,...) and implement them using the native reader from getReader().

This approach can never be fully spec-compliant though, since the spec explicitly forbids these methods to use the public API. For example, pipeTo() must use AcquireReadableStreamDefaultReader() instead of ReadableStream.getReader(), so it cannot be affected by user-land JavaScript code making modifications to ReadableStream.prototype. I don't think that has to be a problem though: we are already a user-land polyfill written in JavaScript that modifies those prototypes, it would be silly for the polyfill to try and guard itself against other JavaScript code making similar modifications.

Steps in the spec that require inspecting the internal state of the stream or call into internal methods will need to be replaced by something that emulates the behavior using solely the public API.

  • Often, this will be easy: e.g. ReadableStreamDefaultControllerEnqueue() becomes controller.enqueue().

  • Sometimes, we have to be a bit more lenient. ReadableStreamPipeTo()'s error propagation says:

    if source.[[state]] is or becomes "errored"

    We can check if it becomes errored by waiting for the source.closed promise to become rejected. However, we can't synchronously check if it is already errored.

  • In rare cases, this may turn out to be impossible. TransformStreamDefaultSinkWriteAlgorithm specifies:

    If state is "erroring", throw writable.[[storedError]].

    Usually, the writable stream starts erroring because the writable controller has errored, which the transform stream's implementation controls. However, it could also be triggered by WritableStream.abort(), which is out of the control of the transform stream implementation. In this case, the controller is only made aware of it after the writable stream finishes erroring (state becomes "errored") through its abort() algorithm, which is already too late.

Of course, we can't just flat-out remove byte stream support from the polyfill, just for the sake of using native streams more. The default should still be a full polyfill, but we might want to give users the option to select which features they want polyfilled (as @surma suggested in another tweet).

Anyway, I still want to give this a try. It might fail catastrophically, but then at least I'll have a better answer on why we use so little from the native streams implementation. 😅

@jimmywarting

This comment has been minimized.

Copy link

@jimmywarting jimmywarting commented May 19, 2019

👍

It just so struck me with my StreamSaver lib where I try to transfer a ReadableStream to a Service Worker. Native streams work OK, But the polyfilled ReadableStream is not transferable with postMessage

For me native ReadableStream's are much more important than having a full stream specification. (just so that you can get byob).
Also the Response.body stream isn't compatible with the polyfill, so that's a problem too in my case

byob reader isn't available in any browser yet, So my guessing is that hardly anyone are using it today.
think byob is not quite ready yet to be used in the user-land. There is also no blob-read-into, WebSocket-read-into, datachannel-read-into or any other native implementation that utilize this byob mode today so it seems a little unnecessary atm to have byob support as you will already have a allocated buffer from fileReader, websocket datachannel etc so you can as well pass it along and use it. And let the garbage collector do it's job

So i'm guessing i'm in favor of removing byob mode and use native stream instead until browsers becomes more ready for it.


Side note: I also wondering if you can't extend the ReadableStream somehow to add support for byob but still have it being seen as a native stream and still be transferable and also work with native Response

new Response(readable).blob()
const klass = ReadableStream || NoopClass

window.ReadableStream = class Polyfill extends klass {
  constructor (args) {
    // check if byob mode and make magic happen
    super(...args)
  }

  someMethod () {
    super.someMethod()
  }
}

Maybe will be more troublesome then actually fixing the missing methods to native ReadableStream.prototype 🤔 but can maybe be the way to solve it?

@jimmywarting

This comment has been minimized.

Copy link

@jimmywarting jimmywarting commented May 19, 2019

Just tried this:

class Foo extends ReadableStream {
  constructor (...args) {
    super(...args)
    console.log('Hi foo, i fix byob for ya')
  }
}

const rs = new Foo({
  start (ctrl) {
    ctrl.enqueue(new Uint8Array([97]))
    ctrl.close()
  }
})

new Response(rs).text().then(console.log)

const rs2 = new Foo()
postMessage(rs2, '*', [rs2])

Works.

(the current polyfill version can't do this - since it's not a native stream)

using native stream instead seems like it would be better then having some "convert to/from native stream" (#1)

@MattiasBuelens

This comment has been minimized.

Copy link
Owner Author

@MattiasBuelens MattiasBuelens commented May 19, 2019

Thanks for your input! 😄

byob reader isn't available in any browser yet, So my guessing is that hardly anyone are using it today. [...]

So i'm guessing i'm in favor of removing byob mode and use native stream instead until browsers becomes more ready for it.

I know at least one person is using BYOB readers with this polyfill, because they found a bug with it: #3.

I want to keep BYOB support, but make it an optional feature rather than a default one. I see two options:

  • Include it by default, but give users a way to opt-out by using a separate variant. This is backwards-compatible, but it means that by default users will get a larger polyfill that can't leverage the native implementation.
  • Don't include it by default, and give users a way to opt-in. This means that the default polyfill can likely use the native implementation, but it would be a breaking change for users currently using BYOB support.

Side note: I also wondering if you can't extend the ReadableStream somehow to add support for byob but still have it being seen as a native stream and still be transferable and also work with native Response

I've had the same idea as well! But it won't be easy.

The PolyfillReadableStream constructor must call the super constructor (i.e. the native ReadableStream constructor). That super constructor only accepts a default underlying source. We cannot pass a "dummy" underlying source, since then super.getReader() wouldn't work (and by extension super.pipeTo() or super.tee() wouldn't work either). So we would need to somehow wrap our original byte source in a default underlying source, so the native implementation can use it. I'm not sure if this is at all possible.

If we can make the constructor work, the rest would be fairly straightforward:

  • getReader() will just return super.getReader(), since our underlying source wrapper will take care of everything. Similarly, pipeTo(), pipeThrough() and tee() will also "just work".
  • getReader({ mode: 'byob' }) must return a polyfilled BYOB reader. We can wrap the default reader from super.getReader() and use its cancel() and releaseLock() implementation. (Since this also locks the native stream, we won't have to do anything special for ReadableStream.locked.) Of course, we have to implement read(view) ourselves using the original byte source, to make it a proper BYOB read.

It'd be incredible if we could get this to work though! It would mean we could restructure the polyfill to "progressively enhance" native streams:

let PolyfillReadableStream;
if (supportsDefaultSource(ReadableStream)) {
  // native supports default source
  PolyfillReadableStream = class extends ReadableStream {};
} else {
  // no native support
  PolyfillReadableStream = PonyfillReadableStream;
}

if (!supportsByteSource(PolyfillReadableStream)) {
  // polyfill byte stream on top of default stream
  PolyfillReadableStream = class extends PolyfillReadableStream {
    /* ... */
  };
}

if (!PolyfillReadableStream.prototype.pipeTo) {
  // polyfill pipeTo and pipeThrough
}

if (!PolyfillReadableStream.prototype.tee) {
  // polyfill tee
}
// ...

We could put these in separate modules, and have users pick only the features they care about (like with core-js):

import {ReadableStream} from 'web-streams-polyfill';

// polyfill only pipeTo
// import `web-streams-polyfill/feature/readable-byte-stream`;
import `web-streams-polyfill/feature/pipe-to`;
// import `web-streams-polyfill/feature/tee`;

const readable = new ReadableStream();
readable.pipeTo(writable);

So yeah: would be cool, if we can get it to work. 😛

@jimmywarting

This comment has been minimized.

Copy link

@jimmywarting jimmywarting commented Jul 7, 2019

Any progress?

@MattiasBuelens

This comment has been minimized.

Copy link
Owner Author

@MattiasBuelens MattiasBuelens commented Jul 9, 2019

I'm currently working on splitting the ReadableStream class into multiple modules (one for each feature), to get a better understanding on the dependencies between each feature.

After that, I have to figure out how these dependencies should be implemented so they can work with either native and polyfilled streams, without breaking any (or too many) tests. For example: ReadableStreamPipeTo uses AcquireReadableStreamDefaultReader. For polyfilled streams, this should call our implementation of this abstract operation so we can set forAuthorCode = false. However, for native streams, we only have stream.getReader() so we will always have forAuthorCode = true. This means that some tests will fail when implementing pipeTo() on top of native readers. I think it's fine in this case, but this is just one of many cases that will need to be considered.

I'm also worried that some of these dependencies on abstract operations cannot be implemented using only the public API of native streams. This would mean I'd have to approximate them, or leave them out entirely. That means more trade-offs about which test failures are acceptable and which aren't. For example: TransformStreamDefaultSinkWriteAlgorithm checks whether writable.[[state]] === "erroring", but a regular writable sink would only know about this after all pending write()s are completed and the sink's abort() method gets called. That means the write algorithm cannot know whether it should skip the PerformTransform() call and let the writable stream become errored, which is definitely going to break at least one test.

There's still a lot of questions, and I'm figuring them out as I go. I'm doing this in my spare time, so it's going to take a bit of time to get there! 😅

@jimmywarting

This comment has been minimized.

Copy link

@jimmywarting jimmywarting commented Jul 9, 2019

Oh, sounds a bit complex 😅
I hope using native streams outweighs the trade-offs.
Thanks for the status update ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.