-
Notifications
You must be signed in to change notification settings - Fork 161
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -379,23 +435,26 @@ class TransformStream { | |||
|
|||
this._transformStreamController = new TransformStreamDefaultController(this); | |||
|
|||
this._readableType = transformer.readableType; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
I tend to agree we should be conservative. |
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? |
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. |
An obvious objects-to-bytes use case is TextEncoder. |
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. |
Random observation: I find writableType/readableType confusing and would prefer to have inputType/outputType. |
Originally transform streams were { input, output } but people found this confusing :(. #174 |
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. |
This is an interesting point of view. But yes, let's keep the readable/writable naming. |
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. |
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