Skip to content
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

Merge two objects recursively #49

Closed
satazor opened this issue Apr 30, 2012 · 11 comments
Closed

Merge two objects recursively #49

satazor opened this issue Apr 30, 2012 · 11 comments
Labels

Comments

@satazor
Copy link
Contributor

satazor commented Apr 30, 2012

At the moment we have mixIn that merges two objects but not recursively.
We could have something similar to mootools Object.merge

@JamesMGreene
Copy link

+1

@millermedeiros
Copy link
Owner

this is indeed useful and not hard to implement. +1

millermedeiros added a commit that referenced this issue May 24, 2012
@millermedeiros
Copy link
Owner

one thing I realized while coding it is that since objects are passed by reference they will end up being updated/edited during the merge, so the proper way of implementing it to avoid undesired side-effects is to clone the object before merging...

what do you guys think? should it also clone arrays?

Current behavior:

var obj1 = {a: 1, b : { c: 1}};
var obj2 = {a: 2, b : { c : { d : 2 } }};
var obj3 = { b : { e : 3 } };

merge(obj1, obj2, obj3); // { a: 2, b : { c : { d : 2 }, e : 3 } }

// since objects are passed by reference this weird behavior happens
// obj2 is also affected during merge
obj1.b === obj2.b; // true
obj2.b.e === 3;    // true

PS: I didn't merged this into master yet since current behavior is funky

@JamesMGreene
Copy link

Yes, I would say both objects and arrays should be cloned. Some people would then say, "what about object/class instances?"; I, however, have never found myself wanting to merge any structure that contains such things... only plain data objects.

@JamesMGreene
Copy link

Also, you may find some of the discussion interesting in this JSFixed thread: https://github.com/JSFixed/JSFixed/issues/16

@satazor
Copy link
Contributor Author

satazor commented Jul 12, 2012

bump!

@millermedeiros
Copy link
Owner

@satazor thanks for the "bump!", I've been busy lately and forgot about it. Merged into master.

@JamesMGreene
Copy link

@millermedeiros Is the committed behavior still funky?

@millermedeiros
Copy link
Owner

@JamesMGreene now it clones all the Object/Array/Date/RegExp, so it shouldn't cause any side-effects. see specs for more info

@JamesMGreene
Copy link

Greats, thanks!

@satazor
Copy link
Contributor Author

satazor commented Jul 15, 2012

@millermedeiros no problem! I was long waiting for this function :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants