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

JSON improvements: throw if JSON parsing failed; number, boolean can be passed directly as payload for encoding to JSON #2613, #61, #907 #3688

Merged
merged 14 commits into from Apr 19, 2021

Conversation

@DigitalBrainJS
Copy link
Contributor

@DigitalBrainJS DigitalBrainJS commented Mar 19, 2021

Added

  • support for primitive types to be converted to JSON if the request Content-Type is application/json (under nodejs we get an error) (#2613);
  • transitional options object;
  • options validator to assert transitional options;
  • transitional option silentJSONParsing - throws SyntaxError if JSON parsing failed while responseType is json and transitional.silentJSONParsing is false; (#61). Default: true - keep current behavior.
  • transitional option forcedJSONParsing to control automatic JSON parsing from the response string. Default true - keep current behavior (#2791, #907)
  • request transformers now calling in the context of the config object.
  • automated deprecation warning message depending on the package version
    Example:
    [Axios v0.21.1] Transitional option 'silentJSONParsing' has been deprecated since v0.19.3 and will be removed in the near future
    Update
  • Added transitional.clarifyTimeoutError to throw ETIMEDOUT error instead of generic ECONNABORTED on request timeout (#1543);
  • Added support of onloadend handler if available instead of onreadystatechange;
  • Added xhr timeout test;
  • Fixed potential bug of xhr adapter with proper handling timeouts&errors (FakeXMLHTTPRequest failed to handle timeouts);

The API behavior is the same as before, but now we can set the transitional.silentJSONParsing to false for verbose JSON parsing.
Since transient parameters are introduced by the PR, we can support multiple API behaviors at the same time, allowing users to choose the one they want and making it easier to transition between package versions.

When silentJSONParsing is false & responseType is 'json':

await axios.get("https://run.mocky.io/v3/95e6ffc7-ac0c-46c2-aded-7149eeb2ef2d", {
  responseType: "json", 
  transitional: {
    silentJSONParsing: false
  }
});
SyntaxError: Unexpected token b in JSON at position 1
    at JSON.parse (<anonymous>)
    at Object.transformResponse (axios2.js:1023)
    at transform (axios2.js:493)
    at Object.forEach (axios2.js:257)
    at Object.transformData (axios2.js:492)
    at onAdapterResolution (axios2.js:1120)

When silentJSONParsing is true (default value & current behaviour):

await axios.get("https://run.mocky.io/v3/95e6ffc7-ac0c-46c2-aded-7149eeb2ef2d", {
  responseType: "json", 
  transitional: {
    silentJSONParsing: true
  }
});
response= {
  config: {url: "https://run.mocky.io/v3/95e6ffc7-ac0c-46c2-aded-7149eeb2ef2d", method: "get", headers: {…}, transformRequest: Array(1), transformResponse: Array(1), …}
  data: "{badJSON": "123"}"
  headers: {...}
    status: 200
    statusText: "OK"
  }
  ...
}

Recently published as axios-lab npm fork
CodeSandbox demo

…n-primitives

� Conflicts:
�	lib/adapters/xhr.js
…est Content-Type is 'application/json';

Added throwing SyntaxError if JSON parsing failed and responseType is json;
Added transitional option object;
Added options validator to assert transitional options;
Added transitional option `silentJSONParsing= true` for backward compatibility;
Updated README.md;
Updated typings;
Fixed typo;
Added validator.spec.js;
@DigitalBrainJS DigitalBrainJS changed the title Feat/JSON improvements #2613, #61, #907 JSON improvements #2613, #61, #907 Mar 19, 2021
@DigitalBrainJS DigitalBrainJS changed the title JSON improvements #2613, #61, #907 JSON improvements: throw if JSON parsing failed; number, boolean can be passed directly as payload for encoding to JSON #2613, #61, #907 Mar 19, 2021
…tead of generic ECONNABORTED on request timeouts;

Added support of onloadend handler if available instead of onreadystatechange;
Added xhr timeout test;
Fixed potential bug of xhr adapter with proper handling timeouts&errors (FakeXMLHTTPRequest failed to handle timeouts);
@jasonsaayman jasonsaayman self-requested a review Apr 13, 2021
@jasonsaayman jasonsaayman self-assigned this Apr 13, 2021
Copy link
Member

@jasonsaayman jasonsaayman left a comment

Looks good to me

Loading

@jasonsaayman jasonsaayman merged commit 5ad6994 into axios:master Apr 19, 2021
1 check passed
Loading
@DigitalBrainJS
Copy link
Contributor Author

@DigitalBrainJS DigitalBrainJS commented Apr 19, 2021

@jasonsaayman Oops. It seems the test failed under IE11 just because it has a different SyntaxError message.
But basically, that is not a necessary assertion, so we can just delete the line:
expect(thrown.message).toContain('JSON');

Loading

@DigitalBrainJS
Copy link
Contributor Author

@DigitalBrainJS DigitalBrainJS commented Apr 19, 2021

@jasonsaayman Just fixed it in #3763 PR;

Loading

@rinsuki
Copy link

@rinsuki rinsuki commented Jul 26, 2021

Could you release new version with this changes? I want to use it soon.

Loading

@kawanet
Copy link

@kawanet kawanet commented Sep 5, 2021

@DigitalBrainJS
Did the PR make any breaking changes?
The PR describes it would keep the previous behavior, however.

Default: true - keep current behavior.
The API behavior is the same as before

My app which uses a POST method fails since your commit 5ad6994 merged.
It seems this makes some part of my request header or body broken.

  • axios release 0.21.1 -> OK
  • commit d99d5fa -> OK
  • commit 5ad6994 -> NG
  • axios release 0.21.2 and 0.21.3 -> NG

Loading

@burgalon
Copy link

@burgalon burgalon commented Sep 5, 2021

Loading

@kawanet
Copy link

@kawanet kawanet commented Sep 5, 2021

I've post a minimum code to reproduce the issue. See 4018 below not 4017 above.

Loading

@phil-w
Copy link

@phil-w phil-w commented Sep 6, 2021

Not sure exactly what's said here, but this just broke my tests.

I had an old axios.post which (a) set the header content type to 'application/json' and passed a JSON.stringify() structure as body. That started to error 400 the latest axios update 0.21.3.

Removing both the header type and the stringification fixed it, so it's all good.

Loading

});

var deprecatedWarnings = {};
var currentVerArr = pkg.version.split('.');
Copy link

@luchaos luchaos Sep 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

var pkg = require('./../../package.json'); tries to load my project's package.json which has no version key set, causing the application to fail on boot in the browser.
what's the purpose of this validator and why does it need my project's package.json and a potentially non-existing version? (it was brought to my attention that that's the culprit after all - for the version is a required package.json field https://docs.npmjs.com/creating-a-package-json-file#required-name-and-version-fields)
it's a vue3/webpack project.
edit: seems to be related? #3499

Loading

Copy link
Member

@jasonsaayman jasonsaayman Sep 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will have a look as to why this is happening

Loading

Copy link

@luchaos luchaos Sep 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's an edge case, really - people do the weirdest things in their project's package.json (as my own example showed). the fix is simple enough, the package.json just has to be valid and meet the minimum requirements (the version should be there at all times, technically - til).

yet, this issue might highlight a completely different problem. relying on the validity of / bundling the package.json might not be the best solution to use a version for negotiation (as seen in this PR) nor to be used for the user agent (as seen in the linked issue above where it is imported in the http adapter).

is this actually a path/loader issue and the version to be read is the one from axios' own package.json, not the project's? looking at the http adapter code it looks like it wants axios' version (headers['User-Agent'] = 'axios/' + pkg.version;)
if that's the case then i'd assume that axios is potentially sending the wrong version (the project's it's used in) in the user agent header, too?

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meet the same question here, so do you know how to solve it?

Loading

Copy link

@luchaos luchaos Sep 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meet the same question here, so do you know how to solve it?

For me the solution was to add a version to package.json. "version": "0.0.0" works just fine.

Loading

}
// readystate handler is calling before onerror or ontimeout handlers,
// so we should call onloadend on the next 'tick'
setTimeout(onloadend);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this setTimeout appears to be causing test failures for me when using sinon/nise FakeServer with FakeTimers in Jest. And possibly with nock.
replacing this line with onloadend() causes my tests to pass again.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

10 participants