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

Allow the source object to be null or undefined #54

Closed
monolithed opened this issue Jan 19, 2017 · 11 comments
Closed

Allow the source object to be null or undefined #54

monolithed opened this issue Jan 19, 2017 · 11 comments

Comments

@monolithed
Copy link

➜ node
> let deepmerge = require('deepmerge');
undefined
> deepmerge({}, undefined)
TypeError: Cannot convert undefined or null to object

Could you provide the same behaviour like Object.assign?

@TehShrike
Copy link
Owner

It's important to realize that deepmerge isn't compatible with Object.assign in several important ways - the first way being the obvious one, where Object.assign isn't recursive, and the second way being that Object.assign returns the first argument that you pass, while deepmerge creates a new target object, and returns that.

You may want to look into deep-assign.

That said, I would be open to returning a clone of the first argument when the second argument is null or undefined. I think it would just take wrapping this block with an if (source != null).

Unless anyone else has any thoughts, I'll say a pull request with tests would be welcome.

@TehShrike TehShrike changed the title Provide backward compatibility with Object.assign Allow the source object to be null or undefined Jan 19, 2017
@monolithed
Copy link
Author

monolithed commented Jan 19, 2017

You may want to look into deep-assign.

Nope, they are so different:

➜ node
> var merge = require('deep-assign')

> merge([1, 2], [2, 3])
[ 2, 3 ]

> merge({a: 1}, {a: [2]})
{ a: { [Number: 1] '0': 2 } }
➜ node
> var merge = require('deepmerge')

> merge([1, 2], [2, 3])
[ 1, 2, 3 ]

> merge({a: 1}, {a: [2]})
{ a: [ 2 ] }

@TehShrike
Copy link
Owner

It's true - deep-assign has optimal compatibility with Object.assign, in that all it does is implement recursion on top of Object.assign. deepmerge opts for merging children with custom logic.

@TehShrike
Copy link
Owner

If nobody is motivated to open pull requests with tests or implementation, I'll close this eventually.

@TehShrike
Copy link
Owner

Should undefined (or maybe even null) be treated like an empty object when passed to the merge function?

If so, should that be the case when it's passed in to the target, or just the source?

See #80

@TehShrike TehShrike reopened this Oct 24, 2017
@macdja38
Copy link
Collaborator

typeof null is an object... so it would probably make sense to treat it like {}. Not sure if we should do the same with undefined though.

@TehShrike
Copy link
Owner

If this library mutates objects by changing its properties (which is what it has done), then I don't like the idea of treating null or undefined as special "empty object" values.

But if this library is moving to an "always clone onto a new object no matter what" paradigm (like I'm pushing it to), it's more understandable.

@TehShrike
Copy link
Owner

I still feel really nervous about adding magical defaults :-x

Cloning is a non-trivial thing to have a mental model for (as you can see by looking back over all the issues and pull requests on this repository), I don't want to try to guess at what people will expect here.

@danmarshall
Copy link

My workaround for this is to use the .all method which takes an array, then use Array.filter(Boolean) to get rid of falsey stuff like undefined:

deepmerge.all([{foo:"foo"}, undefined].filter(Boolean))

@TehShrike
Copy link
Owner

This path has too many scary implications for me. I'm going to close this.

@OmgImAlexis
Copy link

OmgImAlexis commented Aug 15, 2020

For those really needing this I use this as a work around.

const deepmerge = require('deepmerge');
const merge = (first, second) => deepmerge({ object: first }, { object: second }).object;
merge({ }, undefined); // undefined

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

No branches or pull requests

5 participants