Fix #2061 - Add `update` event #2063

Closed
wants to merge 1 commit into
from

Projects

None yet
@caseywebdev
Collaborator

No description provided.

@hswolff

I dig. 🔪 (that's a shovel, right?)

Nice 😋

@braddunbar
Collaborator

From perusing the use cases presented, this is essentially what the "reset" event is for, or was prior to adding update. I was under the impression that the purpose of update is for using add and remove instead of reset. Isn't this a step backwards in that regard?

Also, assuming that this event is needed, why would you then need to silence it?

@hswolff

reset is only triggered when resetting a collection. Previously if you collection.fetch({add: true}) there would be no event to bind to other than the add event or the success callback. This is also true for the update() method as well.

@hswolff

Re: why would you need to silence the event - I assume the same reason you'd want to silence other events would hold true for the update event as well: to silence events from being triggered to augment the behavior of your application.

@vincentbriglia

I would think that with the update method you would just listen to the general change event to update your view or add event of a model on a collection view to add it to the DOM, rather than listening to an 'update' event on the collection. In any case, I don't think there's anything an update event adds except for confusion.

@tgriesser
Collaborator

I agree with @vincentbriglia and @braddunbar about adding confusion. I think that if both the update and reset methods are going to exist side by side, and you want a general event to be fired for all of your updates, you should be using (and listening to) reset - if you want the add/remove listeners, use update.

@hswolff

The problem an update event solves relates to when you call collection.fetch({update: true}). There is no event fired.

Contrast to when you call collection.fetch(). A reset event is fired.

This seems inconsistant. Both methods should fire an event when they have completed.

@tgriesser
Collaborator

Calling collection.fetch({update:true}) fires all sorts of add/remove events that aren't fired when you call collection.fetch().

@hswolff

True, however those events pertain to the individual models being updated.

The reset event pertains to the collection as would an update event.

@vincentbriglia

can you expand on http://jsfiddle.net/gbJDw/11/ and see why or what you would use the update event for. I can manage everything with the reset, add and change events

@hswolff

Expanded your code here: http://jsfiddle.net/hswolff/hv3n7/6/ I modified the ItemsView and simulated the update event at the bottom. I also added the moreItemArrayAdd array.

The main drawback to your method of view insertion is that on each add event you are accessing and manipulating the DOM. This results in repaints and reflows in the browser which is non-performant. Refer to https://developers.google.com/speed/articles/reflow for greater details.

With the update event you can batch the insertion of new views, reducing reflows and improving the performance of your web page.

Also the update event alongside the new request event allows you to take action before and after your collection fetch's. This allows you to add a loading indicator while the request is being made and remove it once it has finished updating.

@meleyal

+1, this would make paginating / batch rendering a collection both easier and more efficient, as @hswolff describes.

@wyuenho

+1 for this provided PR #2083 gets merged.

@akre54
Collaborator

+1 even without #2083. I use paginating in several places, and I'd much rather listen for update than attach the success handler or add logic to my reset listener.

@antonzaytsev

+1 good idea

@jashkenas
Owner

Let's not confuse things by minting new events here. If you want the efficient version, using the old-style reset is a fine way to go. If you want the granular events, use update. And if you want to use update, but still render after the fact in bulk, a little bit of debounce does the trick nicely.

collection.on('add remove change', _.debounce(bulkRender))
@jashkenas jashkenas closed this Jan 16, 2013
@caseywebdev caseywebdev deleted the caseywebdev:update branch Jan 16, 2013
@mateusmaso

+1 for "update" event, _.debounce workaround is async and not always desired.

@foxbunny

+1 for "update", that's one of those things that feel like they should have already been 'minted' a long time ago. I never understood why it didn't exist.

@skotchio

+1 for 'update' event also. Because _.debounce(bulkRender) is not enough to get desired result. We need to add a bunch the following code:

onAdd: function (model) {
  this.newModels || (this.newModels = []);
  this.newModels.push(this.renderModel(model));
  this.appendNewModels.call(this);
},

appendNewModels: _.debounce(function () {
   this.$el.append(this.newModels);
  this.newModels = [];
})

It looks ugly and is not Backbone.js style.

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