Collection#create returns result from Model#save

[kimjoar]

It feels natural that all asynchronous methods return an xhr. create is, as far as I can see, the only one that does not. I can see the value in it returning a model, as it might be created from an object within create, but I feel returning an xhr leads to a better API. It's also more consistent with the other async methods. If the model is needed directly after create is called instead of when the event is triggered, I think it's better to create it yourself, call save on it and add it to the collection.

I have not fixed the failing tests as I thought I'd first see what you guys think. (I didn't find any earlier discussion on it, but that might just be my limited searching skills.)

Any thoughts?

[akre54]

👍

[tgriesser]

The issue I see would be the potential return false that could happen if the _prepareModel fails. Then it would be inconsistent in sometimes returning a xhr and (occasionally) sometimes not. Although, it would be more useful/consistent to return the sync, and chaining isn't particularly useful here.

[kimjoar]

Isn't that what save and destroy do today? There are several return false in there at least. That's without a doubt a bad API, but maybe the best option as Backbone does not want to use $.Deferred internally.

[tgriesser]

Ah, you're correct. Then yeah I'd say it's worth fixing up the tests, I agree it'd be more consistent to return the xhr here.

[braddunbar]

See #1155 for some precedent here.

[kimjoar]

@braddunbar Thanks.

It's interesting, to me the other way around seems like the natural choice. If you need a reference create the model yourself, otherwise you'll receive false or xhr, which is consistent which the other async methods. As you say, it feels incorrect now.

However, as this has already been discussed, and if you guys still think the current solution is the best choice, I'm up for closing this issue. :)

[caseywebdev]

This is a have your 🍰 and 👅 it too problem. I vote current functionality because...

var xhr;
var model = coll.create(attrs, {
  beforeSend: function (_xhr)  { xhr = _xhr; }
});

isn't as bad as trying to do it the other way (onceing 'request' on the collection or something...).

[jashkenas]

Yep. create needs to return a reference to the model. That's what it's for.

[tgriesser]

So another thought about this ticket, it could be a potential solution to part of the issue in #2428, where the model returned by collection.create isn't guaranteed to be the same as the model added to the collection if one with the same id already exists.

While it makes sense the collection.create would return the model, the model it returns isn't terribly useful until you know that it's been persisted...

If we go down the path of making consistent use of promises in more places #2489, then this might be a good compromise, to return the jqXHR, chaining and resolving the promise with the model:

 create: function(model, options) {
   options = options ? _.clone(options) : {};
   if (!(model = this._prepareModel(model, options))) return Backbone.Deferred.reject();
   if (!options.wait) model = this.add(model, options);
   var collection = this;
   var success = options.success;
   options.success = function(resp) {
     if (options.wait) model = collection.add(model, options);
     if (success) success(model, resp, options);
   };
   return model.save(null, options).then(function() {
      return model;
   });
 },

so you could do something like:

collection.create({/*attrs*/}, {/*options*/}).then(function(model) {
  // First argument is the actual model added to the collection.
}, function(err) {
  // Failed prepareModel or validation.
});

Just an idea...

[kimjoar]

@tgriesser I've rebased master and updated to return the model. I think all async methods should return a promise in Backbone, but that might also be because I've gradually changed how I write my apps. I now rely less on events and more on "explicitly" handling async code for certain problems.

If you're still interested in this change I can go through the tests and update them too.

[tgriesser]

@kjbekkelund - there are three other changes in the code snipped above as well, model = this.add(model, options); and model = collection.add(model, options); (to ensure we have the correct model added to the collection), and return Backbone.Deferred.reject(); which would be dependent on #2489.

Just wanted to have a bit more discussion on this one (I also didn't check that it works as I described... but I believe it should).

I agree with the consistency of async methods returning a promise.

[kimjoar]

@tgriesser Ah, of course. I guess I was a little too sleepy this morning when I updated the PR. Fixed.

[RStankov]

This change have a potential to break a lot of code. In my mind create should return the new model, since it is the important here. Also create is more near to add, push than save for collections.

Maybe, xhr should be returned only if you pass the wait option, because it will add the model later to the collection.

[RonnyO]

I was all for this change until I saw @caseywebdev tip about beforeSend.
It does the trick and allows using Model#save. Maybe just mentioning the inconsistency + tip in the docs would do.

[yanickrochon]

The problem, as I see it, is that it should always return a promise. It should never return false. When calling Model#save, instead of returning false, the promise should be rejected. This is the idea behind everything returning a promise, to stay consistent.

Of couse, this would break existing code, then again this is what "release notes" are for.

[nikcorg]

In my opinion, collection.create should either return the deferred from the model.save, or not save the model at all and simply act as a shortcut for creating an instance of the type collection.model.

In its current form it always assumes that the create operation is successful (save for the possible failure during validation in _prepareModel, which strangely returns false); failing the op in backbone.sync echoes to deaf ears and the very useful deferred object is discarded in the process.

Perhaps dropping the save would be more "Backbone style". It would totally make sense to me - it would allow for a collection.create(attrs).save() one liner, and most other commenters should remain happy as well since then create would always return the new instance (as it currently does).

Of course, this would drastically change the the create method's documented behaviour, but it would in contrast make it much more obvious and useful.

[yanickrochon]

I totally agree with this.

[Crisfole]

A way old PR, but comment for anyone who really wants something like this:

Add a three line "build" method to Backbone.Collection (or your own BaseCollection...whatever floats your boat). This keeps things orthogonal to Rails, which, while not explicitly stated anywhere, is pretty consistent with the rest of Backbone, and it means no breaking backwards compatibility:

build: function(attributes, options) {
    var built = new this.model(attributes, options);
    this.add(built, _omit(options, 'parse'));
    return built;
}

Note: this isn't polished.

[jashkenas]

Closing this for now -- but if anyone is tackling a pull request that introduces a pervasive use of promises, this would be one of the key changes to make.

[loicraux]

Just came across this issue.. (unable to cancel() a coll.create() request).
I totally agree with @nikcorg 's proposal.