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

events: graduate Event, EventTarget, AbortController #35949

Closed
wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Nov 3, 2020

Graduate these from experimental status

Signed-off-by: James M Snell [email protected]

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@jasnell jasnell force-pushed the graduate-eventtarget branch 2 times, most recently from 14eabcc to 289d6cf Compare November 3, 2020 20:31
@jasnell jasnell added eventtarget Issues and PRs related to the EventTarget implementation. request-ci Add this label to start a Jenkins CI on a PR. semver-minor PRs that contain new features and should be released in the next minor version. labels Nov 3, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 3, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 4, 2020
@joyeecheung
Copy link
Member

Have we tested them with WPT?

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

The actual code improved a lot and I think like it's almost ready - but I'd like to ping @annevk and @domenic and ask for guidance regarding anything else we might need to do before we graduate them.

@benjamingr
Copy link
Member

@joyeecheung the problem is the WPT for EventTarget are using Nodes and not EventTargets so they ned to be ported manually - we've done a few of these and manually ported others (e.g. #33621 and a few others).

@benjamingr
Copy link
Member

@joyeecheung I did however, go test-by-test over the WPT, Deno's EventTarget tests, JSDom's and userland tests and our event target is mostly correct (our AbortSignal still needs #35931 but that's relatively minor and I wouldn't block solely on that since it's relatively minor)

@domenic
Copy link
Contributor

domenic commented Nov 5, 2020

I don't feel qualified to make a judgment about the Node.js project's graduation guidance. Web platform tests seem like the right criteria in general, and I'm glad to hear that despite them not being easily runnable, @benjamingr has been keeping up to date on running them.

I noticed when skimming #35931 that your event handler implementation doesn't implement the full complexities of the spec: https://html.spec.whatwg.org/#event-handler-idl-attributes . In particular the activate/deactivate semantics. (Thankfully you can ignore all the ridiculousness in https://html.spec.whatwg.org/#getting-the-current-value-of-the-event-handler since you don't have HTML content attributes, i.e. "internal raw uncompiled handlers".) The note and example below https://html.spec.whatwg.org/#event-handler-event-type discuss the consequences of the spec's complexity, which I think you would be missing with the current implementation.

Also onmessage on MessagePort is special and has its own side effects, which I don't believe are implemented based on skimming that PR.

@benjamingr
Copy link
Member

Thanks Domenic!

First to clarify a point:

Also onmessage on MessagePort is special and has its own side effects, which I don't believe are implemented based on skimming that PR.

AFAIK workers are not Web Workers and our MessagePort is not a WHATWG message port (yet! Anna is working towards some level of compatibility - @addaleax feel free to correct me here). This is only about graduating EventTarget and AbortController.

I noticed when skimming #35931 that your event handler implementation doesn't implement the full complexities of the spec: https://html.spec.whatwg.org/#event-handler-idl-attributes . In particular the activate/deactivate semantics.

We only recently moved it from a simple onabort property on the object to use the same event handler order so that the order is correct.

Any help in understanding parts we might have missed would be appreciated.

(Our only EventTarget at the moment is AbortController which to the best of my understanding doesn't have activation behaviour. Eventually our MessagePort will be spec compliant, hopefully)

since you don't have HTML content attributes, i.e. "internal raw uncompiled handlers".

Apologies for the possibly dumb question - but is that for example the onclick in <div onclick="someFunction()"> ?

(If so, I believe (hope)indeed we don't have those)

@addaleax
Copy link
Member

addaleax commented Nov 5, 2020

Also onmessage on MessagePort is special and has its own side effects, which I don't believe are implemented based on skimming that PR.

AFAIK workers are not Web Workers and our MessagePort is not a WHATWG message port (yet! Anna is working towards some level of compatibility - @addaleax feel free to correct me here). This is only about graduating EventTarget and AbortController.

Any incompatibility with the HTML spec would be considered a bug here, and MessagePorts are considered a stable API at this point. If the “own side effects” that are being referred to here are about starting the flow of messages when the listener is added, then yes, we implement that.

@benjamingr
Copy link
Member

@addaleax that's great, I thought we were making progress towards compatibility but I didn't think Node actually implements the WHATWG MessagePort spec for message ports. I recall reviewing a bunch of stuff that helps with that but I didn't make the connection.

Props and awesome! 🎉 🙏

@domenic
Copy link
Contributor

domenic commented Nov 5, 2020

We only recently moved it from a simple onabort property on the object to use the same event handler order so that the order is correct. Any help in understanding parts we might have missed would be appreciated.

Well, it might the case that you've just implemented a very concise version of the algorithms at https://html.spec.whatwg.org/#event-handler-idl-attributes (including the linked "deactivate an event handler" and "activate an event handler"). That is, looking at the code it doesn't seem to match up to the spec steps very much, but maybe it has the same observable consequences. I guess the test would be the following:

const { port1: mp } = new MessageChannel();
mp.addEventListener("message", () => console.log("1"));
mp.onmessage = () => console.log("not called");
mp.addEventListener("message", () => console.log("3"));
mp.onmessage = () => console.log("2");
mp.addEventListener("message", () => console.log("4"));

mp.dispatchEvent(new Event("message"));

which should log 1 / 2 / 3 / 4.

Apologies for the possibly dumb question - but is that for example the onclick in <div onclick="someFunction()"> ?

Yes, exactly. Those are a nighmare of complexity :).

@addaleax
Copy link
Member

addaleax commented Nov 5, 2020

which should log 1 / 2 / 3 / 4.

It does log 1 / 3 / 2 / 4, so yes, I assume that’s still a bug here. But this isn’t specific to onmessage, right, it’s a problem for all EventTarget?

@benjamingr
Copy link
Member

benjamingr commented Nov 5, 2020

It does log 1 / 3 / 2 / 4, so yes, I assume that’s still a bug here. But this isn’t specific to onmessage, right, it’s a problem for all EventTarget?

That sounds like a bug, I think as it's a timing bug it's a big enough to block graduation and I think I just misunderstood the spec at that point.

@addaleax would you mind if I pushed the fix for it + a test to #35931 instead of opening a new PR since it already touches that code ?

It's a pretty simple fix.

(If you prefer a new PR - happy to do that)


Also, I had no idea it worked that way :O

@addaleax
Copy link
Member

addaleax commented Nov 5, 2020

@benjamingr Perfectly fine with me :)

@benjamingr
Copy link
Member

benjamingr commented Nov 5, 2020

Ok, I'll push a fix tomorrow morning and add a test. I have misunderstood the spec and didn't realize our test (that just checks the order between event handlers and event listeners without overriding the handler) was sufficient.

Edit: fix pushed

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

WPT tests are in and the prototype issue on the handlers was fixed as well as everything else I could find or think of.

Ideally #36001 should land as well but that's a DX issue and not a correctness issue and shouldn't block this.

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Needs a rebase but LGTM

@Trott
Copy link
Member

Trott commented Nov 8, 2020

@nodejs/tsc Seems like this is a big enough deal that more TSC eyes would be good.

@benjamingr
Copy link
Member

@jasnell is this still waiting for anything atm?

Graduate these from experimental status

Signed-off-by: James M Snell <[email protected]>
jasnell added a commit that referenced this pull request Nov 23, 2020
Graduate these from experimental status

Signed-off-by: James M Snell <[email protected]>

PR-URL: #35949
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@jasnell
Copy link
Member Author

jasnell commented Nov 23, 2020

Landed in 513764b

@jasnell jasnell closed this Nov 23, 2020
danielleadams pushed a commit that referenced this pull request Dec 7, 2020
Graduate these from experimental status

Signed-off-by: James M Snell <[email protected]>

PR-URL: #35949
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@danielleadams danielleadams mentioned this pull request Dec 7, 2020
danielleadams added a commit that referenced this pull request Dec 7, 2020
PR-URL: #36435

Notable changes:

* child_processes:
  * add AbortSignal support (Benjamin Gruenbaum) (#36308)
* deps:
  * update ICU to 68.1 (Michaël Zasso) (#36187)
* events:
  * support signal in EventTarget (Benjamin Gruenbaum) (#36258)
  * graduate Event, EventTarget, AbortController (James M Snell) (#35949)
* http:
  * enable call chaining with setHeader() (pooja d.p) (#35924)
* module:
  * add isPreloading indicator (James M Snell) (#36263)
* stream:
  * support abort signal (Benjamin Gruenbaum) (#36061)
  * add FileHandle support to Read/WriteStream (Momtchil Momtchev) (#35922)
* worker:
  * add experimental BroadcastChannel (James M Snell) (#36271)
danielleadams added a commit that referenced this pull request Dec 9, 2020
PR-URL: #36435

Notable changes:

* child_processes:
  * add AbortSignal support (Benjamin Gruenbaum) (#36308)
* deps:
  * update ICU to 68.1 (Michaël Zasso) (#36187)
* events:
  * support signal in EventTarget (Benjamin Gruenbaum) (#36258)
  * graduate Event, EventTarget, AbortController (James M Snell) (#35949)
* http:
  * enable call chaining with setHeader() (pooja d.p) (#35924)
* module:
  * add isPreloading indicator (James M Snell) (#36263)
* stream:
  * support abort signal (Benjamin Gruenbaum) (#36061)
  * add FileHandle support to Read/WriteStream (Momtchil Momtchev) (#35922)
* worker:
  * add experimental BroadcastChannel (James M Snell) (#36271)
danielleadams added a commit that referenced this pull request Dec 9, 2020
PR-URL: #36435

Notable changes:

* child_processes:
  * add AbortSignal support (Benjamin Gruenbaum) (#36308)
* deps:
  * update ICU to 68.1 (Michaël Zasso) (#36187)
* events:
  * support signal in EventTarget (Benjamin Gruenbaum) (#36258)
  * graduate Event, EventTarget, AbortController (James M Snell) (#35949)
* http:
  * enable call chaining with setHeader() (pooja d.p) (#35924)
* module:
  * add isPreloading indicator (James M Snell) (#36263)
* stream:
  * support abort signal (Benjamin Gruenbaum) (#36061)
  * add FileHandle support to Read/WriteStream (Momtchil Momtchev) (#35922)
* worker:
  * add experimental BroadcastChannel (James M Snell) (#36271)
@targos targos added the baking-for-lts PRs that need to wait before landing in a LTS release. label May 30, 2021
@targos targos added dont-land-on-v12.x and removed baking-for-lts PRs that need to wait before landing in a LTS release. labels Aug 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. eventtarget Issues and PRs related to the EventTarget implementation. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants