Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

minimalistic transformResponse for json response type #26

Merged
merged 3 commits into from
Jul 23, 2020

Conversation

Arminkhodaei
Copy link
Contributor

Hi,
#21 & maybe #13 could be solved with this PR. I tried to keep it minimal, even though it's not the most generic way to be done.
There should be a minor conflict with my previous PR, but I still preferred to push on separate branches.

@Arminkhodaei
Copy link
Contributor Author

About exposing transformResponse outside of the library, I understand the axios compatibility issues, but I don't find any other reason to do so. As I see it, response interceptors are able to satisfy the same need. Correct me if I'm wrong in this matter, please :)

@tandrewnichols
Copy link

What's the eta here? Would like to switch to redaxios for size, but I'd prefer not to have to go find every existing usage of axios and make them JSON.parse the response data manually.

@developit
Copy link
Owner

developit commented May 11, 2020

Looks good - I believe inlining the transformResponse function will help with file size here (this PR is up 27b, the size bot can't post on fork PRs).

One thing I'm not quite sure of yet though, is whether it's actually necessary to use JSON.parse here - I get the impression Axios defaults to JSON rather than text for responses, or that it tries both. If that's the case, it might be possible to implement that by replacing the following line:

: res[options.responseType || 'text']();

... with something like:

const withData = options.responseType === 'stream'
	? Promise.resolve(res.body)
	: res[options.responseType || 'json']().catch(() => res.text());

@developit developit self-requested a review May 11, 2020 18:57
@Stamper
Copy link

Stamper commented Jun 24, 2020

hi guys,
unfortunately redaxios didn't bring JSON object to me in case of Content-Type == application/json, therefore I can't see how this line could fix #21
I'd rather erase that if (typeof data === 'string') clause and keep try JSON.parse(data) catch itself.

@developit developit merged commit 21cca9e into developit:master Jul 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants