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

Why would deepmerge assign 'undefined' attributes? #53

Closed
ivawzh opened this issue Jan 12, 2017 · 8 comments
Closed

Why would deepmerge assign 'undefined' attributes? #53

ivawzh opened this issue Jan 12, 2017 · 8 comments

Comments

@ivawzh
Copy link

ivawzh commented Jan 12, 2017

Currently deepmerge overwrites non-void attribute with undefined attribute:

$ node
const merge = require('./node_modules/deepmerge')
merge({ a: 'a' }, { a: undefined })
# Result: { a: undefined }

Why is this a desired behaviour?

IMHO, the conventional merge method should skip undefined value. The popular libarary Lodash also aggrees that rule, see http://stackoverflow.com/a/33247597/1401634.

Would you please at least provide an option to turn this behaviour off?

Cheers

@ivawzh ivawzh changed the title Why deepmerge would assign 'undefined' attributes? Why would deepmerge assign 'undefined' attributes? Jan 12, 2017
@TehShrike
Copy link
Owner

deepmerge operates on all properties of the input objects, no matter what their value is.

It may be that most people would assume the opposite - though I see that lodash's assign and extend behave the same way.

Making a breaking change for something fairly opinionated like this seems untenable.

I worry about feature creep, but I could see the argument for this.

I wouldn't be against a pull request adding a new mergeUndefined option.

We could also talk about a general solution - allowing users to pass in a shouldOverwriteSource function that would take the values from both objects, and determine if the merging object should be populated from the source object or the target object. It would default to () => true for backwards compatibility.

Anyone have thoughts?

@macdja38
Copy link
Collaborator

I think if you can gather anything from the lodash example it's that both functionalities can be valuable. undefined is a possibly desirable value just like any other. I support addition of a custom function the user can define to dictate merge functionality, that said assuming the need for undefined is a common case I would also like to see a function that supports it out of the box.

@yvele
Copy link

yvele commented Apr 18, 2019

Is there a workaround or and options not to merge undefined values?

@TehShrike
Copy link
Owner

Yeah, you can pass in a customMerge function: https://github.com/TehShrike/deepmerge#custommerge

Something like

const customMerge = key => (a, b) => b === undefined ? a : deepmerge(a, b)

@yvele
Copy link

yvele commented Apr 18, 2019

@TehShrike unfortunately your suggestion is not working 🤔
But once a working solution has been found, I think I could be interesting to add it in the readme

@yuccai
Copy link

yuccai commented Apr 18, 2019

This should work :

const customMerge = () => (a, b) => {
  const c = Object.keys(b)
    .filter([key, value] => value !== undefined) 
    .reduce((obj, [key, value]) => ({
      ...obj,
     key : value,
    }, {});
  return deepmerge(a, c);
};

@pSnehanshu
Copy link

customMerge never being called #199

@tran-simon
Copy link

@TehShrike Could this issue be reopened? We're having the same issue and the suggested solutions do not work

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

No branches or pull requests

7 participants