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

add "bytes" readableType to TransformStream transformer #601

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

isonmad
Copy link
Contributor

@isonmad isonmad commented Nov 5, 2016

Leaves room for writableType as well, which the WritableStream constructor will reject if it's anything other than undefined, at the moment.

Also removes a pointless catch(e) { assert(false); } block.


Preview | Diff

Copy link
Collaborator

@ricea ricea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the benefit of adding byte stream support on the readable side when we don't yet have it on the writable side. So I will leave this for @domenic or @tyoshino to review, as they are better qualified to comment on the impact of this change.

@@ -379,23 +435,26 @@ class TransformStream {

this._transformStreamController = new TransformStreamDefaultController(this);

this._readableType = transformer.readableType;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason not to validate the value of readableType and writableType here? It seems weird to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was relying on the other two stream constructors already validating it, but I suppose not.

@domenic
Copy link
Member

domenic commented Nov 5, 2016

I tend to agree we should be conservative.

@isonmad
Copy link
Contributor Author

isonmad commented Nov 5, 2016

Is it not useful to allow optimized 'objects-to-bytes' serializing transforms, even before the other 2 combinations of bytes-to-bytes and bytes-to-objects?

@tyoshino
Copy link
Member

tyoshino commented Nov 7, 2016

This attempt clarified some issues regarding extension of TransformStream to more variants corresponding to the (current and future) RS/WS variants. So, it's really useful. Thanks. E.g. I think we don't want to make the TransformStreamController be an all-in-one class with disabled methods (e.g. the byobRequest getter). OTOH, we also don't want to have a lot of TransformStream variants defined for each combination.

The TransformStream class is basically a helper for implementing stuff following the transform streams concept and explanation of one reasonable backpressure handling. No one is disallowed to directly use the ReadableXXXStream and WritableXXXStream to build a TransformStream. I guess we shouldn't bother ourselves for maintaining a lot of wrappers.

I agree that objects-to-bytes use cases are not uncommon. But I think we should be careful not to inflate the spec. Can we componentize the TransformStream class to avoid the all-in-one controller and also avoid combinatorial explosion? Such attempt could also result in some additional complexity, but my gut feeling is we should explore that.

@domenic
Copy link
Member

domenic commented Nov 16, 2016

An obvious objects-to-bytes use case is TextEncoder.

@ricea
Copy link
Collaborator

ricea commented Nov 21, 2016

I created #616 to discuss TransformStream byte streams independently of this particular pull request. Hopefully we can decide the direction there and then come back here.

@ricea
Copy link
Collaborator

ricea commented Nov 21, 2016

Random observation: I find writableType/readableType confusing and would prefer to have inputType/outputType.

@domenic
Copy link
Member

domenic commented Nov 21, 2016

Originally transform streams were { input, output } but people found this confusing :(. #174

@ricea
Copy link
Collaborator

ricea commented Nov 21, 2016

I didn't realise that this had already been bikeshedded.

I would like to be pedantic and say that writableType/readableType are from the point of view of the author of the transform, whereas {writable, readable} are from the point of view of the consumer of the transform. But even I don't find this particularly convincing, so I withdraw my objection.

@tyoshino
Copy link
Member

I would like to be pedantic and say that writableType/readableType are from the point of view of the author of the transform, whereas {writable, readable} are from the point of view of the consumer of the transform.

This is an interesting point of view. But yes, let's keep the readable/writable naming.

@tyoshino
Copy link
Member

An obvious objects-to-bytes use case is TextEncoder.

Ah, yeah! I didn't realize that well. So, given what we're trying to ship for the first time, TextDecoder and TextEncoder are asymmetric, I'm a little more convinced that we could include this in the spec tentatively.

@ricea ricea added byte streams do not merge yet Pull request must not be merged per rationale in comment labels Feb 15, 2018
Base automatically changed from master to main January 15, 2021 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
byte streams do not merge yet Pull request must not be merged per rationale in comment transform streams
Development

Successfully merging this pull request may close these issues.

4 participants