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

Implementing DeepMerge #12769

Closed
AlexGalays opened this issue Dec 8, 2016 · 18 comments
Closed

Implementing DeepMerge #12769

AlexGalays opened this issue Dec 8, 2016 · 18 comments
Assignees
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue

Comments

@AlexGalays
Copy link

Hello and good job on the mapped types!

Though it seems recursive mapped types didn't get as much love.

Is there any way to write the signature of update so that the following can be true?

I run into either too permissive behaviors (I can add keys that are not in the original object, or set a non nullable key to undefined) or inferFromTypes: Maximum call stack size exceeded.

/*
 * SHOULD COMPILE
 */

update({ a: 33 }, { a: 44 })

type A = { a?: number }
update({ a: 33 } as A, { a: undefined })

update(
  { a: { b: [1] }, c: 33, d: '44' },
  { a: { b: [1, 2] }, d: '66' }
)



 /*
  * SHOULD NOT COMPILE
  */

update({ a: 33 }, { a: undefined })

update({ a: 33 }, { b: 44 })

update({ a: 33 }, { a: '33' })

update({ a: { b: [1] } }, { a: { b: ['2'] } })
@mhegazy
Copy link
Contributor

mhegazy commented Dec 8, 2016

these two examples seem conflicting to me..

update(
  { a: { b: [1] }, c: 33, d: '44' },
  { a: { b: [1, 2] }, d: '66' }
)

and

update({ a: 33 }, { a: undefined })

since the first is more or less the same as saying c: undefined.

on any rate, i would say using two type parameters and a constraint to check their relationship would be a good way to go:

declare function update<T, U extends T>(a: T, U): void;

assuming that the first argument is a "spec" and the second can omit members, then it can be as:

declare function update<T, U extends T>(a: T, b:Partial<U>): void;

@mhegazy mhegazy added the Question An issue which isn't directly actionable in code label Dec 8, 2016
@AlexGalays
Copy link
Author

AlexGalays commented Dec 8, 2016

Not sure how that helps :(
Btw, did you mean T extends U ? I doesn't makes sense for U to have more properties than T in my usage.

That's what I have so far btw :

type UpdateSpec<T> = {
  [P in keyof T]?: UpdateSpec<T[P]>
}

export default function update<T>(obj: T, spec: UpdateSpec<T>): T

@mhegazy
Copy link
Contributor

mhegazy commented Dec 8, 2016

i am confused now.. so if all what you want is for the second be a partial of the first, why now just:

declare function update<T>(a: T, b: Partial<T>): void;

@AlexGalays
Copy link
Author

Because it has to be deep. At every level, it is allowed to either pass a primitive (it will be set) or an object with a partial set of keys, and so on.

i.e, this should compile :

const updated = update(
  { a: { b: { c: 33, d: 33, e: 'aa' } } },
  { a: { b: { c: 99 } }
)

// updated is now equals to { a: { b: { c: 99, d: 33, e: 'aa' } } }

// The spec didn't change d and e, so their values are still the same.

@AlexGalays
Copy link
Author

AlexGalays commented Dec 8, 2016

Made some progress with

type UpdateSpec<T> = {
  [P in keyof T]?: T[P] & UpdateSpec<T[P]>
}

export default function update<T>(obj: T, spec: UpdateSpec<T>): T

But I still get stack overflows for type inference on some usages and I wish the ? operator didn't allow explicitly setting undefined values.

This will be troublesome for React too, as setState() will allow one to set undefined on a non nullable property.

@mhegazy
Copy link
Contributor

mhegazy commented Dec 8, 2016

I do not think the intersection is correct.. this should work..

type UpdateSpec<T> = {
  [P in keyof T]?: UpdateSpec<T[P]>
}

that it does not is seems like a bug, and we should investigate this further.

@mhegazy mhegazy added Bug A bug in TypeScript and removed Question An issue which isn't directly actionable in code labels Dec 8, 2016
@AlexGalays
Copy link
Author

AlexGalays commented Dec 9, 2016

Yeah it's strange. Without intersecting with T[P], I can update most properties with another property with a completely different type.

@mhegazy
Copy link
Contributor

mhegazy commented Dec 9, 2016

that is cause the recursion makes it go to any. I will need to discuss this with @ahejlsberg and see if there is anything we can do to make this work in a better way.

@mhegazy mhegazy added this to the TypeScript 2.2 milestone Dec 14, 2016
@ahejlsberg
Copy link
Member

With the current nightly we are actually very close:

type UpdateSpec<T> = { [P in keyof T]?: UpdateSpec<T[P]> };

declare function update<T>(obj: T, spec: UpdateSpec<T>): T;

declare let foo: { a: number, b: { x: string } };

// These all work as expected
update(foo, { a: 1 });
update(foo, { b: { x: "test" } });
update(foo, { a: 10, b: { x: "howdy" } });

// These are all errors as expected
update(foo, { a: "hello" });
update(foo, { b: { x: 42 } });
update(foo, { b: { y: 10 } });
update(foo, { b: { y: 10 } });
update(foo, { c: true });

// These should be errors but aren't
update(foo, "ouch");
update(foo, { b: "ouch" });

The thing that works for us here is that when a homomorphic mapped type { [P in keyof T]: X } is instantiated for a primitive type we simply yield the primitive type. The thing that doesn't work so well is actually quite unrelated to mapped types: When the target of an assignment is a type with properties that are all optional, the source can effectively be any type that doesn't have those properties. For example:

let x: { a?: number, b?: string } = "ouch";

We're perfectly happy with that because type String has no a or b property and therefore satisfies the contract. Since an UpdateSpec<T> has only optional properties we end up in the same situation.

@AlexGalays
Copy link
Author

AlexGalays commented Dec 14, 2016

I see! Would it help with the primitive case if you could say "give me an object with the same shape , with all optional keys BUT with a cardinality of at least one"? After all, if you're willing to update something, you might as well not pass a null object.

So it gets closer to full type safety, but I have the feeling you're not willing to go to full typesafety considering the "primitive to fully optional {} is structurally allowed" (which makes sense in most use cases but is a bit unfortunate in this one); or is it that you know about an upcoming fix specially for Mapped types that could circumvent that?

Anyway, thanks; that's already much better than what can be done currently!

@KnisterPeter
Copy link
Contributor

We have a similar case and stumbled above array properties.
E.g. this mapped type:

type Immutable<T> = { readonly [P in keyof T]: Immutable<T[P]>; };

does not work well with this typecase:

interface SomeInterface {
  property: string[];
}

as the infered result would be:

interface SomeInterface {
  readonly property: Immutable<string[]>;
}

instead of:

interface SomeInterface {
  readonly property: Immutable<string>[];
}

Is there a way to express this recursion with array properties?

@AlexGalays
Copy link
Author

AlexGalays commented Dec 16, 2016

I guess what I am asking for is a way to sometime opt out of classical structural typing.

Sometimes, some APIs makes no sense if they are invoked with an Array instead of an Object even though it's also correct to pass en empty object.

For instance if an object or an Array are semantically different (e.g a config/option object versus a children Array) and can either be in position 2 or 3 of an API. In that case, the implementation have to discriminate on whether we got an Object or an Array.
For that particular case, I had to use hacks in the past:

{
   reduceRight?: 'weReallyNeedThisToBeAnObjectLiteralAndNotAnArray'
   //...other stuff
}

But I don't think there are hacks like this one for every data types.
It annoys me that a string can be assigned in place of "an object that should have very precise optional properties" in some use cases.

Also, the issue of "non optional keys can be updated with null/undefined" remain I think.

@AlexGalays
Copy link
Author

Tried the nightly. Making progress indeed.

type UpdateSpec<T> = { [P in keyof T]?: T[P] & UpdateSpec<T[P]> };

declare function update<T>(obj: T, spec: UpdateSpec<T>): T;

declare let foo: { a: number, b: { x: string }, c?: string };

// These all work as expected
update(foo, { a: 1 });
update(foo, { b: { x: "test" } });
update(foo, { a: 10, b: { x: "howdy" } });
update(foo, { c: undefined });

// These are all errors as expected
update(foo, { a: "hello" });
update(foo, { a: null });
update(foo, { b: { x: 42 } });
update(foo, { b: { y: 10 } });
update(foo, { b: { y: 10 } });
update(foo, { b: "ouch" });
update(foo, { d: true });

// These should be errors but aren't
update(foo, "ouch");
update(foo, { a: undefined });

@ahejlsberg Do you think these last two errors can be fixed without too much trouble?

@ghost
Copy link

ghost commented Feb 16, 2017

@AlexGalays Does the object type help? Maybe:

declare function update<T>(obj: T, spec: object & UpdateSpec<T>): T;

at least prevents

update(foo, "ouch");

@sandersn
Copy link
Member

With 2.4's weak type checking, the last two examples are now errors, so I think the last part of this bug has been addressed.

@esamattis
Copy link

What about when there is a property missing from a nested object?

type UpdateSpec<T> = {[P in keyof T]?: T[P] & UpdateSpec<T[P]>};
declare function update<T>(obj: T, spec: UpdateSpec<T>): T;

interface IFoo {
    a: number;
    b: {x: string; y: number};
    c?: string;
}

var foo: IFoo = {
    a: 1,
    b: {x: "s", y: 1},
};

// Should not error
update(foo, { a: 99, b: { y: 2 } });

but yields

Argument of type '{ a: number; b: { y: number; }; }' is not assignable to parameter of type 'UpdateSpec<IFoo>'.
  Types of property 'b' are incompatible.
    Type '{ y: number; }' is not assignable to type '({ x: string; y: number; } & UpdateSpec<{ x: string; y: number; }>) | undefined'.
      Type '{ y: number; }' is not assignable to type '{ x: string; y: number; } & UpdateSpec<{ x: string; y: number; }>'.
        Type '{ y: number; }' is not assignable to type '{ x: string; y: number; }'.

@mhegazy
Copy link
Contributor

mhegazy commented Sep 18, 2017

should not it be Partial<T[P]> instead of T[P].

@mhegazy mhegazy added the Fixed A PR has been merged for this issue label Sep 18, 2017
@esamattis
Copy link

Ah, you're right. Works with Partial, thanks!

@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue
Projects
None yet
Development

No branches or pull requests

6 participants