jashkenas / jashkenas/backbone
Rethink parsing (Collection#set doesn't intelligently merge unparsed models)
- Dominant language
- JavaScript
- Stars
- 28.1k
- Forks
- 5.3k
- PR merge metrics
- No merged PRs in 30d
Description
`Collection#set` takes an option `parse` that, if set to true, will [call the collection's `parse` method on the input array](https://github.com/jashkenas/backbone/blob/master/backbone.js#L812), as well as the [model's `parse` method on individual objects in the input array](https://github.com/jashkenas/backbone/blob/master/backbone.js#L837). This implies that it's ok for non-parsed data to be passed into `Collection#set`.
However, if non-parsed data is passed to `Collection#set`, [line 833](https://github.com/jashkenas/backbone/blob/master/backbone.js#L833) may fail to correctly merge the new data with existing data in the collection (in particular, if the non-parsed data is wrapped in a way that obscures the idAttribute).
Below is a failing test case that illustrates this issue:
```
test("collection should merge in duplicate raw objects with {merge: true}", 1, function() {
var Model = Backbone.Model.extend({
parse: function(data) { return data.wrapper; }
});
var Col = Backbone.Collection.extend({model: Model});
var col = new Col;
col.set([{wrapper: {id: 1, name: 'Foo'}}], {parse: true, merge: true});
var firstModel = col.first();
col.set([{wrapper: {id: 1, name: 'Bar'}}], {parse: true, merge: true});
var secondModel = col.first();
strictEqual(firstModel, secondModel);
});
```
The `{merge: true}` option indicates that the updated model in the collection should be the same object reference, but instead it is a newly instantiated model.
A potential fix might involve changing [line 833](https://github.com/jashkenas/backbone/blob/master/backbone.js#L833) from this:
```
if (existing = this.get(attrs)) {
```
to this:
```
existing = this.get(attrs);
if (!existing && !this._isModel(attrs) && options.parse) {
var tempModel = new this.model(attrs, options);
existing = this.get(tempModel.attributes);
}
if (existing) {
```
although there may be a more elegant solution. If people agree that this is a bug, I can open a pull request.
Contributor guide
Research direction
Start in backbone.js around lines 812, 833, and 837, then run the supplied Collection#set test case with parse and merge enabled. Trace how the wrapped id is resolved and verify that repeated raw input updates the existing model reference rather than creating a second model.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- javascript
- Domain
- frontend, web-dev
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100