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

#185 change customMerge signature for merging empty values #228

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

Conversation

TheHaff
Copy link

@TheHaff TheHaff commented Jan 27, 2021

@TehShrike original pr: #205
My rebase got in a weird state so i just redid it and cleaned it up a bit

… or empty string overwrites + a sweet little util function customMergeIgnoreEmptyValues for cuteness
@TheHaff TheHaff force-pushed the flexible-custom-merge-signature branch from b965118 to 03a63cc Compare January 27, 2021 16:14
@timberkeley
Copy link

Should this not be

deepmerge.customMergeIgnoreEmptyValues = (key, target, source) => !target || target === ''
	? () => source
	: deepmerge;

otherwise the you only get a shallow copy? Though you could always supply your own function I guess.

@TheHaff
Copy link
Author

TheHaff commented Feb 10, 2021

Should this not be

deepmerge.customMergeIgnoreEmptyValues = (key, target, source) => !target || target === ''
	? () => source
	: deepmerge;

otherwise the you only get a shallow copy? Though you could always supply your own function I guess.

No it's meant to be used as customMerge option. if you look at the test i provided on test/merge.js line: 680. Maybe i should have named it differently? Also i might be misunderstanding what you mean.

@timberkeley
Copy link

No it's meant to be used as customMerge option. if you look at the test i provided on test/merge.js line: 680. Maybe i should have named it differently? Also i might be misunderstanding what you mean.

If you change the test a little:

src = { someNewVariable: { key: "herp"}, very: { nested: { thing: "", another: "derp" } } };
target = { very: { nested: { thing: "derp", another: "" } } };

And run the test with the default merge you will get the overwritten thing :
{ very: { nested: { thing: '', another: 'derp' } }, someNewVariable: { key: 'herp' } }

With the customMergeIgnoreEmptyValues merge function you get:
{ very: { nested: { thing: 'derp', another: '' } }, someNewVariable: { key: 'herp' } }
What I want to achieve is:
{ very: { nested: { thing: 'derp', another: 'derp' } }, someNewVariable: { key: 'herp' } }

It does not look trivial to implement but is also what I expected.

@TheHaff
Copy link
Author

TheHaff commented Feb 12, 2021

No it's meant to be used as customMerge option. if you look at the test i provided on test/merge.js line: 680. Maybe i should have named it differently? Also i might be misunderstanding what you mean.

If you change the test a little:

src = { someNewVariable: { key: "herp"}, very: { nested: { thing: "", another: "derp" } } };
target = { very: { nested: { thing: "derp", another: "" } } };

And run the test with the default merge you will get the overwritten thing :
{ very: { nested: { thing: '', another: 'derp' } }, someNewVariable: { key: 'herp' } }

With the customMergeIgnoreEmptyValues merge function you get:
{ very: { nested: { thing: 'derp', another: '' } }, someNewVariable: { key: 'herp' } }
What I want to achieve is:
{ very: { nested: { thing: 'derp', another: 'derp' } }, someNewVariable: { key: 'herp' } }

It does not look trivial to implement but is also what I expected.

oh crap that's what i want as well. Let me digg a little more.

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.

3 participants