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

Default to only merging plain objects #152

Open
TehShrike opened this issue Jun 20, 2019 · 8 comments
Open

Default to only merging plain objects #152

TehShrike opened this issue Jun 20, 2019 · 8 comments

Comments

@TehShrike
Copy link
Owner

So, I regret adding special-case handling for React objects in #76.

This suggested change makes me think – maybe deepmerge should default to only merging plain objects (as in is-plain-obj) instead of the current combination of "typeof === 'object' minus a hardcoded list of special cases".

This would obviously be a breaking change, but I think it would be an improvement.

Consumers could still pass in their own isMergeableObject for their special cases.

Thoughts?

@yordis
Copy link

yordis commented Jul 8, 2019

@TehShrike definitely I wouldn't like this package to have anything to do with React. That pull request is the representation of the consequences of doing so.

With a good breaking changing readme should be enough, explain to people how they could fix the breaking change if they are relying on that particular checking: wrap this function with my own specialized function.

This would obviously be a breaking change, but I think it would be an improvement.

Probably people are not aware that this feature is here, I would be surprised if somebody tried to merge React elements 😄 (but probably somebody did it)

@TehShrike
Copy link
Owner Author

I'm leaning towards doing this.

@gr2m
Copy link
Collaborator

gr2m commented Sep 2, 2019

I agree, keep it simple 💯

@TehShrike TehShrike changed the title Default to only merging plain objects? Default to only merging plain objects Sep 9, 2019
@TehShrike TehShrike added this to the 5.0.0 milestone Sep 9, 2019
@TehShrike
Copy link
Owner Author

If anyone wants to make this change, go ahead and use is-plain-obj instead of is-mergeable-object, and open your PR against the v5 branch 👍

@TehShrike
Copy link
Owner Author

Noticed by @mnespor - we can't quite drop in is-plain-obj in place of is-mergeable-object because we need the default "is mergeable" check to return true for arrays as well as plain objects.

TehShrike added a commit to ArtskydJ/deepmerge that referenced this issue Oct 21, 2019
@TehShrike
Copy link
Owner Author

I merged this change into the v5 branch

@coyoteecd
Copy link

@TehShrike any plans of releasing v5? I hit the issue with non-plain objects as well when merging objects with Promise properties.

@RebeccaStevens
Copy link

@coyoteecd There's no current timeline for v5's release. You can simply configure v4 like follows to get around this issue:

const isPlainObject = require('is-plain-object')

const options = {
	isMergeableObject: isPlainObject
};

const result = deepmerge(target, source, options);

Alternatively you can use deepmerge-ts. The plan currently is that v5 of this library will be heavily influenced by that library.

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