{"action":"created","comment":{"url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/pulls/comments/89656489","pull_request_review_id":10201566,"id":89656489,"diff_hunk":"@@ -82,7 +83,7 @@ module.exports = function (xhr) {\n       // For older servers, emulate JSON by encoding the request into an HTML-form.\n       if (options.emulateJSON) {\n           params.headers['content-type'] = 'application/x-www-form-urlencoded';\n-          params.body = params.json ? {model: params.json} : {};\n+          params.body = params.json ? {model: params.body || params.json} : {};","path":"core.js","position":15,"original_position":15,"commit_id":"852626b06bf21175c93ea8030b0a770e39a3adb1","original_commit_id":"852626b06bf21175c93ea8030b0a770e39a3adb1","user":{"login":"wraithgar","id":36607,"avatar_url":"https://avatars.githubusercontent.com/u/36607?v=3","gravatar_id":"","url":"https://api.github.com/users/wraithgar","html_url":"https://github.com/wraithgar","followers_url":"https://api.github.com/users/wraithgar/followers","following_url":"https://api.github.com/users/wraithgar/following{/other_user}","gists_url":"https://api.github.com/users/wraithgar/gists{/gist_id}","starred_url":"https://api.github.com/users/wraithgar/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/wraithgar/subscriptions","organizations_url":"https://api.github.com/users/wraithgar/orgs","repos_url":"https://api.github.com/users/wraithgar/repos","events_url":"https://api.github.com/users/wraithgar/events{/privacy}","received_events_url":"https://api.github.com/users/wraithgar/received_events","type":"User","site_admin":false},"body":"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.\r\n\r\nIf `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 `{}`;\r\n\r\nFollowing this logic I believe that this line should read:\r\n\r\n```\r\nparams.body = params.json ? { model: params.body } : {};\r\n```","created_at":"2016-11-25T20:50:15Z","updated_at":"2016-11-25T20:50:15Z","html_url":"https://github.com/AmpersandJS/ampersand-sync/pull/77#discussion_r89656489","pull_request_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/pulls/77","_links":{"self":{"href":"https://api.github.com/repos/AmpersandJS/ampersand-sync/pulls/comments/89656489"},"html":{"href":"https://github.com/AmpersandJS/ampersand-sync/pull/77#discussion_r89656489"},"pull_request":{"href":"https://api.github.com/repos/AmpersandJS/ampersand-sync/pulls/77"}}},"pull_request":{"url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/pulls/77","id":95335527,"html_url":"https://github.com/AmpersandJS/ampersand-sync/pull/77","diff_url":"https://github.com/AmpersandJS/ampersand-sync/pull/77.diff","patch_url":"https://github.com/AmpersandJS/ampersand-sync/pull/77.patch","issue_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/issues/77","number":77,"state":"open","locked":false,"title":"xhr@v2.3 compliance","user":{"login":"dhritzkiv","id":1349865,"avatar_url":"https://avatars.githubusercontent.com/u/1349865?v=3","gravatar_id":"","url":"https://api.github.com/users/dhritzkiv","html_url":"https://github.com/dhritzkiv","followers_url":"https://api.github.com/users/dhritzkiv/followers","following_url":"https://api.github.com/users/dhritzkiv/following{/other_user}","gists_url":"https://api.github.com/users/dhritzkiv/gists{/gist_id}","starred_url":"https://api.github.com/users/dhritzkiv/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dhritzkiv/subscriptions","organizations_url":"https://api.github.com/users/dhritzkiv/orgs","repos_url":"https://api.github.com/users/dhritzkiv/repos","events_url":"https://api.github.com/users/dhritzkiv/events{/privacy}","received_events_url":"https://api.github.com/users/dhritzkiv/received_events","type":"User","site_admin":false},"body":"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.\r\n\r\nThis 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:\r\n\r\n```js\r\n//get an ampersand-model somehow…\r\n// and then proceed to create a custom request using `sync` (with a custom `body`),\r\n// which basically proxies the options to xhr\r\n\r\nconst opts = {\r\n    body: {foo: \"bar\"},\r\n    json: true\r\n};\r\n\r\nmodel.sync(\"create\", model, opts);\r\n\r\n//what currently happens is ampersand-sync will set the `json` option \r\n// to the model's data (`model.toJSON()`), which will\r\n// prevent `opts.body` from being used in `xhr`, as `json` is contain's the model's data, and since it's not a boolean, it takes precedence over `body`\r\n\r\n//with this PR, ampersand-sync will now set `params.body` to the model's data.\r\n// 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`).\r\n// 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()`\r\n\r\n```\r\n\r\nIn 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 used to pass data, sometimes it's tricky to remember which property to use (body or data) when switching between `xhr` and `ampersand-sync`.\r\n\r\nOverall, this PR shouldn't introduce any breaking changes.","created_at":"2016-11-25T17:47:14Z","updated_at":"2016-11-25T20:50:15Z","closed_at":null,"merged_at":null,"merge_commit_sha":"949607e67c4168c9bd714a2dc144f295322df9d3","assignee":null,"assignees":[],"milestone":null,"commits_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/pulls/77/commits","review_comments_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/pulls/77/comments","review_comment_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/pulls/comments{/number}","comments_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/issues/77/comments","statuses_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/statuses/852626b06bf21175c93ea8030b0a770e39a3adb1","head":{"label":"dhritzkiv:xhr-compliance","ref":"xhr-compliance","sha":"852626b06bf21175c93ea8030b0a770e39a3adb1","user":{"login":"dhritzkiv","id":1349865,"avatar_url":"https://avatars.githubusercontent.com/u/1349865?v=3","gravatar_id":"","url":"https://api.github.com/users/dhritzkiv","html_url":"https://github.com/dhritzkiv","followers_url":"https://api.github.com/users/dhritzkiv/followers","following_url":"https://api.github.com/users/dhritzkiv/following{/other_user}","gists_url":"https://api.github.com/users/dhritzkiv/gists{/gist_id}","starred_url":"https://api.github.com/users/dhritzkiv/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dhritzkiv/subscriptions","organizations_url":"https://api.github.com/users/dhritzkiv/orgs","repos_url":"https://api.github.com/users/dhritzkiv/repos","events_url":"https://api.github.com/users/dhritzkiv/events{/privacy}","received_events_url":"https://api.github.com/users/dhritzkiv/received_events","type":"User","site_admin":false},"repo":{"id":74623841,"name":"ampersand-sync","full_name":"dhritzkiv/ampersand-sync","owner":{"login":"dhritzkiv","id":1349865,"avatar_url":"https://avatars.githubusercontent.com/u/1349865?v=3","gravatar_id":"","url":"https://api.github.com/users/dhritzkiv","html_url":"https://github.com/dhritzkiv","followers_url":"https://api.github.com/users/dhritzkiv/followers","following_url":"https://api.github.com/users/dhritzkiv/following{/other_user}","gists_url":"https://api.github.com/users/dhritzkiv/gists{/gist_id}","starred_url":"https://api.github.com/users/dhritzkiv/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/dhritzkiv/subscriptions","organizations_url":"https://api.github.com/users/dhritzkiv/orgs","repos_url":"https://api.github.com/users/dhritzkiv/repos","events_url":"https://api.github.com/users/dhritzkiv/events{/privacy}","received_events_url":"https://api.github.com/users/dhritzkiv/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/dhritzkiv/ampersand-sync","description":"Provides sync behavior for updating data from ampersand models and collections to the server.","fork":true,"url":"https://api.github.com/repos/dhritzkiv/ampersand-sync","forks_url":"https://api.github.com/repos/dhritzkiv/ampersand-sync/forks","keys_url":"https://api.github.com/repos/dhritzkiv/ampersand-sync/keys{/key_id}","collaborators_url":"https://api.github.com/repos/dhritzkiv/ampersand-sync/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/dhritzkiv/ampersand-sync/teams","hooks_url":"https://api.github.com/repos/dhritzkiv/ampersand-sync/hooks","issue_events_url":"https://api.github.com/repos/dhritzkiv/ampersand-sync/issues/events{/number}","events_url":"https://api.github.com/repos/dhritzkiv/ampersand-sync/events","assignees_url":"https://api.github.com/repos/dhritzkiv/ampersand-sync/assignees{/user}","branches_url":"https://api.github.com/repos/dhritzkiv/ampersand-sync/branches{/branch}","tags_url":"https://api.github.com/repos/dhritzkiv/ampersand-sync/tags","blobs_url":"https://api.github.com/repos/dhritzkiv/ampersand-sync/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/dhritzkiv/ampersand-sync/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/dhritzkiv/ampersand-sync/git/refs{/sha}","trees_url":"https://api.github.com/repos/dhritzkiv/ampersand-sync/git/trees{/sha}","statuses_url":"https://api.github.com/repos/dhritzkiv/ampersand-sync/statuses/{sha}","languages_url":"https://api.github.com/repos/dhritzkiv/ampersand-sync/languages","stargazers_url":"https://api.github.com/repos/dhritzkiv/ampersand-sync/stargazers","contributors_url":"https://api.github.com/repos/dhritzkiv/ampersand-sync/contributors","subscribers_url":"https://api.github.com/repos/dhritzkiv/ampersand-sync/subscribers","subscription_url":"https://api.github.com/repos/dhritzkiv/ampersand-sync/subscription","commits_url":"https://api.github.com/repos/dhritzkiv/ampersand-sync/commits{/sha}","git_commits_url":"https://api.github.com/repos/dhritzkiv/ampersand-sync/git/commits{/sha}","comments_url":"https://api.github.com/repos/dhritzkiv/ampersand-sync/comments{/number}","issue_comment_url":"https://api.github.com/repos/dhritzkiv/ampersand-sync/issues/comments{/number}","contents_url":"https://api.github.com/repos/dhritzkiv/ampersand-sync/contents/{+path}","compare_url":"https://api.github.com/repos/dhritzkiv/ampersand-sync/compare/{base}...{head}","merges_url":"https://api.github.com/repos/dhritzkiv/ampersand-sync/merges","archive_url":"https://api.github.com/repos/dhritzkiv/ampersand-sync/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/dhritzkiv/ampersand-sync/downloads","issues_url":"https://api.github.com/repos/dhritzkiv/ampersand-sync/issues{/number}","pulls_url":"https://api.github.com/repos/dhritzkiv/ampersand-sync/pulls{/number}","milestones_url":"https://api.github.com/repos/dhritzkiv/ampersand-sync/milestones{/number}","notifications_url":"https://api.github.com/repos/dhritzkiv/ampersand-sync/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/dhritzkiv/ampersand-sync/labels{/name}","releases_url":"https://api.github.com/repos/dhritzkiv/ampersand-sync/releases{/id}","deployments_url":"https://api.github.com/repos/dhritzkiv/ampersand-sync/deployments","created_at":"2016-11-24T00:13:57Z","updated_at":"2016-11-24T00:13:59Z","pushed_at":"2016-11-25T17:44:27Z","git_url":"git://github.com/dhritzkiv/ampersand-sync.git","ssh_url":"git@github.com:dhritzkiv/ampersand-sync.git","clone_url":"https://github.com/dhritzkiv/ampersand-sync.git","svn_url":"https://github.com/dhritzkiv/ampersand-sync","homepage":"http://ampersandjs.com","size":100,"stargazers_count":0,"watchers_count":0,"language":"JavaScript","has_issues":false,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":0,"mirror_url":null,"open_issues_count":0,"forks":0,"open_issues":0,"watchers":0,"default_branch":"master"}},"base":{"label":"AmpersandJS:master","ref":"master","sha":"a69075ac4ddba9cbef6520328aad3fc92c2d0d46","user":{"login":"AmpersandJS","id":6932645,"avatar_url":"https://avatars.githubusercontent.com/u/6932645?v=3","gravatar_id":"","url":"https://api.github.com/users/AmpersandJS","html_url":"https://github.com/AmpersandJS","followers_url":"https://api.github.com/users/AmpersandJS/followers","following_url":"https://api.github.com/users/AmpersandJS/following{/other_user}","gists_url":"https://api.github.com/users/AmpersandJS/gists{/gist_id}","starred_url":"https://api.github.com/users/AmpersandJS/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/AmpersandJS/subscriptions","organizations_url":"https://api.github.com/users/AmpersandJS/orgs","repos_url":"https://api.github.com/users/AmpersandJS/repos","events_url":"https://api.github.com/users/AmpersandJS/events{/privacy}","received_events_url":"https://api.github.com/users/AmpersandJS/received_events","type":"Organization","site_admin":false},"repo":{"id":17788368,"name":"ampersand-sync","full_name":"AmpersandJS/ampersand-sync","owner":{"login":"AmpersandJS","id":6932645,"avatar_url":"https://avatars.githubusercontent.com/u/6932645?v=3","gravatar_id":"","url":"https://api.github.com/users/AmpersandJS","html_url":"https://github.com/AmpersandJS","followers_url":"https://api.github.com/users/AmpersandJS/followers","following_url":"https://api.github.com/users/AmpersandJS/following{/other_user}","gists_url":"https://api.github.com/users/AmpersandJS/gists{/gist_id}","starred_url":"https://api.github.com/users/AmpersandJS/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/AmpersandJS/subscriptions","organizations_url":"https://api.github.com/users/AmpersandJS/orgs","repos_url":"https://api.github.com/users/AmpersandJS/repos","events_url":"https://api.github.com/users/AmpersandJS/events{/privacy}","received_events_url":"https://api.github.com/users/AmpersandJS/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/AmpersandJS/ampersand-sync","description":"Provides sync behavior for updating data from ampersand models and collections to the server.","fork":false,"url":"https://api.github.com/repos/AmpersandJS/ampersand-sync","forks_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/forks","keys_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/keys{/key_id}","collaborators_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/teams","hooks_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/hooks","issue_events_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/issues/events{/number}","events_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/events","assignees_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/assignees{/user}","branches_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/branches{/branch}","tags_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/tags","blobs_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/git/refs{/sha}","trees_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/git/trees{/sha}","statuses_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/statuses/{sha}","languages_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/languages","stargazers_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/stargazers","contributors_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/contributors","subscribers_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/subscribers","subscription_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/subscription","commits_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/commits{/sha}","git_commits_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/git/commits{/sha}","comments_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/comments{/number}","issue_comment_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/issues/comments{/number}","contents_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/contents/{+path}","compare_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/compare/{base}...{head}","merges_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/merges","archive_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/downloads","issues_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/issues{/number}","pulls_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/pulls{/number}","milestones_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/milestones{/number}","notifications_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/labels{/name}","releases_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/releases{/id}","deployments_url":"https://api.github.com/repos/AmpersandJS/ampersand-sync/deployments","created_at":"2014-03-15T23:57:31Z","updated_at":"2016-11-25T19:05:12Z","pushed_at":"2016-11-25T19:05:11Z","git_url":"git://github.com/AmpersandJS/ampersand-sync.git","ssh_url":"git@github.com:AmpersandJS/ampersand-sync.git","clone_url":"https://github.com/AmpersandJS/ampersand-sync.git","svn_url":"https://github.com/AmpersandJS/ampersand-sync","homepage":"http://ampersandjs.com","size":132,"stargazers_count":21,"watchers_count":21,"language":"JavaScript","has_issues":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":25,"mirror_url":null,"open_issues_count":5,"forks":25,"open_issues":5,"watchers":21,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/AmpersandJS/ampersand-sync/pulls/77"},"html":{"href":"https://github.com/AmpersandJS/ampersand-sync/pull/77"},"issue":{"href":"https://api.github.com/repos/AmpersandJS/ampersand-sync/issues/77"},"comments":{"href":"https://api.github.com/repos/AmpersandJS/ampersand-sync/issues/77/comments"},"review_comments":{"href":"https://api.github.com/repos/AmpersandJS/ampersand-sync/pulls/77/comments"},"review_comment":{"href":"https://api.github.com/repos/AmpersandJS/ampersand-sync/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/AmpersandJS/ampersand-sync/pulls/77/commits"},"statuses":{"href":"https://api.github.com/repos/AmpersandJS/ampersand-sync/statuses/852626b06bf21175c93ea8030b0a770e39a3adb1"}}}}
{"actor":{"display_login":"wraithgar"}}