Fixing parse by responseType

[a631807682]

Issue

Closes #907

Desc

1. Only responseType 'json' should parse to JSON, 'arraybuffer'/'blob'/'stream' return original response, 'text' or '' return string response.
2. transformResponse behavior should be same in nodejs and browser, data params should be a right type.

[benc-uk]

Is anyone from the project looking these PRs?
This one fixes a two year old bug!

[a631807682]

@jcrben I add unit test for responseType json, but test was failed by should support max redirects test case, it is success in my environment and my changes don't involve it, any suggest？

[jcrben]

Not really. Keep in mind I'm just a user like yourself (and a casual one at that). Maybe a maintainer will have an idea.

[Alanscut]

@a631807682 #2570 will fix the CI failed. not your problem.

[a631807682]

@jcrben @Alanscut thanks.

[alinnert]

Isn't it possible to remove the transformResponse function entirely? At this state it's just a noop. Also, users can fix the issue (#907) by override transformResponse with undefined or an empty array which has the same result.

[a631807682]

Isn't it possible to remove the transformResponse function entirely? At this state it's just a noop. Also, users can fix the issue (#907) by override transformResponse with undefined or an empty array which has the same result.

transformResponse is designed for use to transform the data, remove it is a big changes. we can override transformResponse with undefined, but it's still a bug.

[alinnert]

I don't mean to remove that feature entirely. I just mean the left-overs in the code which is:

function transformResponse(data) {
  return data;
}

This is just a default implementation of a row of possible transform functions. The property transformResponse accepts an array of functions after all. If you remove that function it shouldn't change anything. And there's no need for a default transform, especially if it does nothing.
Or in other words: Axios must work with an empty transformResponse array (and according to lib/core/transformData.js it does), which should also be the default value.

The function transformData in the file lib/core/transformData.js is the one to loop over this array and transforms the data. If the array is empty then nothing happens (which is expected and also the point if this PR). If the array contains one method which just returns its input, you have the same effect, but you waste more CPU time.

But I've noticed that the test file test/specs/defaults.spec.js mentions transformResponse[0](...). This is the only place that expects there to be at least one transform function. This could also be removed, because you can't test the "first" function if there's none.
(Sure, transformResponse should be tested. But I think the more general test(s) is/are located in a different spec file. This one in particular is about the default transform function, which should not exist after this PR.)

Do you know what I mean, now?

[a631807682]

@alinnert I set transformResponse to empty array, and remove test case from test/specs/defaults.spec.js . Original test/specs/transform.spec.js contains only transformRequest test case, i wrap them and add test for transformResponse. Thank you for review.

[jasonsaayman]

Since this is a breaking change and changes the behaviour I have added it to v1

[jasonsaayman]

@a631807682 Please can you check tests?

[a631807682]

@jasonsaayman I fixed it, but i don't know why CI hasn't been triggered.

[Liwoj]

Just a question. Is this change going to be released in 1.0 ?

Automatic JSON parsing seemed always a bit strange to me. With current released version of Axios (0.21.4) one can at least disable it by transformResponse: []. With this change (moving parsing directly into xhr adapter) we loose the ability to switch it off.

I don't want Axios to parse JSON responses for me - I want to parse it by myself as I need to pass a custom reviver function into JSON.parse

[jasonsaayman]

Good question it would be released in v1 but maybe it's the wrong approach

[DigitalBrainJS]

@jasonsaayman We already have a transitional option config.transitional.forcedJSONParsing=false to disable automatic JSON parsing. Isn't it the same thing?