Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow the HTTP method to be provided directly #73

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ruiramos
Copy link

@ruiramos ruiramos commented Nov 5, 2015

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.

Allows the callee of ampersand-sync to provide the HTTP method to use on the call, and not the "model's intention" (create, update, ...).

Makes ampersand-sync a bit more generic and avoids hacking around just to send a specific type of request.
Needed in order to include the json data even when providing the method name directly.
Also, all the other checks are being done by `type` and not by `method`.
Updating docs
@fyockm
Copy link
Member

fyockm commented Nov 5, 2015

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?

@@ -66,7 +66,7 @@ module.exports = function (xhr) {
}

// Ensure that we have the appropriate request data.
if (options.data == null && model && (method === 'create' || method === 'update' || method === 'patch')) {
if (options.data == null && model && (type === 'POST' || type === 'PUT' || type === 'PATCH')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we already have lodash.includes, would this line be more readable as:

if (options.data == null && model && includes(['POST', 'PUT', 'PATCH'], type)) {

Similarly on L91

@naugtur
Copy link
Member

naugtur commented Nov 5, 2015

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
Copy link
Author

ruiramos commented Nov 5, 2015

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
Copy link

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

@naugtur
Copy link
Member

naugtur commented Nov 5, 2015

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants