search

AmpersandJS / ampersand-sync

Provides sync behavior for updating data from ampersand models and collections to the server.
http://ampersandjs.com
MIT License
20 stars 28 forks source link

Delete `data` after transforming to query string #48

Closed dhritzkiv closed 9 years ago

dhritzkiv commented 9 years ago

For an HTTP GET request, a request body is not required* and redundant when a query string is used.

I'm proposing that data is deleted from the request options, after it has been transformed into a query string, so that it doesn't get passed along to xhr, which would normally also send that data as its body. (see https://github.com/Raynos/xhr/blob/15501698235a8b5c3151e218ad4f2f3fbda7007d/index.js#L106 and https://github.com/Raynos/xhr/blob/15501698235a8b5c3151e218ad4f2f3fbda7007d/index.js#L163 for where this happens).

This may also have the positive side-effect of sending less data, as the (useless/redundant) body isn't sent. Just a guess.

I also added tweaked the tests to use plain objects (see below) that would cause the request to fail, unless data was deleted from options.

Background: I discovered this when I switched to qs@v3 (in my own code) and noticed that xhr's send(body) line would complain "Cannot convert object to primitive value". This was in part because qs@v3 had switched to plain objects (no prototypal methods), and send(body) would try to call toString (or similar) on it (which won't work on a plain object). However, I posited that for GET requests, the body shouldn't have been set anyway, since data was being transformed to a querystring. So, here we are.

*sending a request body also isn't supported/is ignored by many HTTP servers, because AFAIK, it's loosely defined/confusing in the spec. (see http://stackoverflow.com/a/983458)

pgilad commented 9 years ago

This looks good... +1

pgilad commented 9 years ago

@naugtur Lets try to get this in before ver 4.0.0 or decide to close it if it's un-needed

naugtur commented 9 years ago

@dhritzkiv Is it ok if I don't cherrypick the change adding the line with delete? The test needs to change anyway because it doesn't actually check if data field is deleted.

dhritzkiv commented 9 years ago

@naugtur Not sure I completely understand. Is delete options.data not desired behaviour, or is there something else I need to do? Happy to update the code.

naugtur commented 9 years ago

Let me clarify: delete options.data is the change that we want. The modification you did to tests is not needed and now irrelevant, because the tests are rewritten. (they used to depend on a non-feature leftover object that gave you access to xhr options) I have an update for the tests prepared that checks for delete options.data being there.

I merged your PR manually, but since the code is now in different files, I didn't know how to get your name to show up in git blame for the delete options.data line after merge. Sorry.

dhritzkiv commented 9 years ago

@naugtur understood; totally alright! :ok_hand: