redux-api-middleware icon indicating copy to clipboard operation
redux-api-middleware copied to clipboard

Clone the response object before passing to payload/meta functions

Open lizzielizzielizzie opened this issue 6 years ago • 6 comments
trafficstars

When implementing meta functions, one must currently create a clone of the response object in order to read the response body. To do that, they must also implement the payload function, clone the response, and return the json. This behavior has been documented a few times that I've found so far:

  • https://github.com/agraboso/redux-api-middleware/issues/134
  • https://github.com/agraboso/redux-api-middleware/issues/92

By calling those functions using the clone, we avoid this issue.

Notes:

  • I had to update the test snapshots because the ReadableStream are no longer marked as disturbed after cloning.

lizzielizzielizzie avatar Dec 11 '18 21:12 lizzielizzielizzie

Coverage Status

Coverage remained the same at 100.0% when pulling 6e56b243e41f55c305b0839375fe87171ac77165 on NickWer:master into 6e782cdea6d5451e6e96406de631db7c11df1a2d on agraboso:master.

coveralls avatar Dec 11 '18 21:12 coveralls

I realized I have made a mistake. Will push a fix shortly and unmark as WIP when it's ready. My apologies.

lizzielizzielizzie avatar Dec 12 '18 14:12 lizzielizzielizzie

Should be OK now. I added a little unit test, too.

lizzielizzielizzie avatar Dec 12 '18 16:12 lizzielizzielizzie

I agree with this PR, but I think user should be responsible for reading the response in a correct way. As a user I would just made my own getJSON function. Cloning a big response 2 times, can be a little too much :)

import { getJSON as noCloneGetJSON } from 'redux-api-middleware'

export function getJSON(res) {
  return noCloneGetJSON(res.clone())
}

unrevised6419 avatar Sep 17 '19 08:09 unrevised6419

Thank you @NickWer, and sorry for the delay 😿This PR looks good 👍

This does seem to be a common pain point, which this approach would help avoid.

user should be responsible for reading the response in a correct way Cloning a big response 2 times, can be a little too much :)

@iamandrewluca great feedback, thank you. Are there downsides to cloning the response internally? Is your concern mostly about performance or something else?

nason avatar Dec 15 '19 21:12 nason

@nason Yeap, mostly about performance. But I don't think there is such big performance lose.

unrevised6419 avatar Dec 16 '19 08:12 unrevised6419