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

Useless Partial in type definition #147

Open
teppeis opened this issue May 28, 2019 · 12 comments · May be fixed by #148 or #211
Open

Useless Partial in type definition #147

teppeis opened this issue May 28, 2019 · 12 comments · May be fixed by #148 or #211

Comments

@teppeis
Copy link
Contributor

teppeis commented May 28, 2019

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;
@teppeis teppeis linked a pull request May 28, 2019 that will close this issue
@TehShrike
Copy link
Owner

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
Copy link
Contributor Author

teppeis commented May 29, 2019

@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
Copy link

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
Copy link

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
Copy link
Contributor Author

teppeis commented Aug 11, 2019

@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
Copy link

@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
Copy link
Contributor Author

teppeis commented Aug 12, 2019

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

@RebeccaStevens
Copy link

Isn't the tsc error desired?

comparison of the 3 defs

@s-edlund
Copy link

s-edlund commented Apr 10, 2020

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
Copy link

@s-edlund Can you give a code example?

@s-edlund
Copy link

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
Copy link

RebeccaStevens commented Apr 13, 2020

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.

@RebeccaStevens RebeccaStevens linked a pull request Nov 5, 2020 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants