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

Add AbortSignal.aborted() static #960

Merged
merged 5 commits into from
Mar 12, 2021

Conversation

jasnell
Copy link
Contributor

@jasnell jasnell commented Mar 9, 2021

Returns an already aborted AbortSignal per #959

Fixes: #959

(See WHATWG Working Mode: Changes for more details.)


Preview | Diff

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

This looks great, thanks for working on this James!

@josepharhar @smaug---- @mfreed7 @rniwa thoughts on adding this? Seems simple enough and there's precedent with promises (Promise.reject()). Though maybe that means this should be AbortSignal.abort()?

dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
@annevk annevk added the needs tests Moving the issue forward requires someone to write tests label Mar 10, 2021
@jasnell jasnell force-pushed the abortsignal-aborted-static branch 2 times, most recently from 0559b7a to 089887c Compare March 10, 2021 16:16
jasnell added a commit to jasnell/wpt that referenced this pull request Mar 10, 2021
@jasnell
Copy link
Contributor Author

jasnell commented Mar 10, 2021

PR to add test here: web-platform-tests/wpt#28003
PR to add impl to Node.js: nodejs/node#37693

jasnell added a commit to jasnell/node that referenced this pull request Mar 10, 2021
Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good to me. Mozilla would be fine with adding this.

dom.bs Outdated Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
@annevk annevk added needs implementer interest Moving the issue forward requires implementers to express interest and removed needs tests Moving the issue forward requires someone to write tests labels Mar 11, 2021
jasnell added a commit to jasnell/node that referenced this pull request Mar 11, 2021
@josepharhar
Copy link
Contributor

lgtm, this looks particularly easy to implement :)

@annevk
Copy link
Member

annevk commented Mar 11, 2021

@jasnell thanks! You or your employer will need to sign https://participate.whatwg.org/agreement to appease the IPR bot.

It seems we're all done here apart from that. @benjamingr @jakearchibald @domenic @MattiasBuelens any final thoughts?

@jasnell
Copy link
Contributor Author

jasnell commented Mar 11, 2021

You or your employer will need to sign participate.whatwg.org/agreement to appease the IPR bot.

Done!

@domenic
Copy link
Member

domenic commented Mar 11, 2021

LGTM. I still vaguely prefer the aborted() name but I can bow to the consistency argument with Promise.reject() and Promise.resolve().

@benjamingr
Copy link
Member

This generally looks fine to me and makes sense, though bike-shedding, I'm not sure I like the name.

@mfreed7
Copy link
Contributor

mfreed7 commented Mar 11, 2021

lgtm, this looks particularly easy to implement :)

rsLGTM based on @josepharhar's LGTM. Thanks!

@annevk annevk added topic: aborting AbortController and AbortSignal and removed needs implementer interest Moving the issue forward requires implementers to express interest labels Mar 12, 2021
@annevk
Copy link
Member

annevk commented Mar 12, 2021

@jasnell do you want to file the bug against Firefox?

annevk pushed a commit to web-platform-tests/wpt that referenced this pull request Mar 12, 2021
dom.bs Show resolved Hide resolved
dom.bs Outdated Show resolved Hide resolved
@annevk annevk merged commit aa384af into whatwg:main Mar 12, 2021
@jasnell
Copy link
Contributor Author

jasnell commented Mar 12, 2021

Yeah I'll do so this morning!

@annevk
Copy link
Member

annevk commented Mar 15, 2021

I ended up filing https://bugzilla.mozilla.org/show_bug.cgi?id=1698468.

Thanks everyone for the quick turnaround! Hopefully it'll get implemented in a similar pace. 😊

@jasnell
Copy link
Contributor Author

jasnell commented Mar 15, 2021

Thanks @annevk ... Sorry I had missed my reminder notification to do that! I appreciate it!

jasnell added a commit to nodejs/node that referenced this pull request Mar 15, 2021
Refs: whatwg/dom#960

PR-URL: #37693
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
jasnell added a commit to nodejs/node that referenced this pull request Mar 15, 2021
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #37693
Refs: whatwg/dom#960
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
danielleadams pushed a commit to nodejs/node that referenced this pull request Mar 16, 2021
Refs: whatwg/dom#960

PR-URL: #37693
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
danielleadams pushed a commit to nodejs/node that referenced this pull request Mar 16, 2021
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #37693
Refs: whatwg/dom#960
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
danielleadams pushed a commit to nodejs/node that referenced this pull request Mar 16, 2021
Refs: whatwg/dom#960

