nodejs / whatwg-stream

WIP Official support for WHATWG Stream in Node.js
MIT License
37 stars 8 forks source link

Implement using web-streams-polyfill #3

Closed MattiasBuelens closed 3 years ago

MattiasBuelens commented 4 years ago

This implements the WHATWG Streams API using the ES2018 ponyfill build from web-streams-polyfill.

The WPT set up is based off of @joyeecheung's work in #2. Many thanks for that! πŸ˜„

To do list:

MattiasBuelens commented 4 years ago

Okay, the tests really don't like having the ponyfill come from a different VM. It breaks in all sorts of ways.

webidl2.js contains the following check:

let proto = this;
while (proto !== Object.prototype) {
  // ...
  proto = Object.getPrototypeOf(proto);
}

However, the "parent VM's" Object is injected into the test VM, so any instance of a class that's defined inside the test VM (like Constructor) does not have this particular Object.prototype in its prototype chain... I could work around this by checking for Object.getPrototypeOf(proto) !== null instead, but it feels weird.

Then there's the exposed_in function in idlharness.js, which checks what sort of environment we're running in to figure out which IDL definitions should be exposed. Of course, this function doesn't expect a Node-like environment, so it throws an error. Again, I could work around this by having it assume a worker-like environment, but it's yet another hack.

Finally, there are these tests in idlharness.js which check that an IDL object without a constructor cannot be called as a function or as a constructor. In that case, the VM throws an instance of its original version of TypeError, even if we have already replaced the global TypeError. This causes a test failure on this line, since e is an instance of the test VM's original TypeError, while constructor is the global TypeError that has been replaced with the parent VM's version. I really don't know how to fix this one: if I let the test VM keep its own original TypeError instead, then all of the other tests that deal with TypeErrors originating from the ponyfill (and thus coming from the parent VM) will fail... πŸ˜•


One way to fix these issues would be to load the ponyfill inside of the test VM, rather than loading it in the "parent VM" and copying it into the test VM's global scope. This would be closer to what the reference implementation does with WPT runner, but it would be very different from the existing Node WPT tests. I... don't really know how I feel about this. 🀷

targos commented 4 years ago

Hi @MattiasBuelens !

I hit the same kind of issues as you when I tried to update the WPT test harness in https://github.com/nodejs/node/pull/33770. I think I'm going to try and move the test execution to a child process instead of a VM instance.

MattiasBuelens commented 4 years ago

@targos Interesting! So instead of spinning up a VM and copying variables into its global scope, the WPT runner would spin up a new Node process and have it load all of the necessary code (e.g. load the library and set up the global variables) to run the test?

targos commented 4 years ago

Yes, exactly.

targos commented 4 years ago

I went for a Worker-based approach, which was easier to implement. PR: https://github.com/nodejs/node/pull/34796

MattiasBuelens commented 4 years ago

I see nodejs/node#34796 has landed recently. I'll have a look integrating the new test runner soonβ„’, and see if that improves things. πŸ™‚

Ethan-Arrowood commented 3 years ago

Hi @MattiasBuelens can I help you get this across the finish line?

Ethan-Arrowood commented 3 years ago

Thank you for pushing all of these updates! I'll review next week when I have some more time to go through it all. I'd also like to encourage @nodejs/whatwg-stream and @nodejs/undici to take a look here. Now that things likes EventTarget and AbortController are in core this is the next logical progression towards Fetch and overall WHATWG Stream support for Node πŸ˜„

mcollina commented 3 years ago

Given that this is based on http://npm.im/web-streams-polyfill, is having this repo still useful?

Are you running the WPT tests in http://npm.im/web-streams-polyfill?

ronag commented 3 years ago

This is a bit hard to review with so many files.

MattiasBuelens commented 3 years ago

@ronag Note that everything inside test/fixtures/wpt/ is copied directly from the web platform tests by git node wpt, and should not need review. See the explainer in test/fixtures/wpt/README.md.

MattiasBuelens commented 3 years ago

@mcollina Yes, the polyfill also runs the WPT tests.

The polyfill uses a different test runner though: wpt-runner (based on jsdom) instead of Node's own WPTRunner. So this PR demonstrates that we can integrate with Node's runner as well.

ronag commented 3 years ago

@ronag Note that everything inside test/fixtures/wpt/ is copied directly from the web platform tests by git node wpt, and should not need review. See the explainer in test/fixtures/wpt/README.md.

Yea, I get that but it's still difficult to review with the GitHub UI. Could you perhaps put all the functionality changes in one commit which we can review and the test fixtures in another one?

MattiasBuelens commented 3 years ago

@ronag I've squashed the history together in a few commits. All WPT fixtures are now added in one single commit, so you can skip that one.

mcollina commented 3 years ago

@joyeecheung @targos @benjamingr should we land this?

benjamingr commented 3 years ago

I'm in favour

targos commented 3 years ago

What's going to happen after we land? I don't remember the goal of this repository, sorry πŸ˜„

mcollina commented 3 years ago

provide a stopgap solution while we add them to core and bless something as our official implementation. I'll open an issue on nodejs/node.

targos commented 3 years ago

But if I understand correctly, this delegates the implementation to an existing module, so the stopgap solution already exists? I'm okay with that as a temporary thing but in the end we will not be able to integrate it like this in Node.js core (the code will have to be copied/adapted at some point).

mcollina commented 3 years ago

But if I understand correctly, this delegates the implementation to an existing module, so the stopgap solution already exists? I'm okay with that as a temporary thing but in the end we will not be able to integrate it like this in Node.js core (the code will have to be copied/adapted at some point).

Yes exactly.

MattiasBuelens commented 3 years ago

Now that Node has a built-in experimental implementation of web streams with nodejs/node#39062 (πŸŽ‰πŸŽ‰πŸŽ‰), I believe this PR and (by extension) this repository has become obsolete.

I suggest we close this PR and/or archive this repository. No hard feelings, on the contrary: I'm very happy that web streams made it into Node proper! πŸ˜„

jimmywarting commented 3 years ago

Woho good news!