search

TehShrike / deepmerge

A library for deep (recursive) merging of Javascript objects
MIT License
2.75k stars 216 forks source link

#185 change customMerge signature for merging empty values #228

Open TheHaff opened 3 years ago

TheHaff commented 3 years ago

@TehShrike original pr: https://github.com/TehShrike/deepmerge/pull/205 My rebase got in a weird state so i just redid it and cleaned it up a bit

timberkeley commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.