Fixes #53 Fixes #44 got headers issues in order

[naugtur]

@latentflip @HenrikJoreteg @dhritzkiv Please take a look if you have a minute.

[pgilad]

Looks good. +1

[dhritzkiv]

Ditto; that's pretty much how I'd have done the JSON vs. not-JSON response handling

[naugtur]

My logic was: default accept is the one that gets default handling. So if someone sets a particular accept header, they want to change it. Regarding other parsers... The behavior we want is to parse json responses and return everything else as text. It wouldn't be a good "developer experience" if sync parsed some response types based on a potentially growing list. I would consider adding such types to the list a breaking change every time. I hear people don't like breaking changes in sync.

I used DEFAULT_ACCEPT to prevent "magic string" occurring twice, but you are right that we should have a better check for json type. I'm not sure if that's what you had in mind, but the header could be application/json; charset=utf-8and we need to match that too.

[edit] After giving it some more thought I think the "parse or not" decision should be made based on the response content-type. I'm also considering letting the user opt-out of that somehow, just in case.

[latentflip]

Aside from your addendum about charset being a potential issue (on which I trust your judgement more than mine), this lgtm

[naugtur]

I might be overdoing this, but I analyzed multiple combinations and relying on a response header would be a very breaking change for a potentially big number of users.

The decision here is surprisingly hard. Some facts first:

1. Setting a default accept header disables cross-domain requests for IE8-9 (IE9 still officially supported), luckily if emulateJSON is not used, sync will put body in xhr's json option, which sets accept header as needed.
2. Remember: xhr will try parsing JSON if json option was used for sending it, so we might sometimes just get a readily parsed object
3. Our users relied on ampersand always parsing JSON and we want to give some users an option to opt-out
4. Responses with no content-type header used to be treated as JSON and changing that would be a breaking change
5. Responses with bogus content-types used to be treated as JSON, so same here.

Based on all that:

• no default accept header
• try parsing string as JSON unless accept header was explicitly set by the user and headers['accept'].indexOf('application/json')!==0

It's as close to the original behavior as possible, and it brings back the option to opt-out of parsing.

If someone wants to veto and force adding a check for response content-type do it quick.

[dhritzkiv]

Could it not be argued that this breaking change is excused by an increase in major version number of this module and any modules that depend on it (ampersand-model, ampersand-rest-collection)? Users have to explicitly upgrade to the latest major version, at which point they would consult the changelog/documentation before upgrading or upon noticing the breaking behaviour. I would vote to go with whatever is the most "right", even if it breaks things (things that can be fixed by being more explicit with the request).

Ultimately, you know best as there are lots of intricacies which I can't fully comprehend.

Note: any of the proposed solutions should work for me as I'm always setting Accept and Content-Type request headers as well as Content-Type response headers. So, as long as ampersand-sync doesn't error by trying to parse non-JSON content as JSON, it's good.

[naugtur]

I just spent 2h getting everything to keep working in old IEs. I'm not thinking straight anymore. Will be back...

[naugtur]

Last blocker here: I'm putting isEmpty check in xhr and releasing it together with some minor fixes. Will test in IE tomorrow and publish. Once it's done, I'm merging this PR and everything else is just simple chores. ;)

[HenrikJoreteg]

@naugtur sounds great, thanks for your hard work on this!

[naugtur]

I made the change in XHR, will give everyone the weekend to comment on PR there, while I'm on a bit of vacation. I want to get new changes to xhr2 published and this PR finalized on Monday.