Support for AbortController #7
Conversation
src/xhr.js
Outdated
| @@ -1,5 +1,19 @@ | |||
| import { Headers as HeadersPolyfill } from './polyfill/Headers'; | |||
|
|
|||
| function getAbortError() { | |||
There was a problem hiding this comment.
Nit pick: as this constructs a new instance could we please prefix using make or create instead of get which, IMHO is ambiguous in this regard
README.md
Outdated
| This is fully in any environment that supports both ReadableStreams & AbortController directly, and has basic support in most other environments, though you may need [a polyfill](https://www.npmjs.com/package/abortcontroller-polyfill) in your own code to use it. To abort a request: | ||
|
|
||
| ```js | ||
| let controller = new AbortController(); |
|
@jonnyreeves Linting etc all now fixed, but it now seems to fail because the tests can't start browsers in SauceLabs. Any idea? |
|
Hmm no idea, but this project hasn't been built in quite a long time so it could be due to out of date testrunner dependencies. I'm on holiday until next weekend so won't be able to take a look before then. If you are happy to take a look then please be my guest 😀 |
|
@jonnyreeves sorry, clearly I haven't got around to looking at this. I suspect either you're right and it's a dependency issue, or it's something to do with your SauceLabs config (you might need to update the password or something) but it's hard to debug without any access to the SauceLabs account myself. I'd love to get this merged anyway though, let me know if there's anything I can do. |
pimterry
force-pushed
the
abort-response
branch
from
81932a7 to
83146ed
Compare
|
I've rebased. The new tests fail in SauceLabs though because that's running Chrome 48, which is pretty out of date (January 2016). I think it should definitely pass in 66+, would you be happy for me to update the tests to run in that? In theory it should run in many older versions too, and it'll never break any existing behaviour either way, but as noted in the readme there'll be some differences and only basic support with some older browsers. Note that these tests are passing happily in IE11 & Firefox (via polyfills), it's just Chrome 48 that has trouble. |
README.md
Outdated
| @@ -56,6 +56,23 @@ fetchStream('/endpoint') | |||
| .then(chunks => console.dir(chunks)) | |||
| ``` | |||
|
|
|||
| `AbortController` is also supported in many environments, allowing you to abort ongoing requests. | |||
|
|
|||
| This is fully in any environment that supports both ReadableStreams & AbortController directly, and has basic support in most other environments, though you may need [a polyfill](https://www.npmjs.com/package/abortcontroller-polyfill) in your own code to use it. To abort a request: | |||
There was a problem hiding this comment.
This is fully in any environment that supports both ReadableStreams & AbortController directly
Is the first sentence missing a word? It doesn't quite read correctly
|
Thanks for rebasing and investigating @pimterry From looking at the code, my guess for the failure is because Chrome 48 does not support If updating Chrome to v66 makes the tests pass then I'm happy to include that change, however please can you mention that aborting is only supported natively in Chrome 66+ in the README? Thanks! |
|
Hi @jonnyreeves sorry for the delay. I looked into this further, because while Chrome 48 doesn't support AbortController, it also shouldn't be breaking like this. The tests do pass in IE11 for example, which has even less support for that. It turns out this was actually caused by some incorrect polyfill setup in the tests for certain environments (any that do have fetch & readable stream, but have no abort controller), which caused this problem, so I've now fixed that. In these cases, abort still isn't totally supported (unsent and unread requests can be aborted successfully, but requests that are actively being read from can't), but now nothing breaks, the tests pass, and I think this is sufficient for the few older browsers in this situation. I'v also updated the readme to clarify things a little, and link to caniuse for detailed info on browser support. I think that's everything, let me know if you'd like me to look at anything else. |
There was a problem hiding this comment.
Fantastic work @pimterry; really appreciate your following up on this and diving deep into why the tests were failing 🎉
|
@jonnyreeves would you mind releasing this to npm, so we can start using it elsewhere? Thanks! |
|
Sorry for the delay! https://www.npmjs.com/package/fetch-readablestream/v/0.2.0 |
This brings in the changes from jonnyreeves/fetch-readablestream#7 Change-type: patch
Fixes #6.
This PR makes requests through this library abortable. The tests should explain the target behaviour pretty clearly I think.
I've got these tests passing locally in Firefox 62 & Chrome 67 - some help testing in more environments would be great!
I'm going to do some downstream integration testing in our usage of this library elsewhere to confirm it works nicely there too, I'll update this once that's done.
I'm not confident that this works in 100% of cases, and it definitely doesn't cover 100% of the spec, so I've left some caveats in the readme. It should cover the vast majority though, will work fine anywhere with full native readable-stream + abortcontroller support (for now just new Chrome) and it shouldn't ever break anything, so it's definitely a significant improvement.