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

Editorial: account for removal of AbortSignal's follow #1277

Merged
merged 2 commits into from
May 8, 2023

Conversation

shaseley
Copy link
Contributor

@shaseley shaseley commented May 2, 2023

"Follow" is being removed in whatwg/dom#1152.

cc: @annevk. AFAIK this is the only other use aside from fetch.


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.

Ah, this is pretty straightforward compared to Fetch.

index.bs Outdated
Comment on lines 7002 to 7003
reason=]. Specifications should not [=AbortSignal/signal abort=], as that would interfere with the
normal use of this signal to respond to the stream being [=abort a writable stream|aborted=].
Copy link
Member

Choose a reason for hiding this comment

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

Follow-up nit: must not? Or what would a good reason be?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agreed, "must not" would be better. Only the Streams standard itself should signal abort.

However, since whatwg/dom#1152 will make "to signal abort on an AbortSignal" private, it will no longer be possible for other specifications to affect this signal. So we can hopefully get rid of this entire footnote. 😁

By the way, should we already refactor Streams to use "to signal abort on an AbortController" instead? Or should we wait until whatwg/dom#1152 has landed?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I had missed that it no longer exports that definition.

@shaseley if that was intentional this would have to be a larger PR and other specifications would need updating as well: https://dontcallmedom.github.io/webdex/s.html#signal%20abort%40%40AbortSignal%40dfn.

@MattiasBuelens no need to do anything just yet. Perhaps we should scope down 1152 and leave that as a follow-up.

Copy link
Contributor Author

@shaseley shaseley May 3, 2023

Choose a reason for hiding this comment

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

+1 to scoping down 1152 and making 'signal abort' private in a follow-up. I'll fix 1152, update the language to 'must not', and work on the follow-up after 1152 lands. Sorry about the confusion!

@annevk annevk requested a review from MattiasBuelens May 3, 2023 07:54
@annevk annevk mentioned this pull request May 5, 2023
4 tasks
@domenic domenic merged commit 1222f88 into whatwg:main May 8, 2023
annevk pushed a commit to whatwg/dom that referenced this pull request May 17, 2023
- This implements an optimization that puts all children on
  non-dependent signals (i.e., those associated with a controller).
  This allows "intermediate" nodes (e.g., B in A follows B follows C)
  to be garbage collected if they are being kept alive to propagate
  aborts.

- This removes the follow algorithm, so callsites will need to be
  updated.

- The "create a composite abort signal" algorithm takes an interface so
  that TaskSignal.any() can easily hook into it, but create a 
  TaskSignal.

- Some algorithms that invoke "follow" create an AbortSignal in a 
  particular realm. This enables doing that, but borrows some language 
  from elsewhere in the spec w.r.t. doing the default thing. Neither of
  the other two static members specify a realm.

Follow-up PRs:

- whatwg/fetch#1646
- w3c/ServiceWorker#1678
- whatwg/streams#1277

This also sets the stage to make AbortSignal's "signal abort" fully 
internal. #1194 tracks the remainder.

Tests: web-platform-tests/wpt#37434 and web-platform-tests/wpt#39785.

Fixes #920.
@shaseley shaseley deleted the abortsignal-any branch May 24, 2023 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants