Implement using web-streams-polyfill #3

MattiasBuelens · 2020-07-23T23:43:18Z

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.
- ~~It looks like Node has recently added an experimental implementation for this in lib: initial experimental AbortController implementation node#33527? Could be interesting.~~
- ✅ Fixed. Now using the built-in AbortSignal from Node 15! 😁
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 · 2020-07-24T22:48:11Z

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 · 2020-07-26T13:02:32Z

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 · 2020-07-26T22:43:09Z

@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 · 2020-07-27T07:29:43Z

Yes, exactly.

targos · 2020-08-16T13:31:43Z

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

MattiasBuelens · 2020-08-25T09:30:59Z

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

MattiasBuelens

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

MattiasBuelens · 2020-08-26T22:18:16Z

test/fixtures/wpt/resources/idlharness.js

+        // 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");


Better suggestions are welcome. 😁

Ethan-Arrowood · 2021-03-09T05:20:21Z

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

MattiasBuelens

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

MattiasBuelens · 2021-03-10T23:17:57Z

test/wpt/status/streams.json

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


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)?

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.

On it! 🙂

EDIT: Opened nodejs/node#37720.

Ethan-Arrowood · 2021-03-11T18:15:58Z

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 · 2021-03-11T18:30:23Z

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

ronag · 2021-03-11T21:24:29Z

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

MattiasBuelens · 2021-03-11T21:42:23Z

@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 · 2021-03-11T23:21:55Z

@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 · 2021-03-11T23:34:28Z

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

git node wpt streams git node wpt resources git node wpt interfaces git node wpt common

MattiasBuelens · 2021-03-12T00:15:06Z

@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

lgtm

mcollina · 2021-03-14T09:54:00Z

@joyeecheung @targos @benjamingr should we land this?

benjamingr · 2021-03-14T15:32:21Z

I'm in favour

targos · 2021-03-15T07:37:52Z

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

mcollina · 2021-03-15T08:12:03Z

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 · 2021-03-15T14:18:41Z

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 · 2021-03-15T14:30:42Z

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 · 2021-06-30T20:43:46Z

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 · 2021-06-30T21:16:57Z

Woho good news!

MattiasBuelens mentioned this pull request Jul 24, 2020

Let's do this #1

Open

MattiasBuelens commented Aug 26, 2020

View reviewed changes

MattiasBuelens commented Mar 10, 2021

View reviewed changes

MattiasBuelens marked this pull request as ready for review March 10, 2021 23:32

aduh95 reviewed Mar 11, 2021

View reviewed changes

test/wpt/test.js Outdated Show resolved Hide resolved

MattiasBuelens mentioned this pull request Mar 11, 2021

lib: add brand checks to AbortController and AbortSignal nodejs/node#37720

Closed

MattiasBuelens added 5 commits March 12, 2021 00:57

Implement using web-streams-polyfill

8042b06

Add WPT fixtures

40a80f0

git node wpt streams git node wpt resources git node wpt interfaces git node wpt common

Hack idlharness to use worker interfaces when running in our JS shell

0247851

Add WPT runner

770a58d

Set up tests

35cb5db

MattiasBuelens force-pushed the implementation branch from e16f846 to 35cb5db Compare March 12, 2021 00:13

Ethan-Arrowood approved these changes Mar 12, 2021

View reviewed changes

mcollina approved these changes Mar 12, 2021

View reviewed changes

targos approved these changes Mar 15, 2021

View reviewed changes

jimmywarting mentioned this pull request Apr 21, 2021

Please consider web compatible stream() method. node-fetch/fetch-blob#51

Closed

MattiasBuelens closed this Jul 27, 2021

ronag mentioned this pull request Aug 4, 2021

Fetch nodejs/undici#915

Closed

39 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement using web-streams-polyfill #3

Implement using web-streams-polyfill #3

MattiasBuelens commented Jul 23, 2020 •

edited

MattiasBuelens commented Jul 24, 2020

targos commented Jul 26, 2020

MattiasBuelens commented Jul 26, 2020

targos commented Jul 27, 2020

targos commented Aug 16, 2020

MattiasBuelens commented Aug 25, 2020

MattiasBuelens left a comment

MattiasBuelens Aug 26, 2020

Ethan-Arrowood commented Mar 9, 2021

MattiasBuelens left a comment

MattiasBuelens Mar 10, 2021

Ethan-Arrowood Mar 11, 2021

MattiasBuelens Mar 11, 2021 •

edited

Ethan-Arrowood commented Mar 11, 2021

mcollina commented Mar 11, 2021

ronag commented Mar 11, 2021

MattiasBuelens commented Mar 11, 2021

MattiasBuelens commented Mar 11, 2021

ronag commented Mar 11, 2021

MattiasBuelens commented Mar 12, 2021

mcollina left a comment

mcollina commented Mar 14, 2021

benjamingr commented Mar 14, 2021

targos commented Mar 15, 2021

mcollina commented Mar 15, 2021

targos commented Mar 15, 2021

mcollina commented Mar 15, 2021

MattiasBuelens commented Jun 30, 2021

jimmywarting commented Jun 30, 2021

Implement using web-streams-polyfill #3

Implement using web-streams-polyfill #3

Conversation

MattiasBuelens commented Jul 23, 2020 • edited

MattiasBuelens commented Jul 24, 2020

targos commented Jul 26, 2020

MattiasBuelens commented Jul 26, 2020

targos commented Jul 27, 2020

targos commented Aug 16, 2020

MattiasBuelens commented Aug 25, 2020

MattiasBuelens left a comment

Choose a reason for hiding this comment

MattiasBuelens Aug 26, 2020

Choose a reason for hiding this comment

Ethan-Arrowood commented Mar 9, 2021

MattiasBuelens left a comment

Choose a reason for hiding this comment

MattiasBuelens Mar 10, 2021

Choose a reason for hiding this comment

Ethan-Arrowood Mar 11, 2021

Choose a reason for hiding this comment

MattiasBuelens Mar 11, 2021 • edited

Choose a reason for hiding this comment

Ethan-Arrowood commented Mar 11, 2021

mcollina commented Mar 11, 2021

ronag commented Mar 11, 2021

MattiasBuelens commented Mar 11, 2021

MattiasBuelens commented Mar 11, 2021

ronag commented Mar 11, 2021

MattiasBuelens commented Mar 12, 2021

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Mar 14, 2021

benjamingr commented Mar 14, 2021

targos commented Mar 15, 2021

mcollina commented Mar 15, 2021

targos commented Mar 15, 2021

mcollina commented Mar 15, 2021

MattiasBuelens commented Jun 30, 2021

jimmywarting commented Jun 30, 2021

MattiasBuelens commented Jul 23, 2020 •

edited

MattiasBuelens Mar 11, 2021 •

edited