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

Rewrite TransformStream using the public API #105

Merged
merged 8 commits into from
Nov 17, 2021

Conversation

MattiasBuelens
Copy link
Owner

@MattiasBuelens MattiasBuelens commented Nov 17, 2021

This re-implements TransformStream to use the public API of ReadableStream and WritableStream, rather than using internal abstract ops. The idea is that, in the future, we could construct a polyfill TransformStream on top of a native ReadableStream and/or WritableStream.

This also adds partial support for WritableStreamDefaultController.signal.reason, which we use to detect when writable.abort(reason) is called. We do have to "cheat" a bit here, since AbortSignal.reason may not yet be supported everywhere and web-streams-polyfill does not attempt to polyfill this API by itself.

@MattiasBuelens MattiasBuelens added this to the v4.0.0 milestone Nov 17, 2021
@MattiasBuelens MattiasBuelens self-assigned this Nov 17, 2021
@jimmywarting
Copy link

I'm seeing all this _ prefix everywhere all over the pr and in all the files... why don't you just use # instead?

@MattiasBuelens
Copy link
Owner Author

I still ship a build for ES5, so I have to be very careful with what syntax features I can or cannot use. The down-level emit for private fields uses WeakMap, and I'm not 100% sure if that's safe to use everywhere... 😬

I could move everything into a semi-private Symbol("state") or something (like in Node), and replace Symbol with String on platforms that don't support it. Not sure yet. 😛

@MattiasBuelens MattiasBuelens merged commit 4f231a4 into next Nov 17, 2021
@MattiasBuelens MattiasBuelens deleted the rewrite-transform-stream branch November 17, 2021 21:06
@jimmywarting
Copy link

I'm not sure if typescript can handle that down leveling for you, don't use typescript...
but i prefer the god old _ prefix over symbol cuz it's cleaner code.

@MattiasBuelens
Copy link
Owner Author

MattiasBuelens commented Nov 17, 2021

I could throw Babel into the mix and use the loose option instead. That changes the emit to generate a unique property name for the private field instead, which would work on ES5. See demo.

It would make the brand checks nicer though, since you can do #privateField in this instead and be guaranteed that this was constructed by your class. See docs. This ties in with #70 (comment).

Anyway, I'll leave that for another time. 😛

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

Successfully merging this pull request may close these issues.

2 participants