Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

collection.add has different behavior for passing in array vs non-array #2497

Closed
wants to merge 11 commits into from

5 participants

@nickfun

Calling add with an array of false values will not create empty objects.

Currently, you can call collection.add with a false (or false-y value) and it result in a no-op

myCollection.add( false ); //  nothing is added to the collection

but this behavior does not extend to adding an array with false-y values

myCollection.add( [false, false] ); // two empty models are created and added to the collection

My pull request changes collection.add so that if an array contains false-y data then empty models will not be created & added.

@akre54
Collaborator

@knowtheory heads up, looks like this line was doubled

@knowtheory
Collaborator

whoops, thanks for pointing that out.

Collaborator

They're different dates, no? Same deal in underscore... edit: nevermind :sparkles:

Collaborator

Yes, the first set of dates should have been 2010-2011 (which is the point at which DocumentCloud Inc. ceased to be, and DocumentCloud was merged into Investigative Reporters & Editors).

Collaborator

Makes sense. Thanks.

@nickfun nickfun closed this
@nickfun

OK wow I messed up this Pull Request. I didn't realize that pushing to my branch again would have an effect here. I'll close this down since it now has a bunch of unrelated stuff. Is it worth opening a new request?

@eordano

I'd say yes. A good practice is to open pull requests from feature branches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 22, 2013
  1. @nickfun

    Added a failing test spec

    nickfun authored
  2. @nickfun
  3. @nickfun

    updated test spec

    nickfun authored
Commits on Apr 26, 2013
  1. @knowtheory
  2. @knowtheory

    Fixing licensing typo.

    knowtheory authored
Commits on May 1, 2013
  1. @caseywebdev

    Fix bug causing mutations while merging with nested models

    caseywebdev authored
    Again, I'm not a huge fan of this solution to the merge + defaults problem, but
    this will work until somone more clever than I figures out what to do or we
    reevaluate the whole existance checking ordeal.
Commits on May 8, 2013
  1. @caseywebdev
  2. @caseywebdev
Commits on May 15, 2013
  1. @nickfun

    unit tests to demo how Backbone handles adding raw objects to collect…

    nickfun authored
    …ions that use a model with default properties
  2. @nickfun
  3. @nickfun

    cleanup of unit tests

    nickfun authored
This page is out of date. Refresh to see the latest.
Showing with 177 additions and 13 deletions.
  1. +8 −5 backbone.js
  2. +169 −8 test/collection.js
