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

deepmerge and deepmerge.all behave differently with certain options #201

Open
ArtskydJ opened this issue Jul 24, 2020 · 4 comments
Open

Comments

@ArtskydJ
Copy link
Contributor

deepmerge(a, b, opts) and deepmerge.all([a, b], opts) behave differently given certain options.

Repro:

const deepmerge = require(`deepmerge`)

const data1 = {
	a: false,
	b: true,
	c: false,
	d: true,
}
const data2 = {
	a: false,
	b: true,
	c: true,
	d: false,
}

const deepmerge_opts = {
	customMerge: key => (a, b) => a && b,
	isMergeableObject: () => true,
}

const log = o => console.log(JSON.stringify(o))

log(deepmerge(data1, data2, deepmerge_opts))         // {"a":false,"b":true,"c":false,"d":false} (EXPECTED)
log(deepmerge.all([ data1, data2 ], deepmerge_opts)) // {"a":false,"b":true,"c":true,"d":false} (NOT EXPECTED)
@ArtskydJ
Copy link
Contributor Author

I think the root cause is that deepmerge(data1, {}, opts) is assumed to have no effect, which is essentially what happens in the reduce bit of deepmergeAll

@ArtskydJ
Copy link
Contributor Author

here's a potential fix, but it might not work right if the array is <2 elements in length... I'm not sure.

function deepmergeAll(array, options) {
	if (!Array.isArray(array)) {
		throw new Error(`first argument should be an array`)
	}

	return array.slice(1).reduce((prev, next) => deepmerge(prev, next, options), array[0] || {})
}

@TehShrike
Copy link
Owner

Oh hey, that's interesting.

I think for arrays passed in with 2+ elements, the nicest solution would be to just not pass in an initial value to reduce at all, since if you don't supply one, reduce uses the first element in the array.

If we made that change, I think we'd have to handle the 0 and 1 element cases specifically – if the array has 0 elements, return {}, if it has 1 element, return deepmerge({}, array[0], options)

@RebeccaStevens
Copy link

#217 fixes this but I don't know how it does. I ran this test case there and it works correctly (it not fixed #215)

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

3 participants