-
Notifications
You must be signed in to change notification settings - Fork 23.2k
Update dispatchEvent documentation for clarity #43521
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
Open
yuval-a
wants to merge
3
commits into
mdn:main
Choose a base branch
from
yuval-a:patch-3
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+1
−4
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This rewrite ends up comparing two different things - "calling" and "events".
I suggest we either keep the original phrasing ("Unlike "native" events...")
OR
break it down into two sentences and also change the voice to active to differentiate between browser and developer actions. ("when you call" is a subtle hint that you call this method manually)
What do you think about:
@Josh-Cena could you cross-check the technical accuracy of my suggestion here. Thanks!
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks right to me :) I would suggest:
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Josh-Cena - but that phrasing brings back the technical confusion/inaccuracy that this PR aims to correct from the first place.
Correct me if I'm wrong.
The difference to "native" - is that the call for the dispatch itself is async, but during it, event listeners run synchronously - don't go up the event loop.
The statement
makes it sound as if each of them is scheduled asynchronously on the event loop.
Interestingly, I tried to physically verify this by running some code in the console:
The microtask log does run between listeners, the timeout log only after both run.
When running the same principle with a manual
dispatchEventand a custom event - the microtask log only shows after both listeners run. In native - there is a microtask checkpoint between listeners. But, still - the listeners themselves doesn't run async, no separate async queuing for each...Or are they...? I mean, this test shows at least that no async timers run between (tried with other types of asyncs as well - same result) - my only doubt is that maybe they DO - and have a different "async priority" / are queued as a batch.
There is no place in the specs that explicitly says they run async in native, anyway.
What is the definite proof...?
EDIT: I'm learning to read the specs / distinguish between the terms, and it definitely says the handlers are "called" and not queued.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, now I'm not sure what the issue is claiming.
For a "native" event:
Imagine this:
The job queue:
When job 2 executes,
onclick1may schedule more jobs viasetTimeout,new Promise, etc. Becausenew Promisemakes a microtask with higher priority, when job 2 finishes, the next task the browser picks out will be the microtask instead of job 3, which has to wait a bit more. This is the speculation behavior you have observed.For
dispatchEvent:The job queue:
Nothing is allowed to execute between
onclick1()andonclick2()because it's one synchronous block.My acceptance of the original issue was that the word "asynchronous" is ambiguous in the context of native events because an event handler cannot observe its asynchronicity anyway, when by definition it has no relevant caller or callee. The key point here is that the event handler executes in a separate JavaScript job.
Now I look back at your suggestion, I don't think it makes a lot of sense either:
You are implying the following:
This would mean no other job is allowed to execute between
onclick1()andonclick2(). This is false.So I think there's something to be rewritten about this paragraph, but not your suggestion verbatim. Sorry I didn't read your suggestion in detail when I triaged the issue.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I understand, the mental model (for native events) is more like:
job 1: start internal event dispatch process, which are these steps:
https://dom.spec.whatwg.org/#dispatching-events
these include, for each of the 3 phases: capturing, target, bubbling:
cloning the event handlers registered on each EventTarget object along the "event path" - and calling them one-by-one (like in a for-loop. Specs say: "For each"):
https://dom.spec.whatwg.org/#concept-event-listener-inner-invoke
the key point/question here, again, is if they are called "asynchronously" (as in what JS developers usually expect when they see this term) or not;
They are called in what the specs define as "callback invocation"
https://webidl.spec.whatwg.org/#js-invoking-callback-functions
which, as I'm researching thus far, is not the same as what variations of "running asynchronously" usually mean. It is a specific mechanism, that includes cleanup and microtasks checkpoint after each, but not a "macrotask" checkpoint.
i.e. Promises/Microtasks queued from a listener will run when it's done, but any other "async macrotask" will only run after ALL listeners finished running.
I've seen all sorts of discussions surrounding this, and I also saw this:
whatwg/dom#1308
(A proposal for "asynchronous event listeners" - which I think strengthens my point that event listeners are not considered "asynchronous" in the common/usual way).
so, I think what's actually happening is something that's between the first model you describe in your comment and the second one (that you describe as what I was implying).
And in that model: queued microtasks will run in-between, queued macrotasks will not run in-between, only after.
The issue may be mostly "technical semantics";
I still think the current phrasing:
does not reflect what is technically happening; Event handlers do not go up the event loop the same way other "usual" async tasks usually do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the specs mention something that is supposed to be queued on the event loop - they specifically use the term "Queue a task" - linking to the part detailing the steps for it:
https://html.spec.whatwg.org/multipage/webappapis.html#queue-a-task
For event handlers they say they are called by callback invocation, linking to this:
https://webidl.spec.whatwg.org/#invoke-a-callback-function
and not by queuing a task.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's because in https://webidl.spec.whatwg.org/#call-a-user-objects-operation, when the callback finishes, the browser performs cleanup which includes clearing the microtask queue. This is analogous in every way (that I can think of) in userland to having a separate macrotask for every callback; it's just specified this way
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do they go up on the event loop? If not then at least "via the event loop" should be omitted :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that I read the spec, I actually don't think there's a big difference between
dispatchEventand the "fire an event" operation! So yes I think we should remove event loop and also try to rewrite it more significantly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're going through a similar rabbit hole I went through. I thought at first there is almost no difference.
But, did the experiment from a few comments back, comparing both:
two listeners, the first one schedules both a microtask and a macrotask (timeout), with logs.
In manual (calling
dispatchEvent): two listeners finished, then microtask, them macrotask.In native: one listener finished, microtask, listener 2 finished, macrotask.
So there was a significant difference :) dispatch seems to be 100% synchronous. Native... there's a microtask checkpoint between - so again I think the actual event handlers invocation is different in native - not synchronous, but not fully "async" as well.
In manual they all run sequentially 100% - nothing in between goes through until dispatch returns (it's a "chunk"), in native microtasks get through.