View
13 backbone.js
@@ -1,6 +1,7 @@
// Backbone.js 1.0.0
-// (c) 2010-2013 Jeremy Ashkenas, DocumentCloud Inc.
+// (c) 2010-2011 Jeremy Ashkenas, DocumentCloud Inc.
+// (c) 2011-2013 Jeremy Ashkenas, DocumentCloud and Investigative Reporters & Editors
// Backbone may be freely distributed under the MIT license.
// For all details and documentation:
// http://backbonejs.org
@@ -250,7 +251,7 @@
this.attributes = {};
if (options.collection) this.collection = options.collection;
if (options.parse) attrs = this.parse(attrs, options) || {};
- options._attrs = attrs;
+ options._attrs || (options._attrs = attrs);
if (defaults = _.result(this, 'defaults')) {
attrs = _.defaults({}, attrs, defaults);
}
@@ -606,7 +607,7 @@
// Default options for `Collection#set`.
var setOptions = {add: true, remove: true, merge: true};
- var addOptions = {add: true, merge: false, remove: false};
+ var addOptions = {add: true, remove: false};
// Define the Collection's inheritable methods.
_.extend(Collection.prototype, Events, {
@@ -632,7 +633,7 @@
// Add a model, or list of models to the set.
add: function(models, options) {
- return this.set(models, _.defaults(options || {}, addOptions));
+ return this.set(models, _.extend({merge: false}, options, addOptions));
},
// Remove a model, or a list of models from the set.
@@ -662,7 +663,7 @@
// already exist in the collection, as necessary. Similar to **Model#set**,
// the core operation for updating the data contained by the collection.
set: function(models, options) {
- options = _.defaults(options || {}, setOptions);
+ options = _.defaults({}, options, setOptions);
if (options.parse) models = this.parse(models, options);
if (!_.isArray(models)) models = models ? [models] : [];
var i, l, model, attrs, existing, sort;
@@ -676,6 +677,7 @@
// Turn bare objects into model references, and prevent invalid models
// from being added.
for (i = 0, l = models.length; i < l; i++) {
+ if( !models[i] ) continue;
if (!(model = this._prepareModel(attrs = models[i], options))) continue;
// If a duplicate is found, prevent it from being added and
@@ -684,6 +686,7 @@
if (remove) modelMap[existing.cid] = true;
if (merge) {
attrs = attrs === model ? model.attributes : options._attrs;
+ delete options._attrs;
existing.set(attrs, options);
if (sortable && !sort && existing.hasChanged(sortAttr)) sort = true;
}
View
177 test/collection.js
@@ -967,6 +967,22 @@ $(document).ready(function() {
equal(col.length, 1);
});
+ test('merge without mutation', function () {
+ var Model = Backbone.Model.extend({
+ initialize: function (attrs, options) {
+ if (attrs.child) {
+ this.set('child', new Model(attrs.child, options), options);
+ }
+ }
+ });
+ var Collection = Backbone.Collection.extend({model: Model});
+ var data = [{id: 1, child: {id: 2}}];
+ var collection = new Collection(data);
+ equal(collection.first().id, 1);
+ collection.set(data);
+ equal(collection.first().id, 1);
+ });
+
test("`set` and model level `parse`", function() {
var Model = Backbone.Model.extend({
parse: function (res) { return res.model; }
@@ -1101,16 +1117,161 @@ $(document).ready(function() {
});
test("Attach options to collection.", 2, function() {
- var model = new Backbone.Model;
- var comparator = function(){};
+ var model = new Backbone.Model;
+ var comparator = function(){};
- var collection = new Backbone.Collection([], {
- model: model,
- comparator: comparator
- });
+ var collection = new Backbone.Collection([], {
+ model: model,
+ comparator: comparator
+ });
+
+ ok(collection.model === model);
+ ok(collection.comparator === comparator);
+ });
+
+ test("`add` overrides `set` flags", function () {
+ var collection = new Backbone.Collection();
+ collection.once('add', function (model, collection, options) {
+ collection.add({id: 2}, options);
+ });
+ collection.set({id: 1});
+ equal(collection.length, 2);
+ });
+
+ /**
+ * for pull request 2497
+ *
+ * @author nickfun
+ * @date 2013-04-22
+ */
+ test("`add` `false` to a collection is a no-op", 1, function() {
+ var collection = new Backbone.Collection();
+ collection.add( false );
+ ok( collection.length === 0 );
+ });
- ok(collection.model === model);
- ok(collection.comparator === comparator);
+ /**
+ * for pull request 2497
+ *
+ * @author nickfun
+ * @date 2013-04-22
+ */
+ test("`add` an array of `false` is a no-op", 2, function() {
+ var collection = new Backbone.Collection();
+ collection.add( [ false, false, false, undefined, undefined ] );
+ ok( collection.length === 0 );
+ collection.add( [false, undefined, {} ] );
+ ok( collection.length === 1 );
+ });
+
+ /**
+ * Adding raw data to a collection that uses a model with defaults
+ *
+ * This test does not pass on backbone 1.0.0
+ *
+ * @author nickfun
+ * @date 2013-05-15
+ */
+ test("Adds to a collection using JSLO and defaults", function() {
+ var ModelWithDefaults, CollectionForModels, raw1;
+ // Backbone Model that has default properties
+ ModelWithDefaults = Backbone.Model.extend({
+ idAttribute: '_id',
+ defaults: {
+ age: 21,
+ name: 'John Doe',
+ job: 'driver',
+ mother: 'Sue'
+ },
+ initialize: function() {
+ this.set('_id', this.get('name'));
+ }
+ });
+ // A collection that uses the model by default
+ CollectionForModels = Backbone.Collection.extend({
+ model: ModelWithDefaults
+ });
+ // Raw, non-backbone data. The collection will use the default Model (ModelWithDefaults)
+ // and thus the default values will be copied over for properties that we didnt specify.
+ // In this case, `mother` will be "Sue"
+ raw1 = {
+ name: 'Frank',
+ age: 40,
+ job: 'singer'
+ };
+ // start with an empty collection
+ var col = new CollectionForModels();
+ // add the raw data
+ col.add( raw1 );
+ // get a backbone model from the collection
+ var m1 = col.get('Frank');
+ ok( col.length == 1);
+ ok( m1.id == 'Frank');
+ ok( m1.get('age') == 40);
+ ok( m1.get('mother') == 'Sue');
+ // add something that will merge & maybe replace using defaults
+ var raw2 = {
+ name: 'Frank', // this data will merge with object Frank in the collection
+ job: 'boss' // replace current `job` data with 'boss'
+ };
+ col.add( raw2, {merge: true} ); // merge:true is the key here
+ var m2 = col.get('Frank');
+ // ensure that the object merged with the existing one, not create a new object
+ ok( col.length == 1);
+ // has updated `job` data
+ ok( m2.get('job') == 'boss');
+ // we didnt specify new `age` data, so it should still be the same
+ ok( m2.get('age') == 40, 'checking that age wasnt override by defaults'); // problem
+ // this is the problem. The merged object didnt specify `age`, so backbone used
+ // the default data of 21.
+ ok(m2.get('age') != 21, 'checking that age wasnt override by defaults');
+ });
+
+ /**
+ * Same test as above, but now the model does not use defaults.
+ *
+ * This test passes on backbone 1.0.0
+ *
+ * @author nickfun
+ * @date 2013-05-15
+ */
+ test("Adds to a collection using merge without defaults", function() {
+ var ModelWithoutDefaults, CollectionForModels, raw1;
+ // Backbone Model that has default properties
+ ModelWithoutDefaults = Backbone.Model.extend({
+ idAttribute: '_id',
+ initialize: function() {
+ this.set('_id', this.get('name'));
+ }
+ });
+ // A collection that uses the model by default
+ CollectionForModels = Backbone.Collection.extend({
+ model: ModelWithoutDefaults
+ });
+ // Raw, non-backbone data. The collection will use the default Model (ModelWithoutDefaults)
+ // and thus the default values will be copied over for properties that we didnt specify.
+ // In this case, `mother` will be "Sue"
+ raw1 = {
+ name: 'Frank',
+ age: 40,
+ job: 'singer'
+ };
+ // start with an empty collection
+ var col = new CollectionForModels();
+ col.add( raw1 );
+ var m1 = col.get('Frank');
+ ok( col.length == 1);
+ ok( m1.id == 'Frank');
+ ok( m1.get('age') == 40);
+ var raw2 = {
+ name: 'Frank',
+ job: 'boss'
+ };
+ col.add( raw2, {merge: true} );
+ var m2 = col.get('Frank');
+ ok( col.length == 1);
+ ok( m2.get('job') == 'boss');
+ ok( m2.get('age') == 40);
});
});
Something went wrong with that request. Please try again.