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

Implement using web-streams-polyfill #3

Closed
wants to merge 5 commits into from

Conversation

MattiasBuelens
Copy link

@MattiasBuelens MattiasBuelens commented Jul 23, 2020

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:

  • The Node WPT runner starts running all the test scripts immediately. Since most of the streams tests are asynchronous (using promise_test), this means that tests can interfere with each other. This is especially bad for the "patched globals" tests, which will literally break the API during the test.
    • I'm thinking we should make the runner execute tests sequentially, waiting for each test to complete before starting the next one? Concretely, this would mean we wait until completionCallback is called before we start the next test. I'll try implementing it here first, to see if it works. Hopefully we can upstream that afterwards? 🙏
    • ✅ Fixed. The new test driver now runs each test in a separate worker, so they can no longer interfere with each other.
  • I need to copy a lot of global variables into the test runner. This is because the ponyfill gets loaded inside the original VM context, and uses globals such as Promise or Uint8Array from there. However, the tests are run inside a separate VM context, so when it tries to compare whether e.g. ReadableStreamBYOBRequest.view is a Uint8Array, it's using a different global for Uint8Array than the polyfill.
    • Is this the right approach? Or should we look for a way to e.g. load the ponyfill inside of the test VM context, so it uses that context's globals? It would be quite weird, though...
    • While most globals can be copied into the runner, some things just cannot be patched. For example, there's a test that checks if ReadableStream[Symbol.asyncIterator]() has %AsyncIteratorPrototype% as its prototype. As far as I know, there's no way to replace that...
    • ✅ Fixed. The ponyfill is loaded into the worker's global scope at the start of each test, and uses the worker's global variables. No more copying global variables.
  • ReadableStream.pipeTo() uses an AbortSignal. Currently, I'm using a separate polyfill for that.
  • Similarly, ReadableStream.pipeTo() needs to reject with a DOMException when it is aborted using a signal, which is also tested by WPT.
    • While there is already an implementation in Node, it doesn't look like this is exposed globally. I suppose that if this streams implementation were to be added to Node itself, we could access it through an internal binding like with the URL tests?
    • ✅ Fixed. With --expose-internals, I can retrieve the DOMException implementation from Node itself. It's even given as an example in the test driver's README! 😅

@MattiasBuelens MattiasBuelens mentioned this pull request Jul 24, 2020
@MattiasBuelens
Copy link
Author

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
Copy link
Member

targos commented Jul 26, 2020

Hi @MattiasBuelens !

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

@MattiasBuelens
Copy link
Author

@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
Copy link
Member

targos commented Jul 27, 2020

Yes, exactly.

@targos
Copy link
Member

targos commented Aug 16, 2020

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

@MattiasBuelens
Copy link
Author

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. 🙂

Copy link
Author

@MattiasBuelens MattiasBuelens left a comment

Choose a reason for hiding this comment

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

Progress! With the new WPT test runner (and only a few hacks), I can get all relevant tests to pass. 🎉

The polyfill still needs to be updated to handle one of the more recent spec changes. After that, I'll run git node wpt streams again to get the latest WPT streams tests as well.

test/common/wpt.js Outdated Show resolved Hide resolved
// HACK: Pretend that our JavaScript shell exposes worker interfaces.
// The streams interfaces are currently only exposed for windows, workers and worklets
// and there's no such thing as [Exposed=Shell] in Web IDL yet.
return globals.has("DedicatedWorker");
Copy link
Author

Choose a reason for hiding this comment

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

Better suggestions are welcome. 😁

@Ethan-Arrowood
Copy link

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

Copy link
Author

@MattiasBuelens MattiasBuelens left a comment

Choose a reason for hiding this comment

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

@Ethan-Arrowood Sorry, this has been lingering for a while. I didn't get much feedback, so I thought people weren't very interested. 🤷‍♂️

Either way, I've just updated the PR to the latest version of the polyfill and the WPT tests. Have a look!

@@ -0,0 +1,14 @@
{
"piping/pipe-through.any.js": {
"fail": "Node does not perform a brand check for AbortSignal.prototype.aborted"
Copy link
Author

Choose a reason for hiding this comment

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

This test checks whether passing a "bad" AbortSignal (in particular ) causes pipeThrough() to throw. The polyfill implements this by trying to retrieve AbortSignal.prototype.aborted and checking that it does not throw. WebIDL requires this attribute to perform a brand check, so a compliant implementation should throw a TypeError when this getter is run on such a "bad" AbortSignal.

Unfortunately, Node's AbortSignal implementation does not throw an error when AbortSignal.prototype.aborted is run such a signal. Instead, it will return false and the polyfill will consider this to be a valid AbortSignal.

I'm not sure what we want to do here. Should we fix Node to perform a brand check in get aborted (i.e. throw an error if this[kAborted] does not exist)? Or should we change how the polyfill does this check (although I'm not sure how)?

Choose a reason for hiding this comment

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

If I understand you correctly, I think the best course of action is to make an upstream change to Node.js core AbortSignal so that it aligns more with the spec.

Copy link
Author

@MattiasBuelens MattiasBuelens Mar 11, 2021

Choose a reason for hiding this comment

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

On it! 🙂

EDIT: Opened nodejs/node#37720.

@MattiasBuelens MattiasBuelens marked this pull request as ready for review March 10, 2021 23:32
@Ethan-Arrowood
Copy link

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
Copy link
Member

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?

test/wpt/test.js Outdated Show resolved Hide resolved
@ronag
Copy link
Member

ronag commented Mar 11, 2021

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

@MattiasBuelens
Copy link
Author

@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
Copy link
Author

@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
Copy link
Member

ronag commented Mar 11, 2021

@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
Copy link
Author

@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.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina
Copy link
Member

@joyeecheung @targos @benjamingr should we land this?

@benjamingr
Copy link
Member

I'm in favour

@targos
Copy link
Member

targos commented Mar 15, 2021

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

@mcollina
Copy link
Member

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
Copy link
Member

targos commented Mar 15, 2021

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
Copy link
Member

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
Copy link
Author

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
Copy link

Woho good news!

@ronag ronag mentioned this pull request Aug 4, 2021
39 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants