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

options.json === false #141

Closed
dhritzkiv opened this issue Nov 22, 2016 · 4 comments
Closed

options.json === false #141

dhritzkiv opened this issue Nov 22, 2016 · 4 comments

Comments

@dhritzkiv
Copy link

dhritzkiv commented Nov 22, 2016

Per the recent json option behaviour changes, where:

  • json: true causes body to be serialized with JSON.stringify; and by extension,
  • true is not a valid json value (anymore)

What should the behaviour for json: false be? By the logic of the second point (which I agree with), false should also not be a valid value.

However, passing json: false* currently results in "false" being sent, instead of disabling json behaviour.

* Passing false may be a desired thing to do if json: true is default behaviour for 99% of requests (for example, by using ajaxConfig in ampersand-sync), but you want to disable json behaviour for one request.

I propose that L161 is changed from:

if ("json" in options) {

to

if ("json" in options && options.json !== false) {

To disable json behaviour when json: false is passed into the options.

This raises questions of what to do about null and undefined, but I think this is a good starting point, as false is very explicit and unambiguous.

Let me know your thoughts, and I can submit a PR!

@naugtur
Copy link
Owner

naugtur commented Nov 23, 2016

oops.
I don't know how I overlooked that.

@naugtur
Copy link
Owner

naugtur commented Nov 23, 2016

I was also considering just changing it to

if(options.json)

but that would break backward compatibility for POSTing to APIs that respond with JSON while accepting empty body (undefined) or null

This feels urgent to me, so I'm not going to wait too long before releasing it.

@dhritzkiv
Copy link
Author

Seems

if (options.json)

or more explicitly

if (options.json === true)

and removing backwards compatibility of passing a body to json would be a candidate for v3.0

@naugtur
Copy link
Owner

naugtur commented Nov 23, 2016

Sounds good. Wanna do the honors and suggest that in the v3 roadmap discussion? :D

naugtur added a commit that referenced this issue Nov 23, 2016
fix #141 - json set to false should mean no json
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

No branches or pull requests

2 participants