Skip to content
This repository has been archived by the owner on Jul 28, 2021. It is now read-only.

Let's do this #1

Open
mcollina opened this issue Nov 6, 2018 · 17 comments
Open

Let's do this #1

mcollina opened this issue Nov 6, 2018 · 17 comments

Comments

@mcollina
Copy link
Member

mcollina commented Nov 6, 2018

Hi Folks,

I would like to get this initiative kicked off. How would you like to work on this? Should we go forward and lift the implementation from Chromium? Or should we do it from scratch? Would you like to set up a quick meeting?

I pushed in some bits and bobs for our boilerplate, feel free to send PRs - I definitely did not put in all the needed documents.

Of course, if there are more people that would like to help with this, you are welcome to join!

@benjamingr
Copy link
Member

What about https://github.com/MattiasBuelens/web-streams-polyfill ? CC @MattiasBuelens

Some ideas:

Note that as far as I know ReadableStreams in Chrome don't have Symbol.asyncIterator yet. @devsnek was talking about adding support. If he doesn't have time it looks like a reasonably simple PR to make against ReadableStream.js

@ricea
Copy link

ricea commented Nov 6, 2018

@devsnek has a PR for Symbol.asyncIterator at whatwg/streams#954. There's also some in-progress tests linked from that PR. It shouldn't take much more to push it over the finish line.

You could probably do something similar to https://github.com/whatwg/streams/blob/master/reference-implementation/run-web-platform-tests.js to run the web platform tests.

@devsnek
Copy link
Member

devsnek commented Nov 6, 2018

if anyone wants to pick that spec change up, I'm probably not going to get around to it again until january.

@joyeecheung
Copy link
Member

joyeecheung commented Nov 6, 2018

If anybody is interested...it should be fairly trivial to reuse git node wpt (the thing that automates the update of a subset of WPT) and the WPT test runner I've come up with in nodejs/node#24035 in any usual Node.js project (like this one), and it may have benefit if the eventual goal is to get it landed in node core, or to have testing code shared across the "official" modules (provided that nodejs/node#24035 lands..). I could work on making them more general (I am guessing around < 30 lines of change anyway).

@domenic
Copy link

domenic commented Nov 6, 2018

We really should change all the streams tests to use .any.js. But, doing so would slow down our (the streams standard editors') development workflow unless we also fixed my https://github.com/domenic/wpt-runner project at the same time, since we use that for the whatwg/streams reference implementation development.

If someone has the time to do those things, then the tests would be in the .any.js format that are friendly to @joyeecheung's tooling.

@MattiasBuelens
Copy link

MattiasBuelens commented Nov 6, 2018

I can probably help with the wpt-runner changes. @domenic: can you open an issue in that project to support auto-generating the test boilerplate, or shall I open one? Just to keep the details out of this discussion.

If needed, I could also help finish the async iterator tests afterwards. But if someone else wants to continue that effort, that's totally fine as well. 🙂

@mcollina
Copy link
Member Author

mcollina commented Nov 7, 2018

Would you like to schedule a quick call in the next two weeks to talk about the details on making this happen? Otherwise, let's open a few issues and start cracking them.

Some ideas:

I agree with @benjamingr on the goals. We should really decide if we would like to write our own implementation, or start of from the Chrome one or the polyfill.

I did reserve the name and added nodejs-foundation. Let me know if somebody else would like to have the publish bit. Overall let's time for that.

@MattiasBuelens
Copy link

Refactored streams tests have landed in web-platform-tests/wpt#14172. Hopefully these are now easier to adopt in Node. 🙂

@balupton
Copy link

What's the latest?

@mcollina
Copy link
Member Author

No one has time/passion/money for building this apparently.

@MattiasBuelens
Copy link

I'd be willing to help, but I don't really know where we want to go with this. 🤷

I can give a quick status update on some of the previous ideas though:

Lift the WPT tests from https://github.com/web-platform-tests/wpt/tree/master/streams into this repo.

As explained in my previous comment, these tests are now in .any.js test format. If I understand correctly: since nodejs/node#24035 has landed, Node has the necessary infrastructure to run these tests?

Write our own implementation against those tests, use the Chrome implementation or use the polyfill. It looks like adapting the Chromium implementation shouldn't be too hard

I believe Chrome has replaced their initial JavaScript implementation with a C++ one. I don't know if Node would want to adopt a C++ implementation, or prefer a JavaScript one?

Other implementations which could be used as a starting point:

@mcollina
Copy link
Member Author

As explained in my previous comment, these tests are now in .any.js test format. If I understand correctly: since nodejs/node#24035 has landed, Node has the necessary infrastructure to run these tests?

I think so, maybe @joyeecheung can help.


Regarding the implementation of this spec, I think we can start by using your ponyfill. I guess the Chrome team moved the streams implementation to C++ for performance reasons: I think we might want to do that in the long run.
The current goal is compatibility, not perf.

I'll be happy to review a PR.

@benjamingr
Copy link
Member

I'm not sure I'm still +1 on this, I'm +0 or -0 on this. I am definitely +1 on experimenting and seeing what core whatwg-stream support looks like though.

I'm honestly not sure that people have been asking for this (web stream support) often enough for it to be worth the maintenance burden in core rather than in userland (especially given Deno's negative experience).

What about a blessed model based on the ponyfill (@nodejs/whatwg-stream) which core can run tests against?

I think it would make sense for Node utility methods to take whatwg streams if possible (as async iterables or otherwise) but I am personally not sure it makes sense in core given how complex streams are and given how hard maintaining streams has been traditionally in Node.js. This is true especially compared to alternative web APIs (like EventTarget, AbortController and URL) where the API is arguably a lot simpler and a lot easier to reason about.

@MattiasBuelens
Copy link

I'll be happy to review a PR.

I've started experimenting in #3. 👨‍🔬

I'm honestly not sure that people have been asking for this (web stream support) often enough for it to be worth the maintenance burden in core rather than in userland (especially given Deno's negative experience).

I agree, this should probably live in userland for now. Most browsers only implement a small part of the WHATWG streams spec, and even Chromium does not yet implement readable byte streams with BYOB readers. Maybe we should not even ship the entire ponyfill yet, but only what is supported by most browsers? 🤷

What about a blessed model based on the ponyfill (@nodejs/whatwg-stream) which core can run tests against?

That could work. It's sort of what I'm aiming for with #3: first make the ponyfill and tests work in this (separate) package, so we can later on more easily move the tests or the whole package into Node core (if we want).

I think it would make sense for Node utility methods to take whatwg streams if possible (as async iterables or otherwise)

Indeed, async iterables provide a nice compatibility interface between Node Readables and WHATWG ReadableStreams. Node already has Readable.from(), and a similar API is suggested for WHATWG streams in whatwg/streams#1018.

We could look into adding additional adapter methods for the other streams, like WritableStream.from(nodeWritable) or TransformStream.from(nodeTransform). Unfortunately, there's no common primitive (like async iterables) for these, so such adapter methods would not be part of the WHATWG Streams standard itself. 😞

@devsnek
Copy link
Member

devsnek commented Jul 8, 2021

now that core has whatwg streams, this repo can probably be decommissioned ye?

@jimmywarting
Copy link

I think so

@MattiasBuelens
Copy link

Yes, as I said earlier, this repo has become obsolete.

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

No branches or pull requests

9 participants