Change default `as` in parse middleware

[agilgur5]

trafficstars

Currently the default property for as is body, but the logic makes this undesirables, as:

https://github.com/mjackson/http-client/blob/master/modules/index.js#L205

    if (as in response)
      return response[as]

will by default return response.body, which may already be populated.

I run a response middleware after the parse usually, but using parse('json'), recv(({body}) => body) results in undefined to be output.

If I instead use parse('json', 'jsonData'), recv(({jsonData}) => jsonData), it works as intended, returning the JSON/JS Object.

Not sure what a better name would be; perhaps bodyData sticking by previous conventions?

[mjackson]

Hmm, I'm not sure why parse('json') would give you an undefined response.body. In that case, response.body should be the JSON payload.

The purpose of that if statement is to protect against you running, for example, response.json() twice on the same response because the second one will fail after the body is already used. So the first time it is used response.body would be set. And on the second time, you'd just get the previous result.

Perhaps instead we should just change the parse middleware to throw if response.bodyUsed is true? That would hopefully prevent people from parsing twice in dev.

[rehevkor5]

The analysis by @agilgur5 is right. Response.body is present as a ReadableStream per https://fetch.spec.whatwg.org/#response-class

I ran into the same problem: using parse('json') per the docs gives me: response: ReadableStream {} response.body: undefined response.jsonData: undefined response.json: undefined

Changing to parse('json', 'jsonData') fixes it, I get: response: Response {jsonData: Object, type: "basic", etc...} response.body: ReadableStream {} response.jsonData: Object { my json data here } response.json: function json() { [native code] }

It seems like it would be better to change the default value for as.

[rehevkor5]

I'm guessing you haven't run into this because GitHub's WHATWG Fetch polyfill doesn't have support for streaming, whereas the browsers do now: https://github.com/github/fetch/issues/198

[agilgur5]

@rehevkor5 thanks for debugging this! I was confused as to why I was getting an error when there's a passing unit test on this -- didn't even think to check for environment differences / the underlying Fetch implementation!

[sparty02]

This bit me today too!