Skip to content

Add AbortSignal.aborted() static - #960

Merged
annevk merged 5 commits into
whatwg:mainfrom
jasnell:abortsignal-aborted-static
Mar 12, 2021
Merged

Add AbortSignal.aborted() static#960
annevk merged 5 commits into
whatwg:mainfrom
jasnell:abortsignal-aborted-static

Conversation

@jasnell

@jasnell jasnell commented Mar 9, 2021 •
edited by annevk
Loading

Copy link
Copy Markdown
Contributor

Returns an already aborted AbortSignal per #959

Fixes: #959

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

Preview | Diff

annevk
annevk reviewed Mar 10, 2021
View reviewed changes

@annevk annevk left a comment

Copy link
Copy Markdown
Member

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

Comment thread dom.bs Outdated
Comment thread dom.bs Outdated
Comment thread dom.bs Outdated
@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 jasnell mentioned this pull request Mar 10, 2021
Merged
@jasnell

ghost commented Mar 10, 2021 •
edited by jasnell
Loading

Copy link
Copy Markdown
Contributor Author

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

@jasnell jasnell mentioned this pull request Mar 10, 2021
Closed
annevk
annevk reviewed Mar 11, 2021
View reviewed changes

ghost left a comment

Copy link
Copy Markdown
Member

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.

Comment thread dom.bs Outdated
Comment thread dom.bs
@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
James M Snell added 2 commits March 11, 2021 07:09
@jasnell
jasnell force-pushed the abortsignal-aborted-static branch from 089887c to 7ed2987 Compare March 11, 2021 15:09
@josepharhar

ghost commented Mar 11, 2021

Copy link
Copy Markdown
Contributor

lgtm, this looks particularly easy to implement :)

@annevk

ghost commented Mar 11, 2021

Copy link
Copy Markdown
Member

@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

ghost commented Mar 11, 2021

Copy link
Copy Markdown
Contributor Author

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

Done!

@domenic

ghost commented Mar 11, 2021

Copy link
Copy Markdown
Member

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

@benjamingr

ghost commented Mar 11, 2021

Copy link
Copy Markdown
Member

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

@mfreed7

ghost commented Mar 11, 2021

Copy link
Copy Markdown
Contributor

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

ghost commented Mar 12, 2021

Copy link
Copy Markdown
Member

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

annevk
annevk reviewed Mar 12, 2021
View reviewed changes
Comment thread dom.bs
annevk
annevk reviewed Mar 12, 2021
View reviewed changes
Comment thread dom.bs Outdated
@annevk
annevk merged commit aa384af into whatwg:main Mar 12, 2021
@jasnell

ghost commented Mar 12, 2021

Copy link
Copy Markdown
Contributor Author

Yeah I'll do so this morning!

@annevk

ghost commented Mar 15, 2021

Copy link
Copy Markdown
Member

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

ghost commented Mar 15, 2021 •
edited by jasnell
Loading

Copy link
Copy Markdown
Contributor Author

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

@benjamingr

ghost commented Mar 17, 2021

Copy link
Copy Markdown
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

ghost commented Mar 17, 2021

Copy link
Copy Markdown
Member

cc @whatwg/documentation

@sideshowbarker

ghost commented Mar 18, 2021 •
edited by sideshowbarker
Loading

Copy link
Copy Markdown
Member

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 manekinekko mentioned this pull request Mar 18, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@annevk annevk annevk left review comments

Assignees

No one assigned

Labels

impacts documentation Used by documentation communities, such as MDN, to track changes that impact documentation topic: aborting AbortController and AbortSignal

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Creating an already aborted AbortController

7 participants

@jasnell @josepharhar @annevk @domenic @benjamingr @mfreed7 @sideshowbarker