Handle case where ky responses have no body with a getter for a ReadableStream

[pcowgill]

Closes https://github.com/ipfs/js-ipfs-http-client/issues/1215

@hugomrdias @alanshaw Let me know what you think! The tests are passing, I added a new test case, and I've run the lint script.

[pcowgill]

The basic approach Hugo and I discussed for this PR was to pass the entire ky response to the helper lib that converts the response to an async iterable rather than just passing the Response.body and hoping that part is enough.

[pcowgill]

Thanks for this PR, it mostly looks good, a few changes needed mentioned in the comments.

@achingbrain Thanks for the quick review! I'll work on addressing the comments now.

[pcowgill]

@achingbrain Okay, I think I've addressed all of your comments! Thanks again for the quick review. Could you please give this another review when you have a moment?

I reverted all of the style changes manually, although it retrospect it may have been faster to configure some local workspace settings to match the styling conventions of this project. Longer term I think this would be a nice improvement to make.

[pcowgill]

Yikes, it looks like there are a lot of merge conflicts now that another PR was merged. I'll work on resolving them.

[pcowgill]

Looks like it was this PR https://github.com/ipfs/js-ipfs-http-client/pull/1183

[pcowgill]

@achingbrain @hugomrdias I don't think getting the conflicts resolved gates an interim PR review, though, in case you have time. Thanks!

[pcowgill]

@achingbrain I fixed the merge conflicts. We need to decide how we want to handle this issue - https://github.com/alanshaw/stream-to-it/issues/3#issuecomment-579471374 - before merging this, now that one of the previously modified files, lib/stream-to-async-iterable, was extracted into this shared package

[pcowgill]

Some tests are failing with the new logic introduced in this PR, like this one:

  1) .files (the MFS API part)
       .add pins by default:
     Error: Neither Response.body nor Response.arrayBuffer is defined

Interestingly those tests weren't failing with the earlier version of these changes against version 41.x.

[pcowgill]

The test failure was because I did a find/replace relative to an earlier version master, so the changes were missing some new uses of res.body that needed to be changed to res in the latest version of master. The tests should be passing now 🤞

[pcowgill]

It looks like the Travis failure is just it hitting a timeout, which I don't think is something that my PR would increase the probability of, perhaps something that's just stochastic?

[pcowgill]

As for the codecov warnings, it looks like codecov never ran for https://github.com/ipfs/js-ipfs-http-client/pull/1183, so the difference may be from that PR

[alanshaw]

@pcowgill apologies I didn't get to this sooner. Did you look into adding a global afterResponse hook when in react native?

It might be less invasive to add a .body property to the response here with an async iterable that calls .arrayBuffer(). Then all the rest of the code can remain the same and it would be easier to remove when RN supports .body.

What do you think?

[pcowgill]

@alanshaw To make sure I understand the suggestion, which file are you thinking that change would land in? Thanks!

[pcowgill]

@alanshaw Curious for your thoughts on the codecov comment too!

[alanshaw]

@alanshaw To make sure I understand the suggestion, which file are you thinking that change would land in? Thanks!

https://github.com/ipfs/js-ipfs-http-client/blob/e9aaa750beeb000f485fb31d43d2af648676dc8e/src/lib/configure.js#L40-L42

[elmariachi111]

Just checked out this PR because when I run jest on my setup (ionic / Typescript / React / node 12), and just test a simple ipfs.cat it fails with Cannot read property 'destroy' of undefined. This PR's fix works 99% around that issue by supporting res.arrayBuffer instead of res.body. But I think another cause / usage is hidden in error-handler#21 -> this line fails first on my machine. When I hack into it with

if ((isNode || isElectronMain) && response.body) response.body.destroy()

my jest tests are green again :)

[hugomrdias]

we will be doing some changes around http requests handling soon to fix these issues, stay tuned.

[pcowgill]

we will be doing some changes around http requests handling soon to fix these issues, stay tuned.

@hugomrdias That's great! Can you share any additional details about this? Thanks!

[pcowgill]

@pcowgill apologies I didn't get to this sooner. Did you look into adding a global afterResponse hook when in react native?

It might be less invasive to add a .body property to the response here with an async iterable that calls .arrayBuffer(). Then all the rest of the code can remain the same and it would be easier to remove when RN supports .body.

What do you think?

@alanshaw Sure, that suggestion works for me.

@hugomrdias Are you already planning on implementing the changes Alan described in the child package of the js-ipfs monorepo as part of the ky removal, or should I make a PR against the ky removal branch with these changes? (Assuming the ky removal branch will take at least a few more days to be merged.)

[pcowgill]

Will there still be the equivalent of an afterResponse hook after removing ky?

[hugomrdias]

There will be no hooks and would advise against making the PR that specific problem needs to be fixed outside http-client