jonnyreeves / fetch-readablestream

Compatibility layer for efficient streaming of binary data using WHATWG Streams
MIT License
49 stars 10 forks source link

Support for AbortController #7

Closed pimterry closed 5 years ago

pimterry commented 6 years ago

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.

pimterry commented 6 years ago

@jonnyreeves Linting etc all now fixed, but it now seems to fail because the tests can't start browsers in SauceLabs. Any idea?

jonnyreeves commented 6 years ago

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 😀

pimterry commented 6 years ago

@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.

jonnyreeves commented 6 years ago

@pimterry sorry for the delay; I've fixed the tests on master as you can see here - if you rebase you should get a green tick and I'll merge.

pimterry commented 6 years ago

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.

jonnyreeves commented 6 years ago

Thanks for rebasing and investigating @pimterry

From looking at the code, my guess for the failure is because Chrome 48 does not support AbortController (google suggests it was introduced in Chrome 66).

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!

pimterry commented 5 years ago

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.

pimterry commented 5 years ago

@jonnyreeves would you mind releasing this to npm, so we can start using it elsewhere? Thanks!

jonnyreeves commented 5 years ago

Sorry for the delay! https://www.npmjs.com/package/fetch-readablestream/v/0.2.0