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.

Sign up for GitHub

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

Support abort API #592

Merged
merged 27 commits into from
May 23, 2018
Merged

Support abort API #592

Changes from 2 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
93e9cec
Support abort API
jamesplease Dec 7, 2017
53565c1
Support abort without replacing native fetch
jamesplease Dec 7, 2017
9a7d232
Remove event listener; use request.signal
jamesplease Dec 7, 2017
fd5c261
Skip creation of XHR if signal is already aborted
jamesplease Dec 7, 2017
318e95f
First pass at documentation
jamesplease Dec 8, 2017
0e1b651
More tweaks based on feedback
jamesplease Dec 8, 2017
d47f6c9
Remove some semicolons
jamesplease Dec 8, 2017
529c9fa
Add `signal` property to Request instead of Response
mislav Dec 13, 2017
8ba0cdb
Polyfill DOMException if missing
mislav Dec 13, 2017
074c89e
Polyfill DOMException
mislav Dec 13, 2017
b832229
Add tests for aborting requests
mislav Dec 13, 2017
c6aff30
Avoid creating XMLHttpRequest if we're immediately aborting
mislav Dec 13, 2017
d9aa590
Clear event listener for all xhr completion status.
joaovieira Feb 2, 2018
886a076
Merge pull request #1 from joaovieira/abort-polyfill
joaovieira Feb 2, 2018
9f9a00f
Test signal reuse. Add AbortSignal polyfill.
joaovieira Feb 2, 2018
bab0f3d
Consistent implementation of EventTarget as in MDN.
joaovieira Feb 2, 2018
0bed2ba
Split EventTarget and AbortSignal polyfills.
joaovieira Feb 3, 2018
66ed8ba
Merge pull request #2 from joaovieira/abort-polyfill
joaovieira Feb 3, 2018
738d51a
Add tests with signal within Request object.
joaovieira Feb 3, 2018
8f778f3
Fix EventTarget callback invocation context.
joaovieira Feb 3, 2018
f0e24f8
Native EventTarget is not inheritable.
joaovieira Feb 3, 2018
36bf692
Replace AbortSignal polyfill with abortcontroller-polyfill module.
joaovieira Feb 4, 2018
7f6b8d2
Upgrade abortcontroller-polyfill.
joaovieira Feb 4, 2018
c31fca9
Merge remote-tracking branch 'origin/master' into abort-polyfill
mislav May 23, 2018
f035860
Scope abortcontroller to polyfill only
mislav May 23, 2018
2bd7f22
Export DOMException for `instanceof` checks
mislav May 23, 2018
c873843
Polish documentation for aborting requests
mislav May 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions fetch.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,6 +443,10 @@
reject(new TypeError('Network request failed'))
}

xhr.onabort = function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need to attach onabort callback? The promise can be rejected within abortXhr() just as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mislav I believe this is the most correct as mentioned here. Calling abort on the XHR will set the right status code and readyState (https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/abort).

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with xhr.abort(). I'm just saying that we don't need to install the xhr.onabort callback just to reject the promise. We know that abortXhr() will be executed, so we can reject the promise in there.

Copy link
Contributor

@joaovieira joaovieira Feb 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I see.. xhr is never exposed so it can't be aborted from the outside. Is there any other way it could be aborted (e.g. by the browser)? If not, there shouldn't be any difference. I don't mind this way though, reads well :)

Copy link
Contributor

Choose a reason for hiding this comment

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

onabort wasn't needed until now, and since we control literally the only place from which the xhr is aborted, I don't think adding one level of indirection on top of abortXhr is necessary.

Copy link

Choose a reason for hiding this comment

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

Doesn't onAbort() also confirm that xhr.abort() was successful vs. just trusting the results of abortXhr()?

Copy link
Contributor

Choose a reason for hiding this comment

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

@dysonpro That's correct, but we aren't interested in whether xhr.abort() was performed successfully or not. We want to reject the promise right away because we know for sure an abort was requested.

Copy link

Choose a reason for hiding this comment

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

Thanks @mislav for your explanation. I am excited about this functionality so I have been following this PR carefully. I am already using this branch code in a project that requires aborting no longer needed requests.
Your point is that you want to ensure the promise is rejected, correct? I feel that it is closer to the core intention of the PR (and my own project as well) to ensure that the abort was completed successfully. Is there a way to do that without utilizing the xhr.onAbort() callback? Without that I feel that there could be a gap in the verification of the functionality for every platform and environment. Thanks again for your feedback.

Copy link
Contributor

@mislav mislav Feb 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@dysonpro Your concerns are valid, but we have different priorities when thinking about this. At the point when abortXhr() is invoked (which can only happen upon AbortSignal), I consider the request most definitely discarded. We can then call xhr.abort(); reject(...) all at once. Now, if there is a browser which would somehow fail to actually abort the XHR when xhr.abort() is called, and thus never invoke onabort callback, that is fine with me because we already ensured that the promise is rejected. I don't expect this case to happen often (or ever, for that matter) and so I'm not concerned with it. I also don't expect onabort happening without us explicitly requesting it (after all, it wasn't needed until now). The code change that I suggested, when all is said and done, is mostly stylistic.

Copy link

@MattiasBuelens MattiasBuelens May 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Is there any other way it could be aborted (e.g. by the browser)? If not, there shouldn't be any difference.

  • A browser extension (e.g. an ad blocker) could abort a request.
  • A service worker could intercept the fetch and replace it with a new request that has (or will have) an aborted signal.

These are rather obscure use cases, but they're real nonetheless. I think a general fetch polyfill should handle these, so it should listen for xhr.onabort.

There's nothing wrong with immediately rejecting after calling xhr.abort(), but it should also reject from xhr.onabort.

reject(new DOMException('Aborted', 'AbortError'))
}

xhr.open(request.method, request.url, true)

if (request.credentials === 'include') {
Expand All @@ -459,6 +463,10 @@
xhr.setRequestHeader(name, value)
})

if (init.signal && init.signal.addEventListener) {
init.signal.addEventListener('abort', xhr.abort)
Copy link

Choose a reason for hiding this comment

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

Should we remove the event listener also in the xhr.onabort function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup. I'll do that.

}

xhr.send(typeof request._bodyInit === 'undefined' ? null : request._bodyInit)
})
}
Expand Down