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

{ readableStream, writableStream } should be { readable, writable } #40

Closed
domenic opened this issue Aug 11, 2020 · 8 comments · Fixed by guest271314/webrtc-media-streams#2 or #43

Comments

@domenic
Copy link
Contributor

domenic commented Aug 11, 2020

https://htmlpreview.github.io/?https://github.com/w3c/webrtc-insertable-streams/blob/master/index.html#dictdef-rtcinsertablestreams

The conventional names for a pair of readable and writable streams are { readable, writable }, with no suffix. Aligning these will improve interoperability with the rest of the streams ecosystem, both concretely (e.g., making the object usable with pipeThrough()) and just in terms of web developer familiarity.

@guidou
Copy link
Collaborator

guidou commented Aug 11, 2020

An early version of the API had them named {readable,writable}, but the feedback we got was that it was a bad idea to have those names precisely because it could be confused with a TransformStream.
This pair of streams is not a TransformStream and trying to use it as the argument to pipeThrough() would do nothing useful.

@guidou guidou closed this as completed Aug 11, 2020
@domenic
Copy link
Contributor Author

domenic commented Aug 11, 2020

Not just transform streams use { readable, writable }; all "duplex streams" (i.e., all pairs of streams) do.

I can add some more specific "must" guidance to the Streams Standard if that would be helpful in moving this issue forward.

@guidou guidou reopened this Aug 11, 2020
@guidou
Copy link
Collaborator

guidou commented Aug 11, 2020

Reopening to continue the discussion.
cc @alvestrand

@sandersdan
Copy link

I was the source of the feedback mentioned above, and my concern was that it could be very hard to reason about the correct way to use the streams. pipeThough() appearing to work but not doing anything useful seems like it will cause a lot of frustration for first-time users. I think that error being present in the issue description above supports my concern.

Adding a 'Stream' suffix isn't inherently more clear though, it just prevents that specific mistake and encourages users to look at the documentation.

I don't know if you are considering more substantial changes, like for example inverting the relationship to be a method that takes a Transformer and sets up the pipes for you.

@alvestrand
Copy link
Contributor

alvestrand commented Aug 13, 2020

pipeThrough() on this object would create a loop, so pipeThrough() not working is a feature. This API is exposing what's effectively a pipeline insertion point, not a duplex channel.

@domenic
Copy link
Contributor Author

domenic commented Aug 13, 2020

That's a good explanation, and it would have been useful if it were in the spec or explainer. Regardless, it's still ideal for readable/writable pairs to use uniform naming, instead of some of them having arbitrary redundant "-Stream" suffixes.

guest271314 added a commit to guest271314/webrtc-media-streams that referenced this issue Aug 16, 2020
@guidou
Copy link
Collaborator

guidou commented Aug 20, 2020

We're going for the readable/writable name for consistency with other stream pairs.

@guidou guidou closed this as completed Aug 20, 2020
@domenic
Copy link
Contributor Author

domenic commented Aug 20, 2020

Thank you very much! I'll work on getting the Streams spec updated with stronger guidance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants