customMerge never being called

[xenoterracide]

export function guessJodaOptions(): deepmerge.Options {
  return {
    customMerge: (key: string): ((left: unknown, right: unknown) => unknown) | undefined => {
      if (/.*(date|time|On).*/.test(key)) return jodaMerge;
      return undefined;
    },
  };
}

usage

    const defaults: UserEntityOpt = {
      username: shortUniqueString(),
      email: `${uniqueString()}@briggo.com`,
      firstName: shortUniqueString(),
      lastName: shortUniqueString(),
      password: 'DontUseThisPassword',
      isStaff: false,
      isActive: true,
      isSuperUser: false,
      dateJoined: ZonedDateTime.now(),
    };
 return repo.save(UserEntity.of(deepmerge(defaults, part, guessJodaOptions())));

note: without joda this thing works perfectly

I breakpointed it, and guessJodaOptions() is called but never customMerge.

[superguineapig]

Hi, I'm not affiliated with this library, but I find it quite useful and stumbled across similar issues recently, so I created a codesandbox to test this issue.

It seems that there are 2 problems:

• ZonedDateTime has a very large inheritance chain. That causes the merge routine to exceed the JS recursion limit. This can be avoided by setting a custom isMergeableObject that will return false for class-based objects. but...
• when isMergableObject returns false, the customMerge function will never be run. This behavior seems slightly ambiguous as the non-mergeable-object is still copied (by reference) to the output.

I think it would make sense that a custom merger should always be invoked, if available, when the item is non-mergeable.

It also might make sense to have an additional configuration option(s) to determine whether or not an object is included/excluded from the output. This would make the semantics behind isMergeableObject more "pure".

There are several scenarios to consider, all of which would change the existing behavior and API surface area. For example:

• what is the order of the pipeline? E.g. "is mergeable" -> "include/exclude" -> "custom merger"
• include/exclude semantics: E.g. "include/exclude if not-mergeable," "always copy by reference if not mergeable and no custom merger available, else exclude" etc.

I'm not sure how much the author(s) would be comfortable with changing though.

I'm happy to review and discuss as time permits.

[wilcoxpdfrd]

Echoing @superguineapig comment, and expanding on it a bit; the default isMergeableObject function checks for non-null "objects", but boolean, string, etc. values will not then be "mergeable", and customMerge will not be called for properties with such values. But I want my custom merge function to compare let's say true and true for a particular property on target and source and return undefined in that case, so that the destination property value is set to undefined (essentially "voiding" it), for the purpose of producing a merged object that just reflects properties that have changed.

So it seems we should not be calling isMergeableObject on the value of source keys and only when that returns true then getting the merge function and using it to perform the merge. Rather we should call propertyIsOnObject(source, key) instead, just as we do on the target object at that time.

Thanks!