Change the json option to be a boolean-only modifier for body

[bjoerge]

This fixes #134 and makes xhr behave like request wrt the json option. I wasnt able to run the phantom tests locally, but manually verified that the test suite passed in IE11, MS Edge, Firefox, Chrome and Safari 10.

This is a breaking change, so qualifies for a major version bump.

[yoshuawuyts]

have definitely run into this before; think this is more intuitive for sure. So personally 👍 on this. If this is going to be a thing you might also want to consider updating the docs

[bjoerge]

Ah, yeah, thanks for headsup about the docs. Guess I assumed they were already aligned with request. Just pushed an update.

[TehShrike]

Oh wow, I can't believe we didn't even have any tests testing POSTing a request body!

This change looks good to me.

[mhart]

Just to be clear – this is a pretty massive breaking change in API (just making sure this isn't bumped lightly)

[mhart]

(by that I mean – we use the json option all over our codebase to send JSON FWIW)

[TehShrike]

Definitely. I think maintaining compatibility with request is still a goal of @naugtur's, and the current behavior isn't that intuitive anyway, so I imagine we'll want to deploy this, even though it will be a breaking change.

I was just surprised that the current behavior wasn't asserted at all in the tests already :-x

[mhart]

Right, that was a goal that only came to light recently: 743ea3e#diff-04c6e90faac2675aa89e2176d2eec7d8 (I had no idea it was trying to replicate the request API tbh)

[mhart]

I'm just saying – this change is all fine if xhr wants to change its API, but it should land in 3.x – not 2.x – otherwise you're going to break a lot of ppl's code.

I'm sure that was the plan anyway, just wanted to double check.

[TehShrike]

No worries. I'm pretty hardcore on semver.

It might be a discussion for another issue, but I'm not massively into claiming interface parity with request, since we don't have the tests to guarantee that. If xhr changed to be framed as a "request-like wrapper for XMLHttpRequest", that would be fine with me. That's more @naugtur's decision for now though.

Another side note about breaking versions: this project doesn't have a changelog yet. Adding one would probably be a good idea, but whether we do or not, I think it would be a good idea to note at least recent breaking changes in the readme. Some kind of "Interface changes between 2.x and 3.x" heading above the "Other signatures" heading, maybe.

[naugtur]

I don't want to be the party pooper here, but If it's not consistent with how request handles json option, it defeats the purpose of having xhr as a miniature drop-in replacement for request in the browser.

Some of xhr users (including me personally) have code that is shared between browser and node without any ifs for detecting that around the http requests.

@mhart the goal was there from the beginning, but we updated the docs to prevent future maintainers from forgetting that ;)

[mhart]

I don't want to be the party pooper here, but If it's not consistent with how request handles json option...

I'm not sure I understand what you mean by "it" here? 🤔 How would you be being a party pooper with this PR? This pull request makes xhr like request – xhr's json option is different from request's and always has been since I've been using it (a few years now).

(which I've been fine with – I chose xhr as a lightweight callback mechanism around XMLHttpRequest, not cause it was like request)

I'm not against changing the API if you want to make it all more like request (assuming it doesn't add bloat) – but of course any breaking changes like this should be in a major version (which, I didn't notice until now, but @bjoerge pointed this out in the description of the PR so I don't even know why I was concerned about it in the first place 😸 )

[naugtur]

Uh, ok. Looks like I got confused.

I'll get my act together tomorrow. Now it's already getting late ;)

I'm happy to be wrong about what I wrote 😅

[TehShrike]

I think @naugtur was just responding to my "I'd be fine if xhr didn't maintain request compatibility any more" statement there.

I don't personally use xhr as a drop-in for request, but it's totally a reasonable goal.

And with that goal, this pull request looks like a must-have to me!

[mhart]

Yeah, I can't help but feel the "make it a drop-in replacement for request" goal must be a little new – the original 1.x API certainly had many differences from request.

Maybe it's worth, as @TehShrike said, making this a more clearer goal for 3.x and actually going through the API to see what else differs?

[TehShrike]

The best way towards guaranteeing request compatibility would be adding tests that assert request-like behavior, whether they're passing right away or not.

Perhaps we could even find a way to run request's test suite against this library?

[naugtur]

Ok, I'm on the same page with you guys.

Marking this as v3

[naugtur]

based on @wesleytodd 's idea from #134 I took this and made it backward-compatible.

thanks for all your input guys. please go to #139 and share your opinion about versioning the change.