Useless `Partial` in type definition

[teppeis]

I don't know why Partial<> is used for the types of params.

declare function deepmerge<T1, T2>(x: Partial<T1>, y: Partial<T2>, options?: deepmerge.Options): T1 & T2;

The Partial<> causes a difference between static typing and actual values in runtime.

interface T {
  a: string;
  b?: string;
}
const t1: T = { a: "foo" };
const t2: T = { a: "bar" };

// The inferred type of `result` is `{a: string, b: string}`,
// but the actual value is `{a: "bar"}` without prop `b`.
const result = merge(t1, t2);

// tsc doesn't throw any compile errors, but a runtime error is thrown.
// TypeError: Cannot read property 'toLowerCase' of undefined
console.log(result.b.toLowerCase());

I think the Partial<> should be just removed.

declare function deepmerge<T1, T2>(x:T1, y: T2, options?: deepmerge.Options): T1 & T2;

[TehShrike]

I think I'm with you on the two-type case.

There are some other cases that I think we should add type tests for before dropping the one-type case, or the Partials in all:

interface Whatever {
    important: string,
    info: string,
}

const merged = deepmerge<Whatever>(
    { important: 'yes' },
    { info: 'okay' }
)

const alsoMerged = deepmerge.all<Whatever>([
    { important: 'yes' },
    { info: 'okay' }, 
    { info: 'really?' } 
])

I haven't been doing a lot of TS lately, so let me know if you think there's a more idiomatic way to type those operations

[teppeis]

@TehShrike In the first case, you don't have to specify the template type. → Playground code

declare function deepmerge<T1, T2>(x:T1, y: T2): T1 & T2;

interface Whatever {
    important: string,
    info: string,
}

// The inferred type is `{important: string} & {info: string}`,
// which is equivalent to `Whatever`.
const merged = deepmerge(
    { important: 'yes' },
    { info: 'okay' }
)

// Also you specify `Whatever` type explicitly. No errors.
const merged2: Whatever = deepmerge(
    { important: 'yes' },
    { info: 'okay' }
)

The second case is more complicated. TypeScript does not have a perfect solution for functions like all().

See official typings of Object.assign or Function.prototype.bind cases. They can check typings correctly only for 3 or 4 params. If more params are received, it compromises and assumes that all are the same type T.

I think deepmerge should do the same. → Playground

declare function deepmerge<T1, T2>(x:T1, y: T2): T1 & T2;
declare namespace deepmerge {
    export function all<T1, T2> (objects: [T1, T2]): T1 & T2;
    export function all<T1, T2, T3> (objects: [T1, T2, T3]): T1 & T2 & T3;
    export function all<T1, T2, T3, T4> (objects: [T1, T2, T3, T4]): T1 & T2 & T3 & T4;
    export function all<T1, T2, T3, T4, T5> (objects: [T1, T2, T3, T4, T5]): T1 & T2 & T3 & T4 & T5;
    export function all<T> (objects: T[]): T;
}

interface Whatever {
    important: string,
    info: string,
}

// The inferred type: `{important: string} & {info: string} & {info: string}`,
// which is equivalent to `Whatever`
const merged = deepmerge.all([
    { important: 'yes' },
    { info: 'okay' }, 
    { info: 'really?' } 
])

// It's same to the result of official `Object.assign` typing.
const merged2 = Object.assign(
    { important: 'yes' },
    { info: 'okay' },
    { info: 'really?' }
);

// # More than 5 params
// The inferred type: `{important: string, info?: string} | {important?: string, info: string}`,
// which is NOT equivalent to `Whatever`.
const merged3 = deepmerge.all([
    { important: 'yes' },
    { info: 'okay' },
    { info: 'okay1' },
    { info: 'okay2' },
    { info: 'okay3' },
    { info: 'really?' }
]);

// So just cast, honestly.
// Because tsc can not guarantee that the typing is correct statically.
const merged4 = deepmerge.all([
    { important: 'yes' },
    { info: 'okay' },
    { info: 'okay1' },
    { info: 'okay2' },
    { info: 'okay3' },
    { info: 'really?' }
]) as Whatever;

Casting is better than using Partial<> by default, because Partial<> hides potential type errors (original issue I reported). Developers should be aware of it and cast explicitly.

If you like this idea, I will update my PR.

[RebeccaStevens]

With your first example, explicitly declaring a generic type will fix the issue.

E.g.

const result = merge<T>(t1, t2);

I don't think the issue is with Partial<> itself. Partial<T> seems like the right type to me as neither parameter value needs to be of type T; they just need to result in type T when merged.

[chengluyu]

I think deepmerge is frequently used in configuration object merging. In this situation, developers believe that the resulting object contains nothing undefined. That's why the author uses Partial in the parameter type and no partial in the return type.

I agree with you that(a: U, b: T) -> U & T are more reasonable. But the current type definition is also essential in some situation.

[teppeis]

@RebeccaStevens It is not type safety because the generic type can receive any super classes (equivalent to casting).

const result = deepmerge<{a: string}>({}, {});
console.log(result.a.toUpperCase()); // throw a runtime error

neither parameter value needs to be of type T

Yes, so you don't have to specify T if it just returns T1 & T2 without Partial. It's exactly the same type as the implementation.

declare function deepmerge<T1, T2>(x:T1, y: T2, options?: deepmerge.Options): T1 & T2;

Imagine re-implementing this deepmerge with TypeScript. Do you use Partial<T> as the argument types? I don't think it can be implemented well.

[RebeccaStevens]

@teppeis Looking into your PR implementation I think I understand this better now and where you are coming from.

What's your thoughts on this alternative type def?

declare function deepmerge<T0 extends T1 & T2, T1, T2>(x: T1, y: T2, options?: deepmerge.Options): T0;

[teppeis]

@RebeccaStevens The same error occurs as in the first example.

[RebeccaStevens]

Isn't the tsc error desired?

comparison of the 3 defs

[s-edlund]

What I really want is being able to use DeepPartial from ts-essentials here, but passing a DeepPartial to deepmerge breaks the when types are checked for deep properties (e.g. Type 'undefined' is not assignable to type 'xyz'.). Using DeepPartial for deepmerge makes total sense to me. Any possibility? Or a workaround?

[RebeccaStevens]

@s-edlund Can you give a code example?

[s-edlund]

Sure:

import {DeepPartial} from 'ts-essentials';
import deepmerge from 'deepmerge';

interface Test {
    x: number;
    y: {
        z: number
    }
}

const deep : DeepPartial<Test> = {
    y: {
        z:1
    }
}

deepmerge<Test>({x:1}, deep);

Gives error:

src/test.ts:17:24 - error TS2345: Argument of type '{ x?: number | undefined; y?: { z?: number | undefined; } | undefined; }' is not assignable to parameter of type 'Partial<Test>'.
  Types of property 'y' are incompatible.
    Type '{ z?: number | undefined; } | undefined' is not assignable to type '{ z: number; } | undefined'.
      Type '{ z?: number | undefined; }' is not assignable to type '{ z: number; }'.
        Types of property 'z' are incompatible.
          Type 'number | undefined' is not assignable to type 'number'.
            Type 'undefined' is not assignable to type 'number'.

17 deepmerge<Test>({x:1}, deep);

[RebeccaStevens]

So with the current implementation of this library's types, deepmerge<Test>({x:1}, deep) is the same as deepmerge({x:1}, deep) as Test.

In the new proposed implementation, that error message is desired as based on the types of the inputs, the resulting type is DeepPartial<Test> & { x: number } - We can't guarantee that the resulting type is of type Test.