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.clone() #1239

Merged
merged 2 commits into from Apr 23, 2012

Conversation

Projects
None yet
4 participants
@philfreo
Contributor

philfreo commented Apr 19, 2012

Models have a .clone() and it seems like you should be able to clone a collection, so that you can operate on a copy of the collection / list of models separately (even though both would reference the same model objects)

@jashkenas

This comment has been minimized.

Owner

jashkenas commented Apr 23, 2012

Before we merge this -- what was your use case where you found yourself needing this?

@philfreo

This comment has been minimized.

Contributor

philfreo commented Apr 23, 2012

I needed to temporarily operate on a collection/list of models (add and remove them), but didn't want to affect the original collection/list until the user was sure they wanted to commit & save their changes. Basically needed a way to have a cloned copy of the collection :)

jashkenas added a commit that referenced this pull request Apr 23, 2012

@jashkenas jashkenas merged commit 04a778c into jashkenas:master Apr 23, 2012

@Yahasana

This comment has been minimized.

Yahasana commented on 7eca8c7 Apr 24, 2012

this code actually share the same models. see
http://jsbin.com/uwadag/edit#javascript,html,live

    clone: function() {
        var models = [];
        this.each(function(v){models.push(v.clone())});
        return new this.constructor(models);
    }

This comment has been minimized.

Collaborator

braddunbar replied Apr 24, 2012

Yes, but that's ok. Backbone collections still function properly when sharing models.

This comment has been minimized.

Yahasana replied Apr 24, 2012

it means if the model(s) of original collection change, the clone collection change too. the clone collection doesn't function as the case philfreo addressed

This comment has been minimized.

Collaborator

braddunbar replied Apr 24, 2012

Why is that? He said he needed a copy of the collection, not the models, right?

This comment has been minimized.

Yahasana replied Apr 24, 2012

He said he needed a copy of the collection, maintain the original state of all the models. user can change any models' attributes value and store it temporarily into a new collection. user can revert all his changes if he like base on the clone collection. if you're not clear, please re-read his statement again, until you find out the exact mean.

This comment has been minimized.

Yahasana replied Apr 24, 2012

but didn't want to affect the original collection/list until ...

this implement mean the new collection share the same models with the original collection's models. if add or remove some models, both collection affect!

This comment has been minimized.

Collaborator

braddunbar replied Apr 24, 2012

this implement mean the new collection share the same models with the original collection's models. if add or remove some models, both collection affect!

That's not true. Adding or removing from the original collection will not affect the models contained in the cloned collection or vice versa.

var original = new Backbone.Collection([{id: 1}, {id: 2}, {id: 3}]);
var clone = new Backbone.Collection(original.models);
clone.pop();
console.log([original.length, clone.length]); // [3, 2]

This comment has been minimized.

Yahasana replied Apr 24, 2012

okay, you are right, add / remove doesn't affect the models in each clone collection, but it's still not good they share the models' attributes.

This comment has been minimized.

Collaborator

braddunbar replied Apr 24, 2012

Not if you need them to be separate, but sharing models (and attributes) can be very useful. For instance, keeping a filtered copy of the same models for display purposes.

This comment has been minimized.

Contributor

philfreo replied Apr 25, 2012

In this case (as @braddunbar pointed out) I was fine with the collections sharing references to the same models, as the cloned collection was only being used to change the list (which models were in the list + ordering), not operate on the models themselves.

I think this pull request makes sense for the default case of collection clone() and I think it can be merged as-is.

For deep cloning, one could either extend the clone() method in a specific collection, or we could have Collection.deepClone() or a parameter like Collection.clone(true). But then keep in mind Model.clone() doesn't technically do a deep clone either, if you have a model attribute that is a reference to an object, like user.get('address') that actually points to an Address model.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment