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

Fix #12 by adding a _.mergeWith customizer function #13

Merged
merged 2 commits into from
Oct 16, 2020

Conversation

moqmar
Copy link
Contributor

@moqmar moqmar commented May 18, 2020

See #12 for details

@moqmar
Copy link
Contributor Author

moqmar commented May 18, 2020

0: moritz will still override existing entries in this approach because it results in { "0": "moritz" }, and Array.isArray correctly returns false for that construct.

@moqmar moqmar mentioned this pull request May 18, 2020
@alexlafroscia
Copy link
Owner

Thanks for the PR! I think you're right in the fact that it makes more sense that arrays are fully merged, rather than some items being replaced the way they are right now.

This would constitute a breaking change in the library, but I am personally OK with that.

const outputJSON = _.merge({}, ...files);
const outputJSON = _.mergeWith({}, ...files, (objValue, srcValue, key, object, source, stack) => {
if (Array.isArray(objValue) && Array.isArray(srcValue)) return [...objValue, ...srcValue];
return undefined; // handle it just like with _.merge
Copy link
Owner

Choose a reason for hiding this comment

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

Just to make sure I understand, returning undefined means "do whatever lodash would normally do by default" and not "replace with undefined", right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, exactly, that's according to their specification.

Choose a reason for hiding this comment

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

This now breaks my use cases so I am sticking to 2.0. Would it have been better to provide a customizer fn as a prop for the node package for custom use cases such as these.

@alexlafroscia
Copy link
Owner

Since we're changing some behavior here, I would really appreciate it if you could add a test case! That would help make sure that it's really clear how this library merges arrays.

@moqmar
Copy link
Contributor Author

moqmar commented May 20, 2020

Sure, I'll try to do this in the next couple of days.

@thewizarodofoz
Copy link

Hey @alexlafroscia @moqmar any updates on this? We would really love to see thing merged as we are blocked by currenty behaviour

@alexlafroscia
Copy link
Owner

Not from me -- still waiting for some tests!

@moqmar
Copy link
Contributor Author

moqmar commented Oct 16, 2020

Whoops, totally forgot about this - I've added a test case for this now. 🙃

@alexlafroscia alexlafroscia merged commit 19dae8b into alexlafroscia:master Oct 16, 2020
@alexlafroscia
Copy link
Owner

Thanks @moqmar !

@alexlafroscia
Copy link
Owner

v3.0.0 released with this change 🎉

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.

4 participants