PR-URL: #37693
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
danielleadams pushed a commit to nodejs/node that referenced this pull request Mar 16, 2021
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #37693
Refs: whatwg/dom#960
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@benjamingr
Copy link
Member

Does this need an "impacts documentation" label?

@annevk annevk added the impacts documentation Used by documentation communities, such as MDN, to track changes that impact documentation label Mar 17, 2021
@annevk
Copy link
Member

annevk commented Mar 17, 2021

cc @whatwg/documentation

@sideshowbarker
Copy link
Contributor

sideshowbarker commented Mar 18, 2021

I’ll make time to write up MDN documentation for this some time this month (unless somebody gets to it before I manage to)

manekinekko added a commit to manekinekko/node that referenced this pull request Mar 18, 2021
manekinekko added a commit to manekinekko/node that referenced this pull request Mar 18, 2021
aduh95 pushed a commit to manekinekko/node that referenced this pull request Mar 20, 2021
PR-URL: nodejs#37798
Refs: whatwg/dom#960
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
webkit-commit-queue pushed a commit to WebKit/WebKit that referenced this pull request Mar 22, 2021
https://bugs.webkit.org/show_bug.cgi?id=223071
<rdar://problem/75575483>

Reviewed by Darin Adler.

LayoutTests/imported/w3c:

Resync WPT test from upstream a8cbe9c to gain test coverage.

* web-platform-tests/dom/abort/event.any-expected.txt:
* web-platform-tests/dom/abort/event.any.js:
* web-platform-tests/dom/abort/event.any.worker-expected.txt:

Source/WebCore:

Implement AbortSignal.abort() which creates and returns an already aborted AbortSignal:
- whatwg/dom#960
- web-platform-tests/wpt#28003

No new tests, covered by updated test.

* dom/AbortSignal.cpp:
(WebCore::AbortSignal::createAborted):
(WebCore::AbortSignal::AbortSignal):
* dom/AbortSignal.h:
* dom/AbortSignal.idl:


Canonical link: https://commits.webkit.org/235582@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@274773 268f45cc-cd09-0410-ab3c-d52691b4dbfc
ruyadorno pushed a commit to nodejs/node that referenced this pull request Mar 24, 2021
PR-URL: #37798
Refs: whatwg/dom#960
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Zijian Liu <lxxyxzj@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Cwiiis pushed a commit to Cwiiis/webkit-deprecated that referenced this pull request Mar 24, 2021
https://bugs.webkit.org/show_bug.cgi?id=223071
<rdar://problem/75575483>

Reviewed by Darin Adler.

LayoutTests/imported/w3c:

Resync WPT test from upstream a8cbe9c712ad032d36 to gain test coverage.

* web-platform-tests/dom/abort/event.any-expected.txt:
* web-platform-tests/dom/abort/event.any.js:
* web-platform-tests/dom/abort/event.any.worker-expected.txt:

Source/WebCore:

Implement AbortSignal.abort() which creates and returns an already aborted AbortSignal:
- whatwg/dom#960
- web-platform-tests/wpt#28003

No new tests, covered by updated test.

* dom/AbortSignal.cpp:
(WebCore::AbortSignal::createAborted):
(WebCore::AbortSignal::AbortSignal):
* dom/AbortSignal.h:
* dom/AbortSignal.idl:


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@274773 268f45cc-cd09-0410-ab3c-d52691b4dbfc
targos pushed a commit to targos/node that referenced this pull request Apr 24, 2021
Refs: whatwg/dom#960

PR-URL: nodejs#37693
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Apr 24, 2021
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#37693
Refs: whatwg/dom#960
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Apr 26, 2021
Refs: whatwg/dom#960

PR-URL: nodejs#37693
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Apr 26, 2021
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#37693
Refs: whatwg/dom#960
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Apr 30, 2021
Refs: whatwg/dom#960

PR-URL: nodejs#37693
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Apr 30, 2021
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#37693
Refs: whatwg/dom#960
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos pushed a commit to nodejs/node that referenced this pull request Apr 30, 2021
Refs: whatwg/dom#960

PR-URL: #37693
Backport-PR-URL: #38386
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos pushed a commit to nodejs/node that referenced this pull request Apr 30, 2021
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #37693
Backport-PR-URL: #38386
Refs: whatwg/dom#960
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impacts documentation Used by documentation communities, such as MDN, to track changes that impact documentation topic: aborting AbortController and AbortSignal
Development

Successfully merging this pull request may close these issues.

Creating an already aborted AbortController
7 participants