-
Notifications
You must be signed in to change notification settings - Fork 30k
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
lib: implement AbortSignal.any() #47821
Merged
nodejs-github-bot
merged 10 commits into
nodejs:main
from
atlowChemi:abortsignal-any-implemantation
May 18, 2023
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
0d8f9ff
lib: implement AbortSignal.any()
atlowChemi fff6b40
CR
atlowChemi f86f5bb
experimental
atlowChemi 2ab215a
add UT
atlowChemi db2c783
wip
atlowChemi 6086ef6
CR
atlowChemi e09e0ee
test: keep the event loop alive
panva b5be2a8
CR
atlowChemi a086465
test no sleep
atlowChemi 28f5b84
fix
atlowChemi 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 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
// META: script=./resources/abort-signal-any-tests.js | ||
|
||
abortSignalAnySignalOnlyTests(AbortSignal); | ||
abortSignalAnyTests(AbortSignal, AbortController); |
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.
I think this can be simplified to:
Since we already support weak listeners on event handlers inside core. You've effectively reimplemented parts of this in this PR and we can reuse the existing machinery.
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.
@benjamingr The implementation currently is spec compliant. do we want to implement it differently than the spec?
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.
@atlowChemi the spec talks about behavior not about how we implement the spec behavior. We can be spec compliant even if we don't follow the spec machinery word for word as long as we have the same external API and the same behavior.
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.
@benjamingr Got it, thanks for the explanation 🙂
That said, I tried implementing based on your suggestion, but it causes the execution order to change (which is breaking the tests - as it doesn't comply with spec). I was unable to figure this out, any suggestions?
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.
That's interesting and really surprising behavior for the signal
"abort"
to not be in "listener added order. We can work around it but I wonder if there is context regarding why they did this.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.
Worst case this is just event listener order and not hard to work around but I wanna understand why
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.
See spec discussion in whatwg/dom#1152 (comment)
Just for completeness the way to work around it with my suggestion implementation without absorbing the additional complexity is to add the ability to tag event listeners as "run first" perhaps in an EventTarget subclass.
I generally think it's better to fix it in the spec but to be absolutely clear I feel strongly our behavior should be aligned with the spec as that was a big part of the motivation to implement AbortSignal and web standard more generally and diverging would defeat the purpose.
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.
@benjamingr I am not fully sure I follow the discussion in
whatwg
... As far as I understood, concerns were raised regarding usingaddEventListener
related to user-land being able to callstopImmediatePropagation
.Were does this suggestion stand currently?
Should it be implemented?
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.
We patiently wait (or engage in the discussion) until @shaseley and @annevk decide and then follow what they decide. I've raised my concerns there but I feel strongly we should follow whatever consensus that PR concludes with and the correct spec.