-
Notifications
You must be signed in to change notification settings - Fork 159
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
Use abort reason in ReadableStreamPipeTo #1182
Merged
Merged
Changes from 15 commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
5f36dda
Merge pull request #1 from whatwg/master
nidhijaju b5dbc0e
Merge pull request #2 from whatwg/master
nidhijaju f423409
Merge pull request #3 from whatwg/master
nidhijaju 631447b
Merge remote-tracking branch 'upstream/master'
nidhijaju 72e8f57
Merge remote-tracking branch 'upstream/master'
nidhijaju 7c80a45
Merge branch 'whatwg:main' into master
nidhijaju e1cfefe
Merge branch 'whatwg:main' into master
nidhijaju 8ed62f7
Merge branch 'whatwg:main' into master
nidhijaju 3763ad2
Merge branch 'whatwg:main' into master
nidhijaju 53562da
Merge branch 'whatwg:main' into master
nidhijaju ec7a41a
use abort reason in PipeTo
nidhijaju 8b2053c
update ref impl
nidhijaju d2abdfa
pass reason for signal abort
nidhijaju 122b7ff
update ref impl
nidhijaju 0f057fc
fix formatting
nidhijaju d36baab
remove defaulting logic
nidhijaju 15e4efe
use aborted predicate
nidhijaju 2bf43f6
update wpt submodule
nidhijaju 8bd9267
Update wpt-runner
MattiasBuelens 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
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 we might want an abstraction for this if every specification that adopts signals needs this. I put a suggestion in whatwg/fetch#1343 (comment).
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.
Why do we need these logic?
signal.reason
will be an "AbortError"DOMException
whencontroller.abort(undefined)
is called so it should be rare to seeundefined
here. I don't think we should care about the case.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 a good point, when would we see undefined here?
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.
Ah, fair point. I'll remove the if condition here and just set
error
to the signal's abort reason directly. Do you think it would be a good idea to have an assert here to check that the reason is not undefined just to be sure?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 was wondering about that. Perhaps it should be in DOM's "signal abort" as otherwise we'd have to put the assert in each spec for consistency?
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 it might be better to have the assert here instead of DOM's "signal abort" because
error
is not only used to signal abort in this function.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 is run as the result of signal abort being run, no? Or I suppose in the case the signal was already aborted. Either way, perhaps whatwg/dom#1027 (comment) is a better clarification?
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.
Ah yes, I think you are right. I've made the changes to the DOM spec based on your suggestion :)