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

UnidirectionalStream could be simpler #210

Closed
jan-ivar opened this issue Feb 21, 2021 · 5 comments · Fixed by #233
Closed

UnidirectionalStream could be simpler #210

jan-ivar opened this issue Feb 21, 2021 · 5 comments · Fixed by #233
Labels
Discuss at next meeting Flags an issue to be discussed at the next WG working

Comments

@jan-ivar
Copy link
Member

jan-ivar commented Feb 21, 2021

Conceptually, bidirectional streams (1, 2) are inherent duplex streams, but unidirectional streams (1, 2) are not. They're conceptually just writable streams on sending, and just readable streams on reception.

So it feels like I should be able to write:

const wt = new WebTransport(url);
await readableStreamOfTextData
  .pipeThrough(new TextEncoderStream("utf-8"))
  .pipeTo(await wt.createUnidirectionalStream());

...instead of:

const wt = new WebTransport(url);
await readableStreamOfTextData
  .pipeThrough(new TextEncoderStream("utf-8"))
  .pipeTo((await wt.createUnidirectionalStream()).writable); // clunky

Similarly on reception, it feels more natural to guess I would need to write:

for await (const stream of wt.incomingUnidirectionalStreams) {
    await stream
     .pipeThrough(new TextDecoderStream("utf-8"))
     .pipeTo(createWritableStreamForTextData());
}

...instead of:

for await (const stream of wt.incomingUnidirectionalStreams) {
    await stream.readable // why?
     .pipeThrough(new TextDecoderStream("utf-8"))
     .pipeTo(createWritableStreamForTextData());
}

A counter-argument might be symmetry with bidirectional streams — that I may want to drop in one for the other, or not care which one I used. But this seems like false symmetry. I think I'd rather have a TypeError if I did that, to catch either why no data is being received, or being dropped because I'm not handling it.

The precedent in the streams spec are duplex and transform streams, versus readable and writable streams, which are all called "streams", even though the former have readable and writable members, whereas the latter are those members. They're two classes of inherently incompatible (duck) types: the former accepted by pipeThrough (which returns a readable stream), the latter by pipeTo (which returns a promise) as this or input. Asymmetry abounds.

I think we should follow that same conceptual model to avoid confusion: Have them be OutgoingStream on sending, or IncomingStream on reception.

A second counter-argument might be that OutgoingStream and IncomingStream have other methods on them. But that's not a problem according to @domenic.

This would be a breaking change.

@yutakahirano
Copy link
Contributor

A second counter-argument might be that OutgoingStream and IncomingStream have other methods on them. But that's not a problem according to @domenic.

Can you elaborate on this? I think I don't understand why that's not a problem. IIUC @domenic was talking about putting readable/writable members at the same level with other methods, but this proposal seems to put ReadableStream methods at the same level with other methods.

@domenic
Copy link

domenic commented Feb 24, 2021

Although @yutakahirano's read of my previous comment is correct, I'm sympathetic to the OP.

Putting ReadableStream/WritableStream methods on the same level as other methods is done through subclassing. The example we have so far on the platform is https://wicg.github.io/file-system-access/#api-filesystemwritablefilestream .

IMO if something is called a "stream" then it should either be a ReadableStream/WritableStream or a subclass. (Or a duplex or transform stream; those are a special case.) It sounds like the current spec uses the name "stream" for something that is actually a stream container, i.e. has a .readable or .writable property.

So I'd suggest either: using a ReadableStream/WritableStream subclass, to implement the semantics from the OP. Or, renaming the containers to not use the word "stream", e.g. renaming incomingUnidirectionalStreams to something like incomingUnidirectionalStreamContainers or incomingUnidirectionalTransports or similar.

@wilaw wilaw added the Discuss at next meeting Flags an issue to be discussed at the next WG working label Feb 24, 2021
@jan-ivar
Copy link
Member Author

jan-ivar commented Mar 3, 2021

@ricea Any concerns with doing this? This would be a breaking change.

@ricea
Copy link
Contributor

ricea commented Mar 3, 2021

I have no concrete objections.

I worry that subclassing may come back to bite us in future, but it's just a feeling. There used to be implementation problems in Blink which made subclassing hard to implement. @jorendorff, could you comment on whether subclassing ReadableStream is easy or hard in Firefox?

@jan-ivar
Copy link
Member Author

Meeting: no objections.

@jorendorff any concerns with subclassing streams in Firefox?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discuss at next meeting Flags an issue to be discussed at the next WG working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants