Collection#update to reduce waste #1873

Merged
merged 8 commits into from Dec 10, 2012

Conversation

Projects
None yet
8 participants
Collaborator

caseywebdev commented Dec 5, 2012

I created a PR a few days ago about this and (A) the PR became out of sync with my repo and (B) I feel I explained my rationale poorly which lead to its abrupt close. So I'm sorry for the poor re-issue etiquette, but I really think this should be at least discussed.

reset definitely has it's place as a quick way to completely wipe a collection and start over with new models. Often times though, I'd like to update a collection without obliterating my current models but rather firing off helpful add/change/remove events. This allows me to poll the server with fetch and effectively listen for these events without fear of having to recreate any models (wasteful). Here is an example:

var UsersView = Backbone.View.extend({
  delegateEvents: function () {
    Backbone.View.prototype.delegateEvents.apply(this, arguments);
    this.collection.on('add', this.addUser, this);
    this.pollInterval = setInterval(_.bind(this.poll, this), 10000);
  },

  undelegateEvents: function () {
    clearInterval(this.pollInterval);
    Backbone.View.prototype.delegateEvents.apply(this, arguments);
  },

  addUser: function (user) {
    var userView = new UserView({model: user});
    this.$el.append((userView).el);
  },

  poll: function () {
    // Ideally `update` is the default action, but that would be a
    // breaking change so the options object is used for now.
    this.collection.fetch({update: true});
  }
});

var UserView = Backbone.View.extend({
  initialize: function() {
    // Can be fine tuned thanks to `change:attr`, but in general...
    this.model.on('change', this.render, this);
    this.model.on('remove', this.remove, this);
  }
});

var users = new Backbone.Collection();
users.url = '/users';
var usersView = new UsersView({collection: users});
$('body').append(usersView.el);
usersView.poll();

This is not a complete example, but you can see what I'm getting at. The ability to rig a view (and subviews) this way makes for a clean, clear intent as to what should happen when an add/change/remove happens. The only alternative right now is a not-so-pretty reset dance that just doesn't use Backbone.Events to their potential.

Again, I apologize for the duplicate but I would really appreciate some input especially before the release is cut.

Collaborator

akre54 commented Dec 6, 2012

👍 to an update event. I oftentimes find myself extending the fetch success function or other hackarounds to avoid listening to the reset event, and for where the add event gets fired too frequently to be useful.

I'm against an entire update method though. This can easily be accomplished by writing your own fetchMoreUsers method or somesuch:

fetchMoreUsers: (options) ->
  defaults =
    add: true    # append to collection instead of replacing
    merge: true  # if model exists in collection, update its attributes
    data:
      offset: @length # e.g., if you wanted your server to begin serving models from an offset

  options = _.extend defaults, options

  @fetch options

If you use a fetchMore method enough to be useful in several different collections, I'd advise you add it to a base collection class that extends from Backbone.Collection and that your collections in turn inherit from.

Collaborator

caseywebdev commented Dec 6, 2012

Your case doesn't cover removed models. And yes, I always wrap my Backbone Models/Classes in my own subclass, but I feel this would benefit the core.

Collaborator

akre54 commented Dec 6, 2012

That's a good point. I would still like to see it built around Backbone's existing add and remove methods, since this pull seems to be duplicating a lot of functionality. It also iterates over and recreates every model in the collection, regardless of whether the collection or models have been changed.

What if then update was a wrapper around reset (or reset around update), where update by default does a deep merge for models, plus add and remove as necessary?

Take a look at Chaplin's implementation, which makes use of add and remove, and does a good job of avoiding a double-model creation by creating a fingerprint of the collection ids (sidenote: Chaplin's implementation also isn't perfect, so I'll look into it more).

update: function(models, options) {
  var fingerPrint = this.pluck('id').join();
  var ids = _(models).pluck('id');
  var newFingerPrint = ids.join();

  // Only remove if ID fingerprints differ
  if (newFingerPrint !== fingerPrint) {
    // Remove items which are not in the new list
    this.remove(this.reject(function(model){
      return _(ids).include(model.id)
    });
  }
  ... 
}
Collaborator

caseywebdev commented Dec 6, 2012

You're right about creating new models, that was unnecessary (see changes). I used the indexed _byId and _byCid to make this much more efficient.

I think reset and update can co-exist as they both have a distinct purpose. reset completely disregards the current content of the collection and starts fresh. update gracefully notifies with add/change/remove events accordingly. The default update options has {merge: true} (as you suspected it should) as it is intended to..er..update attributes by matching ids and cids. You can pass {merge: false} and no changes will be made to existing models. This PR isn't redefining add or remove, but instead using them to their potential when updating a collection's contents.

Regarding fetch, I feel the default fetch action should be an update (I don't like dumping all of my existing models/views and starting over just because a value or two has changed!), but I don't know how everyone else feels about that.

Owner

jashkenas commented Dec 6, 2012

This is very tempting, but I'm afraid I'm going to leave it be for now. I have a couple of concerns. We need real use cases from real apps for this type of "smart" behavior. Backbone at the moment gives you the functionality you need to build this sort of thing: add these models here, remove these models there, update these others ... pretty easily. But I haven't seen a case where you have a user interacting with a graph of rich models, and you also want them to be added/removed/updated at arbitrary times by polling the server side. So -- talk use cases, not implementation.

@jashkenas jashkenas closed this Dec 6, 2012

Contributor

philfreo commented Dec 6, 2012

But I haven't seen a case where you have a user interacting with a graph of rich models, and you also want them to be added/removed/updated at arbitrary times by polling the server side.

I haven't been paying attention to this conversation, but this part stuck out to me. We use this pattern you're describing all the time! It's what makes our app (http://close.io/) real-time. You're on an object page (a "lead") that has a bunch of nested models on that object ("comments", "contacts", etc.), and if a 2nd user updates the lead by editing a comment, adding a contact, etc. it gets displayed to the 1st in near real-time due to polling. Relies on add/remove/reset/change events for everything.

It's a similar argument for wanting { merge: true } functionality on Collection.reset: documentcloud#1752

Collaborator

caseywebdev commented Dec 6, 2012

I'll briefly go over what prompted me to implement this function. I'm creating a PhoneGap game using Backbone. The game's main menu features a list (Collection) of games (Models) relevant to the user. I periodically poll the server for updates, as other players take their turn (change), new games are added (add), and closed games fall off (remove). update has made it easy to focus on exactly what I want to do when a specific event happens, and not have to comprehend the entirety of the update and pick and choose what needs to happen where.

Similarly, inside of the game, users can join (add), make a move (change), or quit (remove).

The use cases are there, and I feel very strongly that this would be used if available.

@jashkenas jashkenas reopened this Dec 6, 2012

Collaborator

braddunbar commented Dec 6, 2012

While the use cases are certainly valid, the collection is the wrong place for this type of functionality. Views can handle "reset" as they see fit, replacing elements or subviews as necessary. It's not terribly difficult to implement and is specific to the view's behavior. For example, I often use it to update a list of sub views.

Collections have enough primitives and I think introducing more muddies the intent of the API.

Collaborator

tgriesser commented Dec 6, 2012

@braddunbar - that assumes you're using views. I could see functionality this being useful on the server or in other situations where a view isn't the target for the collection change. Keeping this in the collection would make it more reusable.

Collaborator

braddunbar commented Dec 6, 2012

Perhaps, but none of the use cases mentioned thus far have included viewless situations.

Collaborator

caseywebdev commented Dec 6, 2012

Don't you think instead of the example you linked it would be much more succinct to leverage Backbone's events? We're not adding any more events (an 'update' event, but that's not really the important part here), just putting the existing ones that everyone knows and loves to better use.

Also, @braddunbar that code you linked could be simplified from

reset: function() {
  this.collection.each(this.add, this);
  var models = _.pluck(this.views, 'model');
  _.each(_.difference(models, this.collection.models), this.remove, this);
  this.views = this.collection.map(this.findView, this);
  this.$el.append(_.pluck(this.views, 'el'));
}

to

reset: function() {
}

assuming you used an update instead of reset listener.

Collaborator

caseywebdev commented Dec 6, 2012

Perhaps, but none of the use cases mentioned thus far have included viewless situations.

// I want to keep a "recent" collection with recent players who have left the game
var recent = new Backbone.Collection();

// assume bootstrapped data in `players` for the sake of saving a fetch
players.on('remove', function(model) {
  // this will never be called with `reset`!

  // a player has been removed from a game,
  // add the model to the recent players list
  recent.add(model);
});
var interval = setInterval(function() {
  players.fetch({update: true}); // <3 if update defaults to true ;)
}, 10000);

Clear and succinct as to what is happening. Also, simply removing a player myself via players.remove(model) lets me reuse my same hook without having to special case the reset event to run the same code.

Contributor

philfreo commented Dec 6, 2012

It makes sense to have it happen in the collection, because you may have a bunch of views that want to do things based on a single collection.

Collaborator

caseywebdev commented Dec 6, 2012

Yup, and as far as good MVC is concerned this is regarding data/model events, not UI/view events.

Collaborator

akre54 commented Dec 6, 2012

I agree with @philfreo and @caseywebdev. Collection updates don't belong on the view.

Collaborator

braddunbar commented Dec 6, 2012

@caseywebdev says above:

Clear and succinct as to what is happening. Also, simply removing a player myself via players.remove(model) let's me reuse my same hook without having to special case the reset event to run the same code.

This is a good example and, admittedly, not directly a view concern. However, reset still exists and can be called on your collection. If you later call players.reset(...) (as a result of fetch or manually) you will miss the removed models. Since you still have to handle "reset" events, I don't see the benefit.

I understand that you could just never use reset, but this muddies the API considerably. We should choose one strategy or the other.

Collaborator

caseywebdev commented Dec 6, 2012

Since there are already breaking changes ( @jashkenas just introduced one with validate minutes ago), I think the next cut would be an awesome time to override reset with update.

  • update respects all existing add/change/remove events
  • update leaves with the collection in the same state as reset (with the exception of keeping existing models where possible instead of recreating)
    • update() === reset()
    • update(models) ~== reset(models)
  • update matches the reset signature and the options are passed in the same way
  • update reduces wasteful re-instantiation of models that already exist
  • update will never fire sort unless it's needed (thanks to the recent sort necessity check in add), reset will always fire sort, even if nothing has changed in the new collection data

The only con I see is that update will always be slower than reset for obvious reasons. That said, how often is it going to be called in practice? In my case maybe a dozen times a minute...

Thinking about that sort statement more, that means you can actually bind a method to collection.on('sort',...) and reliably only reorder your DOM when it's truly necessary 👍

Collaborator

braddunbar commented Dec 6, 2012

For some background, this was discussed previously in #955. I think @jashkenas summed it up rather well.

Furthermore, you'd have to deal with a number of API choices, most of which have no "right" answer

  • If a model is present in the collection, but not in the response, do you delete it?
  • If a model is present in the response, but not in the collection, do you add it?
  • How does it work for infinite-scroll-style collections, where you're only fetching a window of models from the server?
Collaborator

caseywebdev commented Dec 6, 2012

I would say these can boil down to two options add and remove, both of which default to true.

  • If a model is present in the collection, but not in the response, do you delete it?

    If remove is true, yes, otherwise, no.

  • If a model is present in the response, but not in the collection, do you add it?

    If add is true, yes, otherwise, no.

  • How does it work for infinite-scroll-style collections, where you're only fetching a window of models from the server?

    This is simply a case of {remove: false}, as you'll want to keep the existing previous entries and just add to the list, modifying your url value as necessary, of course.

  • For the missing case regarding merging, this is already possible via {merge: true} (default) or {merge: false}

I'm going to update the commit with these inferences.

Collaborator

tgriesser commented Dec 6, 2012

This seems to be getting too complicated... model.update([], {add:true, remove:false, merge:true})?

I think @braddunbar brought up good points that there are enough edge cases for desired functionality here that a catch-all might not be suitable for the core... it seems it could get tricky with large collections.

Collaborator

caseywebdev commented Dec 6, 2012

I don't see this as that complex?

match local collection to response data (default)

collection.fetch();

don't remove any models

collection.fetch({remove: false});

don't add any models

collection.fetch({add: false});

infinity scroller

collection.fetch({remove: false});

don't merge any changed models

collection.fetch({merge: false});

I think it's safe to assume true for these values because setting any of them to false effectively mismatches your local data with the server data.

Collaborator

tgriesser commented Dec 6, 2012

@caseywebdev you're right, looking at it again it's not too bad, and I can see the upsides on doing this.

For another potential use case - I'm playing around with a side project that's porting jQuery style syntax and Backbone Models/Collections to Titanium Appcelerator... I could see this functionality being quite convenient on TableViews where in a lot of cases you'd want the ability to add/remove rows individually as opposed to calling a reset and blowing away the table data, or having to add in something like this yourself to a base TableView.

Collaborator

caseywebdev commented Dec 6, 2012

This last commit lets you combine any combination of those options to do what you'd expect 😉...

// I only want to merge in the data for the models I have in this collection, no `length` changing!
collection.fetch({add: false, remove: false});

// This is an example of a completely wasted request...
collection.fetch({add: false, remove: false, merge: false});

you'd want the ability to add/remove rows individually as opposed to calling a reset and blowing away the table data, or having to add in something like this yourself to a base TableView.

Exactly! I understand the need for hesitance when pulling in new features, but I honestly believe this will be used and benefited from all around.

The use case we've stumbled upon quite often, is that we tend to use a Backbone collection to store same flavour of models that come from different collections.

The main reasoning behind this is that Backbone Models can exist with the same instance in different collections, which is quite handy when you have a handful of collections that don't know about eachother. Since this is the case, when you update the instance, it will update everywhere. The word 'instance' here is important, because across the application, we might still hold a reference to a model. Doing a regular fetch, will destroy the instance.

To accomplish 'updating' collection this we use the update method from #955 vincentbriglia/backbone@fe4efc2

// we never fetch here, this is a collector
var globalusers = new UsersCollection();

var friends = new FriendsCollection();
friends.fetch({
    update: true,
    success: function (c) {
        globalusers.update(c.models);
    }
});

var followers = new FollowersCollection();
followers.fetch({
    update: true,
    success: function (c) {
        globalusers.update(c.models);
    }
});

this way we can just query the globalusers collection, rather than querying the followers or friends collection for a particular user.

now, however, with the new merge option on the add method, the above snippet we use could be easily rewritten to the following, to achieve the same result

// we never fetch here, this is a collector
var globalusers = new UsersCollection();

var friends = new FriendsCollection();
friends.fetch({
    merge: true,
    success: function (c) {
        globalusers.add(c.models, {
            merge: true
        });
    }
});

var followers = new FollowersCollection();
followers.fetch({success: function(c) {
    merge: true,
    success: function (c) {
        globalusers.add(c.models, {
            merge: true
        });
    }
});

It doesn't change much at all, it's just a different entry-point. Doing this through the Collection.add method seems fair enough everything considered. We will be removing our Collection.update implementation in favour of the add method if it accomplishes the same.

Collaborator

caseywebdev commented Dec 7, 2012

Yea the master version of add with the merge option is what you want for your case. You aren't accounting for any removed models, though as your global collection can only grow with that setup. That could be just what you need, which is great, but the current fetch reset dance doesn't help us who need to account for those removed models.

On Dec 7, 2012, at 5:55 AM, Vincent Briglia notifications@github.com wrote:

The use case we've stumbled upon quite often, is that we tend to use a Backbone collection to store same flavour of models that come from different collections.

The main reasoning behind this is that Backbone Models can exist with the same instance in different collections, which is quite handy when you have a handful of collections that don't know about eachother. Since this is the case, when you update the instance, it will update everywhere. The word 'instance' here is important, because across the application, we might still hold a reference to a model. Doing a regular fetch, will destroy the instance.

To accomplish 'updating' collection this we use the update method from #955 vincentbriglia@fe4efc2

// we never fetch here, this is a collector
var globalusers = new UsersCollection();

var friends = new FriendsCollection();
friends.fetch({
update: true,
success: function (c) {
globalusers.update(c.models);
}
});

var followers = new FollowersCollection();
followers.fetch({
update: true,
success: function (c) {
globalusers.update(c.models);
}
});
this way we can just query the globalusers collection, rather than querying the followers or friends collection for a particular user.

now, however, with the new merge option on the add method, the above snippet we use could be easily rewritten to the following, to achieve the same result

// we never fetch here, this is a collector
var globalusers = new UsersCollection();

var friends = new FriendsCollection();
friends.fetch({
merge: true,
success: function (c) {
globalusers.add(c.models, {
merge: true
});
}
});

var followers = new FollowersCollection();
followers.fetch({success: function(c) {
merge: true,
success: function (c) {
globalusers.add(c.models, {
merge: true
});
}
});
It doesn't change much at all, it's just a different entry-point. Doing this through the Collection.add method seems fair enough everything considered. We will be removing our Collection.update implementation in favour of the add method if it accomplishes the same.


Reply to this email directly or view it on GitHub.

caseywebdev added some commits Dec 5, 2012

Rebasing `update`
Reuse prepared model in Collection#update

Remove extra whitespace

Trigger `update` after Collection#update unless silent

Don't create new models in Collection#update

Remove `slice` call from Collection#update

It isn't necessary since the array isn't being modified. `add` and
`remove` both slice the arrays they are passed, so simply reading
and passing on the pristine array to those functions is harmless.

Simplify excessive ternary in Collection#update

`update` supports simple add/remove/merge options

Cleanup `update`, support for any combination of add/merge/remove

Save a potentially useless `slice` in `update`

Add comments and more tests for Collection#update

Avoid splice and slice, they're too slow!
Collaborator

caseywebdev commented Dec 10, 2012

Another use case in #1877 easily accounted for by update.

caseywebdev added some commits Dec 10, 2012

Pass models in `update` trigger
Let `sort` trigger "sort" in an effort to phase out `reset`

Avoid unneccessary array rebuild in `update`

@jashkenas jashkenas merged commit c6ca928 into jashkenas:master Dec 10, 2012

Collaborator

caseywebdev commented Dec 10, 2012

Right on 👍 thanks @jashkenas

Owner

jashkenas commented Dec 10, 2012

I've merged in this patch, with some significant tweaks here: 104e9ba ... notably:

  • There's no new "update" event. If you want to update, you get all of your granular events like you want.
  • Models are not removed from the collection by default -- you have to remove: true to get that behavior.
  • reset is still the default behavior upon fetch.
  • Unrelated, but I also unified get and getByCid under get.

If you've got a problem with any of those changes, let me know.

Contributor

philfreo commented Dec 10, 2012

Yay!

Contributor

ianstormtaylor commented Dec 11, 2012

This is the most timely pull request ever. Thanks!

@akre54 akre54 referenced this pull request in chaplinjs/chaplin Dec 17, 2012

Merged

Update to Backbone 1.0 (0.9.9 RC) #313

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