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

Allow the HTTP method to be provided directly #73

Open ruiramos opened 8 years ago

ruiramos commented 8 years ago

This PR will allow the ampersand-sync's callee to provide the HTTP method to use in the call directly (instead of forcing you to pass the model's intention).

In REST systems, the need to call custom endpoints is a common one (Eg, sending a POST request to some ...api/:id/refresh). In order to accommodate for this, a good approach would be to implement a custom method on the model, or a more generic function such as: https://gist.github.com/ruiramos/1687be7a7edaa14816ec

This PR intends to remove the need for that unMapMethod map and make ampersand-sync a bit more generic and less dependent on model lingo.

fyockm commented 8 years ago

Makes sense to me, but I'd like to see some updated tests included with the PR. e.g. What happens if you pass in a bad method?

naugtur commented 8 years ago

Method can be provided directly in options that are passed to xhr.

sync(whatever,model, { method: "POST" }) should do.

I don't see why making the first argument ambiguous is beneficial to any usecase. Could you provide your usecase? Maybe you just wanted to invoke xhr directly?

ruiramos commented 8 years ago

Hey @naugtur and thanks for your inputs. I think the use case is pretty clear in the description/gist linked: to be able to extend a model with custom RESTful requests, based on its endpoint.

It's true that xhr could be directly used in the model, or the first parameter simply discarded and overwritten by options, but these doesn't seem better options to me as you lose the common interface and all the extra features sync is supposed to give you -- like triggering request on model, the ability to build query strings on GETs, emulate JSON, etc.

aaronmccall commented 8 years ago

I'm a +1 with tests as per @fyockm's suggestion.

naugtur commented 8 years ago

@ruiramos makes sense to keep the api, I'm just worried it's going to be hard to explain. And sorry I didn't look at the gist, I just jumped in to check if it's something that the options were for, and I still don't see how handling this as a first argument is significantly better.