-
Notifications
You must be signed in to change notification settings - Fork 522
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 multiple objects #357
base: master
Are you sure you want to change the base?
Conversation
Hi @rajlomror, the solution that you did is for two objects not for multiple objects |
@@ -0,0 +1,14 @@ | |||
~~~ javascript | |||
const mergeObjects = (obj1, obj2) => Object.assign(obj1, obj2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const mergeObjects = (obj1, obj2) => Object.assign(obj1, obj2); | |
const mergeObjects = (...objs) => Object.assign({}, ...objs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good solution, but better not using too much spreading as it can kill performance.
Both our solutions can be added in that markdown file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, is just a suggestion, please, add the best possible solution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can consider this solution instead :
const mergeObjects = (...objects) => objects.reduce((lookup, obj) => { Object.keys(obj).forEach(key => { lookup[key] = obj[key]; }); return lookup; }, {});
console.log(mergeObjects({a: 2, b: 4}, {c: 1})); // { a: 2, b: 4, c: 1 }
Hey, @rajlomror, that is a great use of JavaScript! However, I'm not sure if using a build-in construct should be considered a utility function. Why not use it directly? What is there extra? @elkarouani, the function you wrote works well and while it cannot be optimized by the VM (like native ones) it's actually quite OK-performant. One thing to keep in mind is that this ignores all symbol properties which object spread would keep. |
I think this utility can be helpful in some cases where you want to manipulate or organize objects.
Thank you @robinpokorny for reviewing my solution, do you have an example for that point of ignoring symbol properties so I can understand it more ? |
@elkarouani, sure, look at this code: const o = {
a: 1,
[Symbol.toPrimitive]: () => 2,
};
const p = {
a: 3,
};
const r = { ...o, ...p };
const mergeObjects = (...objects) => objects.reduce((lookup, obj) => { Object.keys(obj).forEach(key => { lookup[key] = obj[key]; }); return lookup; }, {});
const s = mergeObjects(o, p)
console.log(1 + r)
// -> 2
console.log(1+s)
// -> '1[object Object]' |
Merge multiple objects into an object.