Throw error when target and source are different types

[fgarcia]

I've been a while battling a merge problem until I realized that merging an object with an array silently fails.

For example

   let o1 = {
      services: {
        nginx: {
          labels: {
            register: true,
          },
        },
      },
    }

    let o2 = {
      services: {
        nginx: {
          labels: ['enable=true'],
        },
      },
    }

The current result is

{
  services: { nginx: { labels: [ 'enable=true' ] } }
}

Looking at the custom array merge it seems that the object is silently translated into []

I would rather have an error thrown (incompatible source/target) than silently throwing away info

[TehShrike]

This has always been the merge behavior – if the types of the values don't match, the value source object is cloned onto the target.

deepmerge/index.js

Lines 87 to 88 in 7640d50

	if (!sourceAndTargetTypesMatch) {
	return cloneUnlessOtherwiseSpecified(source, options)

[fgarcia]

My issue was to argue that from my perspective this was a silent error where some information is thrown away.

I understand that raising an error now would break compatibility with existing code. Could a hook option to solve a type mismatch be an alternative?

[TehShrike]

What behavior are you imagining for the case where two un-mergeable values exist on the same property?

[fgarcia]

The implementation would be like this:

options.cloneTypeMismatch = option.cloneTypeMismatch || 
  (_target, source, options)  => cloneUnlessOtherwiseSpecified(source,options)

 if (!sourceAndTargetTypesMatch) { 
 	return option.cloneTypeMismatch(target, source, options) 
}

If you meant which type of custom implementations the user might provide:

1. Just throw an error about the type error
2. Convert the case I found about Docker labels (both are valid configuration values, but break the merge)

labels: [ 'traefik.enable= True' ]

labels: {
  traefik.enable: true
} 

[RebeccaStevens]

@fgarcia you can use customMerge to detect a type mismatch:

import deepmerge from 'deepmerge';

const a = { a: ['foo', 'bar'] };
const b = {
  a: {
    foo: true,
  },
};

try {
  const r = deepmerge(a, b, {
    customMerge: () => (source, target, options) => {
      if (Array.isArray(source) !== Array.isArray(target)) {
        throw new Error('Type Mismatch.');
      }

      // Merge as normal.
      return deepmerge(source, target, options);
    },
  });

  console.log('result', r);
} catch (error) {
  console.error(error);
}

Looking at the custom array merge it seems that the object is silently translated into []

No this isn't what's happening. Deepmerge is seeing the two types as being incomparable to merge and so is doing what it always does in this case which is to keep the one from "target" and drop the one in "source".
If you pass your objects to merge in reverse order, the other value will be kept instead.

A new function option isn't needed but I'm torn on whether a new flag to more simply enable this behavior should be added or not. @TehShrike What's your thoughts?

[fgarcia]

Thanks! I did not realize it was so easy to create a custom merge that could forward calls to the default

Having that I can always use my own wrapper with a safe merge (avoid the silent error).

From my point of view it would be safer and more new user friendly if this were the default behavior, but I understand it would be a nasty breaking change.

My issue is solved with the sample code, but I leave it open for the maintainer to evaluate if adding or not the flag is worth it. Probably it would only be valuable if shown in one of the main examples of the README. That way users could notice that by default the library will silently throw away mismatching fields

[TehShrike]

Honestly this feels like a really niche request, I wouldn't want to add another flag for it. I have been thinking a bit about what a purely generalized, open-for-modification type of merge function would look like, and if that ever sees the light of day it would probably be clearer to the consumer what the default behavior was in every case.