[email protected] compliance

[dhritzkiv]

Recently, xhr clarified its behaviour for the json option, suggesting that json is flipped on/off, and the body is instead set to… the body. This has the side effect of preventing true/false being sent as a value via the json option, but is otherwise backwards compatible.

This PR is mostly to bring ampersand-sync inline with how xhr prefers its data, but also solves one very specific, confusing use case that would fail (without this PR). I'll try to explain it:

//get an ampersand-model somehow…
// and then proceed to create a custom request using `sync` (with a custom `body`),
// which basically proxies the options to xhr

const opts = {
    body: {foo: "bar"},
    json: true
};

model.sync("create", model, opts);

//what currently happens is ampersand-sync will set the `json` option 
// to the model's data (`model.toJSON()`), which will
// prevent `opts.body` from being used in `xhr`, as `json` contain the model's data,
// and since it's not a boolean, it takes precedence over `body`

//with this PR, ampersand-sync will now set `params.body` to the model's data.
// However, when it comes time to invoke `xhr`, `ajaxSettings` is
// built up of `params` and `options` (with the values in `options` taking precedence via `assign`).
// so, in the end the `body` property of `ajaxSettings` –which is what `xhr`will use in `send()`–
// is equal to `opts.body` (which is arguably what we want) and not the value from `model.toJSON()`

In short, this should make .sync options behaviour more congruent with xhr's options behaviour. While using body is not an option specified within ampersand-sync's documentation, and data should probably be the preferred way to pass data, sometimes it's tricky to remember which property to use (body or data) when switching between using xhr and ampersand-sync.

Overall, this PR shouldn't introduce any breaking changes.

[wraithgar]

Trying to think of the condition where params.json would be true, and there would be no params.body. This would result in params.body being set to {model: true} which seems incorrect.

If params.json is true, it means new lines 70 and 71 ran above, which means params.body has to be set to either options.attrs or model.toJSON(options). So at the bare minimum it's already set to {};

Following this logic I believe that this line should read:

params.body = params.json ? { model: params.body } : {};

[dhritzkiv]

Yep, you're right.

The case this was attempting to address was if somebody passed a non-boolean truthy value to json in options. Except now I realize this is json in params, which will always be (true || undefined).

Making this change shortly.

[wraithgar]

This looks great, thanks for staying on top of this change. +1 from me

[naugtur]

Great thinking, this could have been considered a bug before!
Travis seems broken because of missing credentials. If they're not there, how did it work before?

[dhritzkiv]

@naugtur tests have been broken across all Ampersand modules for about a year now :(

I don't have the expertise or access to fix the CI tests.

[fyockm]

👍

[wraithgar]

Tests ran from PRs from external repos will always fail, only tests made from PRs from local branches will run.