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.

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

Collection.add return value no longer the same as Backbone.Collection's #419

Closed
mlilley opened this Issue Nov 20, 2013 · 5 comments

Comments

Projects
None yet
3 participants
@mlilley

mlilley commented Nov 20, 2013

According to the Backbone docs, Collection.add() returns the added model (or array of models when adding multiple), and the changelog says this changed for all of set, add, remove, and reset in version 1.1 (previous behaviour was to return the collection).

Issue #412 and associated fix 285161e shows some work was done on this to bring backbone-relational into line (specifically for set) in master.

Unfortunately either add didn't get the same love, or the fix didn't work completely -- I'm seeing add return an array in both cases, instead of just the model when adding a single item (using backbone-relational master).

I haven't checked whether set, remove, and reset are likewise affected.

@mlilley

This comment has been minimized.

Show comment
Hide comment
@mlilley

mlilley Nov 20, 2013

Yeah, ok, I'm seeing the issue for all 4 functions.

Here's some quick and dirty code showing the problems:
(change Car to extend Backbone.Model, and the return values differ)

var Car  = Backbone.RelationalModel.extend({ });
var Cars = Backbone.Collection.extend({ model: Car });
var cars = new Cars();

console.log(cars.add({name:'A'}));                  // returns [model] (should not be array)
console.log(cars.add([{name:'B'},{name:'C'}]));     // returns [model,model] (OK)

console.log(cars.remove(cars.at(0)));               // returns [model] (should not be array)
console.log(cars.remove([cars.at(0), cars.at(1)])); // returns [model,model] (OK)

console.log(cars.reset({name:'D'}));                // returns [model] (should not be array)
console.log(cars.reset([{name:'E'},{name:'F'}]));   // returns [model,model] (OK)

var e = cars.at(0), f = cars.at(1);
console.log(cars.set(e));                           // returns [model] (should not be array)
console.log(cars.set([e,f]));                       // returns [model,model] (OK)

mlilley commented Nov 20, 2013

Yeah, ok, I'm seeing the issue for all 4 functions.

Here's some quick and dirty code showing the problems:
(change Car to extend Backbone.Model, and the return values differ)

var Car  = Backbone.RelationalModel.extend({ });
var Cars = Backbone.Collection.extend({ model: Car });
var cars = new Cars();

console.log(cars.add({name:'A'}));                  // returns [model] (should not be array)
console.log(cars.add([{name:'B'},{name:'C'}]));     // returns [model,model] (OK)

console.log(cars.remove(cars.at(0)));               // returns [model] (should not be array)
console.log(cars.remove([cars.at(0), cars.at(1)])); // returns [model,model] (OK)

console.log(cars.reset({name:'D'}));                // returns [model] (should not be array)
console.log(cars.reset([{name:'E'},{name:'F'}]));   // returns [model,model] (OK)

var e = cars.at(0), f = cars.at(1);
console.log(cars.set(e));                           // returns [model] (should not be array)
console.log(cars.set([e,f]));                       // returns [model,model] (OK)
@mlilley

This comment has been minimized.

Show comment
Hide comment
@mlilley

mlilley commented Nov 20, 2013

eostrom added a commit to eostrom/santa that referenced this issue Dec 7, 2013

@PaulUithol

This comment has been minimized.

Show comment
Hide comment
@PaulUithol

PaulUithol Jan 21, 2014

Owner

This was the correct behavior for Backbone 1.0. Backbone 1.1 adds different behavior when 'single' models are added to a collection.

Owner

PaulUithol commented Jan 21, 2014

This was the correct behavior for Backbone 1.0. Backbone 1.1 adds different behavior when 'single' models are added to a collection.

@lsimoneau

This comment has been minimized.

Show comment
Hide comment
@lsimoneau

lsimoneau Feb 14, 2014

Any chance of seeing a new release with this fix? Cheers.

lsimoneau commented Feb 14, 2014

Any chance of seeing a new release with this fix? Cheers.

@PaulUithol

This comment has been minimized.

Show comment
Hide comment
@PaulUithol

PaulUithol Feb 14, 2014

Owner

Yep, I'm looking a couple more issues I'd like to fix before creating a new release. Won't be too long (well, I hope ;).

Owner

PaulUithol commented Feb 14, 2014

Yep, I'm looking a couple more issues I'd like to fix before creating a new release. Won't be too long (well, I hope ;).

nl0 added a commit to nl0/Backbone-relational that referenced this issue Feb 25, 2014

Merge upstream
* upstream/master: (35 commits)
  Upgrade qunit
  Fix subModels not being populated properly on Underscore 1.6.0, due to `_.each` api change.
  Fire change events right away if we're not in a nested scenario. Fixes PaulUithol#427
  Document `store.unregister`
  `store.unregister` now also accepts collections or a model type
  Change `findOrCreate` to pass `parsedAttributes` to `build`, and adjust tests to reflect change.
  Remove unnecessary locking
  Update change log
  Add test for models not being added to store until they get an id
  Change where we listen to `relational:unregister`.
  Only add models with an id to the store. Closes PaulUithol#411
  Fix a bug in the (crude) performance test
  Add a small test for `clear`
  Fix collection return values when setting/removing `[]` and `null`. Ref PaulUithol#419
  Clarify the behavior of `findOrCreate` when it just a receives a scalar value. Ref PaulUithol#399
  Proper return values on collection methods for Backbone 1.1. Closes PaulUithol#419
  Submodels: accommodate multiple 'type' keys for the same submodel. Closes PaulUithol#429
  Hmm, fix spaces/tabs mix in the example
  Update a few version numbers in the docs to 0.8.7
  Backbone-relational 0.8.7
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment