-
Notifications
You must be signed in to change notification settings - Fork 28
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
Use built-in Backbone.Collection.prototype.set #23
Conversation
... which already performs intelligent adds/updates/removes.
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 |
@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? |
No but I can run them. The collection.set([...]) function performs an intelligent On Thursday, July 2, 2015, Bret Little notifications@github.com wrote:
|
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. |
@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:
Thoughts? |
@blittle Thank you for looking into it and helping me understand the difference. It sounds like the main difference is (obviously) that the default |
@colllin yeah, backbone-nested-models just adds a bit more magic for merging deep objects :) |
@blittle Ok, I looked into this for a while this morning. It's a tricky situation. You invented this plugin before In case you're not familiar with the You can modify the behavior by passing in options to Ok, so your Understanding all of this, it seems that what this PR is really about is upgrading to the normal I see a couple paths forward (other than the trivial solutions of doing nothing or forking the project):
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. |
@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. |
…nested collection updates, and passes along the options so that the default `collection.set()` behavior can be modified.
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 |
Sweet! Thanks @blittle for taking the time to review & accept this PR! |
Thank you for submitting the PR :) |
... which already performs intelligent adds/updates/removes.