Use built-in Backbone.Collection.prototype.set

[colllin]

... which already performs intelligent adds/updates/removes.

[colllin]

This fixes a bug I encountered in our application. I must admit that I didn't look closely enough at the existing problem to know exactly why this fixed it. But I'm hoping that you can glance at it and see if using Backbone.Collection.prototype.set makes sense instead of the custom logic.

[blittle]

@colllin I'd be happy to merge this if it solves your problem. Honestly I haven't been working on a backbone application in over a year, so I am unfamiliar with how the API has changed. Did you run the tests on this?

[colllin]

No but I can run them.

The collection.set([...]) function performs an intelligent
add/update/remove based on existing models. Is that what you were trying to
accomplish here, or were there any additional goals?

On Thursday, July 2, 2015, Bret Little notifications@github.com wrote:

@colllin https://github.com/colllin I'd be happy to merge this if it
solves your problem. Honestly I haven't been working on a backbone
application in over a year, so I am unfamiliar with how the API has
changed. Did you run the tests on this?

—
Reply to this email directly or view it on GitHub
#23 (comment)
.

[blittle]

Yes, this was an intelligent merge. At the time Backbone did not support smart merging. If the tests pass and we decide to merge this, I'd like to make sure we bump the backbone dependent version to make sure it is compatible.

[sspread]

@colllin @blittle After several hours of trying to figure out why I couldn't update my nested attributes, I ran into this pull request and implemented on my project. This fixed my problem!

[blittle]

@colllin I just ran the tests and they are broken in a couple ways. It looks like backbone 1.2.1 has a different merging algorithm that backbone-nested-models. This therefore would be a breaking API change. Specifically there are 3 merging techniques that are different:

• Should merge new models into a relation which is a collection
• Should merge values into models which already exist in a collection
• Should remove models which no longer exist in a sub collection

Thoughts?

[colllin]

@blittle Thank you for looking into it and helping me understand the difference.

It sounds like the main difference is (obviously) that the default collection.set doesn't know about nested models. I'm actually surprised that it doesn't already handle this appropriately -- collection.set should call model.set on the existing model with the new properties, so if model.set is implemented to handle nested models/collections, this should work out of the box. Maybe it's a subtle disconnect. I'll look into it and get back to you.

[blittle]

@colllin yeah, backbone-nested-models just adds a bit more magic for merging deep objects :)

[colllin]

@blittle Ok, I looked into this for a while this morning. It's a tricky situation.

You invented this plugin before collection.set() was added to Backbone, so there wasn't a standard implementation for you to base yours on. Now there is a standard expected behavior, but the problem is more complicated than just being a different implementation. You've actually implemented two different versions of the Backbone algorithm, depending on whether you pass in a single model or an array/collection.

In case you're not familiar with the collectionA.set(modelsB) algorithm, by default it adds all new models from modelsB to collectionA, merges new attributes from modelsB into any existing models in collectionA, and removes any existing models in collectionA that aren't present in modelsB. This also triggers 'add' and 'remove' events appropriately.

You can modify the behavior by passing in options to collection.set(models, options). The default options are {add: true, remove: true, merge: true}.

Ok, so your setRelation(attr, val, options) implementation essentially does the same thing as collection.set(), but with different options.remove behavior, depending on whether val is a collection/array of models or a single model. If val is a list of models, setRelation performs collection.set(val, {remove: false}). If val is a single model, setRelation performs collection.set([val]), which uses options {remove: true} by default.

Understanding all of this, it seems that what this PR is really about is upgrading to the normal collection.set() behavior, which is probably what users of the latest version of Backbone would expect. That's why this change fixed my "bug" but broke your tests. It actually wasn't a bug at all, but a design difference.

I see a couple paths forward (other than the trivial solutions of doing nothing or forking the project):

1. We switch to using collection.set() while maintaining the original plugin behavior, and providing options for modifying the default setRelation behavior.
2. We switch to using collection.set() with it's default options for all cases, and update the tests to match the new expected behavior. We also provide options for modifying the behavior of setRelation() back to the original plugin behavior for developers who experience breaking changes after the upgrade.

If this were my project, I would probably go for number 2, which seems more promising for the future of the project, since the behavior is more aligned with the default Backbone behavior (now that there is a default behavior). But it's totally up to you.

[blittle]

@colllin Thanks a lot for looking into this. I agree with you, I think it would be better to change to the default merge behavior. I don't use backbone often anymore, so for the benefit of the community I think it'd be better to change the API. I am fine with making a breaking API change as long as the major version is rev-ed. My initial impression is to wait to add backwards compatibility until it is obviously needed. I'll update the tests and merge later today.

[colllin]

Sounds great. Thank you. I just pushed a couple more changes based on what I learned this morning about how you were handling it differently depending on whether a list of models or a single model is passed to setRelation(). Now it treats those both the same (uses default collection.set() behavior), but converts the single model to an array containing a single model since collection.set() expects an array. I also updated it to pass along the options argument to collection.set().

[colllin]

Sweet! Thanks @blittle for taking the time to review & accept this PR!

[blittle]

Thank you for submitting the PR :)