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

GenericTransformStream mixin vs. subclassing #1074

Open
domenic opened this issue Aug 26, 2020 · 2 comments
Open

GenericTransformStream mixin vs. subclassing #1074

domenic opened this issue Aug 26, 2020 · 2 comments
Labels
other spec interface How the Streams spec interfaces with other specifications

Comments

@domenic
Copy link
Member

domenic commented Aug 26, 2020

#1073 works to formalize and formally recommend the pattern used by https://encoding.spec.whatwg.org/ and https://wicg.github.io/compression/, of wrapping up an internal TransformStream and re-exposing it through readable and writable properties.

However, I've lost track of why we do this, instead of subclassing TransformStream.

NativeFS shows how to subclass WritableStream, and #1073 makes it much easier by allowing you to reduce much of the boilerplate there to [=WritableStream/Set up=] [=this=] with |writeAlgorithm| and |closeAlgorithm|. It seems we could similarly make subclassing TransformStream easy.

This isn't very consequential. In general I expect the most noticable effect to be on instanceof. I don't expect us to add any significant public APIs to TransformStream that we'd want subclasses to inherit.

However, I am a bit concerned about recommending a totally different pattern for TransformStream than we do for WritableStream and ReadableStream. So I thought it'd be worth raising, and seeing if perhaps we want to change course.

@ricea
Copy link
Collaborator

ricea commented Aug 26, 2020

See whatwg/encoding#72 (comment) and so on for the historical background.

I'd summarise it as: subclassing from TransformStream places constraints on implementations that don't benefit users. In the case of ReadableStream and WritableStream there's a lot of stuff you get from subclassing, but TransformStream is just a {readable, writable} pair.

Since this debate happened, there have been a few relevant developments:

  • Blink has switched to WebIDL for TransformStream, which makes subclassing it technically easy for Blink, not necessarily other browsers.
  • Transferable streams have added a kind of "fake" TransformStream which just wraps an existing {readable, writable} pair while bypassing the internal machinery.
  • All specs so far using GenericTransformStream delegate to TransformStream internally anyway.

I still prefer to have the GenericTransformStream mixin. Maybe we could make TransformStream include the mixin as well?

@domenic
Copy link
Member Author

domenic commented Aug 27, 2020

Right, OK, thank you for digging that up.

My main concern is that, looking at the section I wrote up in #1073, it looks kind of pointless. The wrapping is basically isomorphic to subclassing; instead of setting up this and operating on it, you create this's transform and operate on this's transform. And, this wrapping is inconsistent with how we handle specializations of WritableStream and ReadableStream.

Maybe we could make TransformStream include the mixin as well?

This doesn't work, because then every TransformStream would have an associated transform, which is a TransformStream, which has an associated transform, which is...

Transferable streams have added a kind of "fake" TransformStream which just wraps an existing {readable, writable} pair while bypassing the internal machinery.

This is pretty interesting, but I'm not sure how to think about how it interacts with the above concerns. (It does indicate that destination.postMessage(ts, { transfer: [ts.readable, ts.writable] }) would have been more honest, but as discussed in #276 (comment), that doesn't really work.)

@domenic domenic added the other spec interface How the Streams spec interfaces with other specifications label Mar 10, 2021
domenic added a commit that referenced this issue Mar 10, 2021
Closes #1084 by encouraging use of "set up" instead. Also partially addresses #1074 by softening the language about subclassing vs. mixins.
ricea pushed a commit that referenced this issue Mar 30, 2021
Closes #1084 by encouraging use of "set up" instead. Also partially addresses #1074 by softening the language about subclassing vs. mixins.
yutakahirano pushed a commit to yutakahirano/streams that referenced this issue Jun 3, 2021
Closes whatwg#1084 by encouraging use of "set up" instead. Also partially addresses whatwg#1074 by softening the language about subclassing vs. mixins.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
other spec interface How the Streams spec interfaces with other specifications
Development

No branches or pull requests

2 participants