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

Already on GitHub? Sign in to your account

add if statement and add passing test #2494

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants
Contributor

runemadsen commented Apr 20, 2013

This has been discussed in documentcloud#1686, but I'm going to try to explain why I think my tiny change matters.

I often find myself overriding the Backbone.sync method, in order to set custom headers, etc. For example, to use the Github API, I need to do something like this:

GitHub.sync = function(method, model, options) {
  var extendedOptions;
  extendedOptions = _.extend({
    beforeSend: function(xhr) {
      xhr.setRequestHeader('Accept', 'application/vnd.github+json');
      if (GitHub.token) {
        return xhr.setRequestHeader('Authorization', "bearer " + GitHub.token);
      }
    }
  }, options);
  return Backbone.sync(method, model, extendedOptions);
};

I often find myself wanting to call the Backbone.sync method directly, so I can use it outside a specific model or collection, like this:

Backbone.sync('read', false, {url : "someurl", data : ""}) 

As discussed in documentcloud#1686, Backbone.sync now requires a model to work. That's because of a single model.trigger call, as backbone.sync will correctly check for the existence model on all other occasions, and pull all of the needed data from the options if they exist. Except for this one line.

First question you might ask: Why not just use jQuery.ajax?

Well, because Backbone.sync handles a lot of stuff for me, and I already extended it to work with the API I'm talking to. Besides that, the functionality to check for whether the model exist is already implemented other places in the Backbone.sync method. This is not changing the way it works already.

Second question you might ask: Why can't you use a model or collection?

An example of this would the Github contents API, which doesn't conform to REST, as it can return an object or an array of objects on the same API endpoint. In this case, I don't know if I need a collection or a model, and I therefore use Backbone.sync to load the result, and then instantiate the needed object.

I made the proposed file changes and added a test that fails without, and passes with my proposed file change.

Let me know what you think.

@tgriesser tgriesser and 1 other commented on an outdated diff Apr 20, 2013

@@ -1433,7 +1433,7 @@
// Make the request, allowing the user to override any Ajax options.
var xhr = options.xhr = Backbone.ajax(_.extend(params, options));
- model.trigger('request', model, xhr, options);
+ if(model) model.trigger('request', model, xhr, options);
@tgriesser

tgriesser Apr 20, 2013

Collaborator

I'd say go with:

if (model && model.trigger === Events.trigger) model.trigger('request', model, xhr, options);

so this can be used with a non-model/collection object as the second argument.

@runemadsen

runemadsen Apr 20, 2013

Contributor

Oh yes, this should obviously be the implementation.

Collaborator

tgriesser commented Apr 20, 2013

For the record, I really like this change (as was evidenced in #1686).

Keeping Backbone.sync defined as standard interface for any ajax/websocket/(*your-persistence-method-here) is convenient, and maintains that its purpose is an interface to deal with requests, permitting anything that can be "stringified" as the second argument.

Anything model/collection specific can and should be taken care of in the respective calling methods (see #2221) and Backbone.sync should be just about the interaction with an api...

This was also brought up in #1927 - and the point made up by @jashkenas:

Sync already doesn't work with arbitrary objects, because they at least have to have a url defined in them -- very unusual for data objects.

For these cases, the url passed in the options object would take care of this.

Owner

jashkenas commented Apr 21, 2013

Alright then -- amend the pull request with proper spacing, and the safety change, and go for it.

Contributor

runemadsen commented Apr 21, 2013

Done! Let me know if it looks alright.

Collaborator

tgriesser commented Apr 22, 2013

So I just patched this in and saw that another check is required now to use it the way I described, because the implementation used to be:

params.data = JSON.stringify(model);

and after #1838 (options sent to toJSON) it's now:

params.data = JSON.stringify(options.attrs || model.toJSON(options));

so I added this check...

var attrs = options.attrs || (_.isFunction(model.toJSON) ? model.toJSON(options) : model);
params.data = JSON.stringify(attrs);

@jashkenas - feel free to revert if you look at it again and don't think it's worth the trouble (I maintain that it adds a nice feature to an otherwise publicly unused Backbone.sync).

@tgriesser tgriesser closed this Apr 22, 2013

Collaborator

braddunbar commented Apr 22, 2013

My apologies for beating a dead horse, but I think it's easy enough to paper over this already. Just wrap your object with a model before passing to Backbone.sync.

var sync = function(method, obj, options) {
  var model = new Backbone.Model(obj);
  return Backbone.sync.call(model, method, model, options);
};

Backbone.sync is a stretched API as it is, and I don't think we should be shoehorning in new usage guarantees unless they will be documented as part of the public interface.

Owner

jashkenas commented Apr 22, 2013

Yes, that's kinda nasty -- and a great example of why we probably shouldn't be doing this. Brad -- feel free to revert.

Collaborator

tgriesser commented Apr 22, 2013

Fair enough... I was thinking that we would document this as a nice new feature on the public interface (sometimes you don't want models/collections, for various reasons), and it decouples Backbone.sync from models & collections (in the same way you can use Backbone.Events stand alone)... but whatever you think is best.

Edit: from the original request

Second question you might ask: Why can't you use a model or collection?

An example of this would the Github contents API, which doesn't conform to REST, as it can return an object or an array of objects on the same API endpoint. In this case, I don't know if I need a collection or a model, and I therefore use Backbone.sync to load the result, and then instantiate the needed object.

Collaborator

braddunbar commented Apr 22, 2013

An example of this would the Github contents API, which doesn't conform to REST, as it can return an object or an array of objects on the same API endpoint. In this case, I don't know if I need a collection or a model, and I therefore use Backbone.sync to load the result, and then instantiate the needed object.

The method I suggested above will work for this use case. The response isn't parsed in any case and you can do with it whatever you like.

Contributor

runemadsen commented Apr 22, 2013

Just to note: The problem I described is solved just by adding the original if statement. The other improvements are probably too much, but they are also unrelated to what I'm trying to accomplish:

Backbone.sync('read', false, {url : "someurl", attr : ""}) 

@jashkenas jashkenas reopened this Apr 24, 2013

Owner

jashkenas commented Oct 10, 2013

Looking at this again, Brad's got a good point -- it would be unexpected if this kinda-worked, but then if you didn't pass data to a call, it suddenly failed. Being unsupported is better than being halfass-supported in this case, I think. But feel free to patch your version to suit.

@jashkenas jashkenas closed this Oct 10, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment