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 multiple objects #357

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

rajlomror
Copy link

Merge multiple objects into an object.

@elkarouani
Copy link
Contributor

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);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const mergeObjects = (obj1, obj2) => Object.assign(obj1, obj2);
const mergeObjects = (...objs) => Object.assign({}, ...objs);

Copy link
Contributor

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.

Copy link

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

Copy link
Contributor

@elkarouani elkarouani left a 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 }

@robinpokorny
Copy link
Contributor

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.

@elkarouani
Copy link
Contributor

elkarouani commented Dec 13, 2021

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?

I think this utility can be helpful in some cases where you want to manipulate or organize objects.

@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.

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 ?

@robinpokorny
Copy link
Contributor

@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]'

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

Successfully merging this pull request may close these issues.

None yet

4 participants