TehShrike / deepmerge

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

Default to only merging plain objects #152

Open TehShrike opened 5 years ago

TehShrike commented 5 years ago

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

@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 commented 4 years ago

I'm leaning towards doing this.

gr2m commented 4 years ago

I agree, keep it simple 💯

TehShrike commented 4 years ago

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

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

I merged this change into the v5 branch

coyoteecd commented 2 years ago

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

RebeccaStevens commented 2 years ago

@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.