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.

Sign up for GitHub

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

adding feedback to update event of collection #3711

Closed
wants to merge 19 commits into from
Closed

adding feedback to update event of collection #3711

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
f55d3b5
Update collection.js
Jul 13, 2015
c45b27f
adding feedback to update event of collection
Jul 13, 2015
57db882
Merge branch 'master' of https://github.com/elgubenis/backbone
Jul 13, 2015
1dacae8
update events of remove/set
Jul 13, 2015
434fcbb
Merge branch 'develop'
Jul 13, 2015
cea9ed1
update typos in collection tests
Jul 13, 2015
2929423
remove and set, update event returns changes
Jul 13, 2015
6298647
close feedback
Aug 17, 2015
c5db447
Merge remote-tracking branch 'origin/develop'
Aug 17, 2015
2c3a1e8
#3711 - fix tests
Aug 17, 2015
41c04c5
use strictEqual
Aug 17, 2015
077d19e
use more strictEqual
Aug 17, 2015
108ab3b
collection set, changes: removed, added, merged
Aug 19, 2015
79dae25
collection set, trigger merge
Aug 19, 2015
86740b6
Merge branch 'master' of https://github.com/jashkenas/backbone
Aug 19, 2015
b13581d
collection set, update returns feedback
Aug 19, 2015
6911ecc
removedModels attr when removing model from set
Aug 19, 2015
7e862eb
adds 2 tests for checking merged models on set
Aug 19, 2015
c9c1ed6
sets update event should not merge models existing
Aug 19, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions backbone.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -799,6 +799,7 @@
var singular = !_.isArray(models);
models = singular ? [models] : _.clone(models);
var removed = this._removeModels(models, options);
if (removed.length) options.removedModels = removed;
if (!options.silent && removed) this.trigger('update', this, options);
return singular ? removed[0] : removed;
},
Expand Down Expand Up @@ -890,15 +891,19 @@
// Silently sort the collection if appropriate.
if (sort) this.sort({silent: true});

// Unless silenced, it's time to fire all appropriate add/sort events.
// Unless silenced, it's time to fire all appropriate add/update/sort events.
if (!options.silent) {
var addOpts = at != null ? _.clone(options) : options;
for (var i = 0; i < toAdd.length; i++) {
if (at != null) addOpts.index = at + i;
(model = toAdd[i]).trigger('add', model, this, addOpts);
}
if (toAdd.length || toRemove.length) {
if (toAdd.length) options.addedModels = toAdd;
if (toRemove.length) options.previousModels = toRemove;
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not just always set addedModels and previousModels on options and trigger updates only if theres a length change. Seems mostly harmless (actually better IMO as the user won't have to check if toAdd is an array or not)

If in agreement requires a simple test

Copy link
Author

Choose a reason for hiding this comment

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

i saw the same behavior in the set method, previousModels is only set when there are previousModels, so i just copied that, normally i would also be in favor of having it all the time, but i didnt want to change how the other methods do work.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The only other previousModels always attaches. I think attaching an empty array here is fine as well.

Also, can we rename this removedModels? previousModels makes sense in #reset where every model is "previous", but here only a subset are being removed.

Copy link
Author

Choose a reason for hiding this comment

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

ok, thank you for your feedback

this.trigger('update', this, options);
}
if (sort || orderChanged) this.trigger('sort', this, options);
if (toAdd.length || toRemove.length) this.trigger('update', this, options);
}

// Return the added (or merged) model (or models).
Expand Down
104 changes: 99 additions & 5 deletions test/collection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1626,31 +1626,31 @@
collection.add([{id: 3}], {at: '1'});
deepEqual(collection.pluck('id'), [1, 3, 2]);
});
test("adding multiple models triggers `set` event once", 1, function() {
test("adding multiple models triggers `update` event once", 1, function() {
var collection = new Backbone.Collection;
collection.on('update', function() { ok(true); });
collection.add([{id: 1}, {id: 2}, {id: 3}]);
});

test("removing models triggers `set` event once", 1, function() {
test("removing models triggers `update` event once", 1, function() {
var collection = new Backbone.Collection([{id: 1}, {id: 2}, {id: 3}]);
collection.on('update', function() { ok(true); });
collection.remove([{id: 1}, {id: 2}]);
});

test("remove does not trigger `set` when nothing removed", 0, function() {
test("remove does not trigger `update` when nothing removed", 0, function() {
var collection = new Backbone.Collection([{id: 1}, {id: 2}]);
collection.on('update', function() { ok(false); });
collection.remove([{id: 3}]);
});

test("set triggers `set` event once", 1, function() {
test("set triggers `update` event once", 1, function() {
var collection = new Backbone.Collection([{id: 1}, {id: 2}]);
collection.on('update', function() { ok(true); });
collection.set([{id: 1}, {id: 3}]);
});

test("set does not trigger `set` event when nothing added nor removed", 0, function() {
test("set does not trigger `update` event when nothing added nor removed", 0, function() {
var collection = new Backbone.Collection([{id: 1}, {id: 2}]);
collection.on('update', function() { ok(false); });
collection.set([{id: 1}, {id: 2}]);
Expand All @@ -1671,4 +1671,98 @@
collection.invoke('method', 1, 2, 3);
});

test("#3711 - remove's `update` event returns one removed model", function() {
var model = new Backbone.Model({id: 1, title: 'First Post'});
var collection = new Backbone.Collection([model]);
collection.on('update', function(context, options) {
if (options.removedModels[0] !== model) {
ok(false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Try just ok(options.removedModels[0] === model) or even better, strictEqual(options.removedModels[0], model) instead of this if block.

Copy link
Author

Choose a reason for hiding this comment

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

like that? !strictEqual(options.removedModels[0], model) i guess?

Copy link
Collaborator

Choose a reason for hiding this comment

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

notStrictEqual()

} else {
ok(true);
}
});
collection.remove(model);
});

test("#3711 - remove's `update` event returns multiple removed models", function() {
var model = new Backbone.Model({id: 1, title: 'First Post'});
var model2 = new Backbone.Model({id: 2, title: 'Second Post'});
var collection = new Backbone.Collection([model, model2]);
collection.on('update', function(context, options) {
var removedModels = options.removedModels;
if (!removedModels || removedModels.length !== 2) ok(false);
if (removedModels.indexOf(model) > -1 && removedModels.indexOf(model2) > -1) {
ok(true);
} else {
ok(false);
}
});
collection.remove([model, model2]);
});

test("#3711 - set's `update` event returns one added model", function() {
var model = new Backbone.Model({ id: 1, title: 'First Post'});
var collection = new Backbone.Collection();
collection.on('update', function(context, options) {
var addedModels = options.addedModels;
if (!addedModels || addedModels.length !== 1) ok(false);
if (addedModels[0] === model) {
ok(true);
} else {
ok(false);
}
});
collection.set(model);
});

test("#3711 - set's `update` event returns multiple added models", function() {
var model = new Backbone.Model({ id: 1, title: 'First Post'});
var model2 = new Backbone.Model({id: 2, title: 'Second Post'});
var collection = new Backbone.Collection();
collection.on('update', function(context, options) {
var addedModels = options.addedModels;
if (!addedModels || addedModels.length !== 2) ok(false);
if (addedModels[0] === model && addedModels[1] === model2) {
ok(true);
} else {
ok(false);
}
});
collection.set([model, model2]);
});

test("#3711 - set's `update` event returns one removed model", function() {
var model = new Backbone.Model({ id: 1, title: 'First Post'});
var model2 = new Backbone.Model({id: 2, title: 'Second Post'});
var model3 = new Backbone.Model({id: 3, title: 'My Last Post'});
var collection = new Backbone.Collection([model]);
collection.on('update', function(context, options) {
var removedModels = options.previousModels;
if (!removedModels || removedModels.length !== 1) ok(false);
if (removedModels[0] === model) {
ok(true);
} else {
ok(false);
}
});
collection.set([model2, model3]);
});

test("#3711 - set's `update` event returns multiple removed models", function() {
var model = new Backbone.Model({ id: 1, title: 'First Post'});
var model2 = new Backbone.Model({id: 2, title: 'Second Post'});
var model3 = new Backbone.Model({id: 3, title: 'My Last Post'});
var collection = new Backbone.Collection([model, model2]);
collection.on('update', function(context, options) {
var removedModels = options.previousModels;
if (!removedModels || removedModels.length !== 2) ok(false);
if (removedModels[0] === model && removedModels[1] === model2) {
ok(true);
} else {
ok(false);
}
});
collection.set([model3]);
});

})();