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

More accurate typing of Object.assign and React component setState() #6613

Closed
rogierschouten opened this issue Jan 25, 2016 · 37 comments
Closed
Assignees
Labels
Committed The team has roadmapped this issue Suggestion An idea for TypeScript

Comments

@rogierschouten
Copy link

This is a proposal for solving the second use case mentioned in Issue #6218

Problem

/**
 * This class is a given by the React framework
 */
class ReactComponent<S> {
    protected _state: S;

    /**
     * This method actually accepts any supertype of S
     */
    protected setState(newState: S): void {
        for (let name in newState) {
            if (newState.hasOwnProperty(name)) {
                this._state[name] = newState[name];
            }
        }
    }

    protected componentWillMount(): void {
        // abstract
    }
}

/**
 * Some state interface declaration. Note all members are optional to allow setState to
 * be called with supertypes of BaseState
 */
interface BaseState {
    a?: string;
}

/**
 * My own base class for certain React widgets
 */
class BaseWidget<S extends BaseState> extends ReactComponent<S> {

    constructor() { 
        super();
        this._state = {};
    }

    protected componentWillMount(): void {
        this.setState({ a: "boo" });
    }
} 
$ tsc v1.ts
v1.ts(39,9): error TS2322: Type '{}' is not assignable to type 'S'.
v1.ts(43,23): error TS2345: Argument of type '{ a: string; }' is not assignable to parameter of type 'S'.

The compiler cannot know that the setState() method will accept any super-type of S, i.e. an object with any subset of the members of S.

Proposal

Add a keyword to type setState() properly. To avoid confusion, I chose partof instead of e.g. 'supertype of' (see also initial confusion in #6218). For me, 'partof' conveys that I can give the method any object with a subset of the members of S.

/**
 * This class is a given by the React framework
 */
class ReactComponent<S extends {}> {
    protected _state: S;

    /**
     * This method accepts any supertype of S
     */
    protected setState(newState: partof S): void {
    }
}

Properties of partof

  • Only works for object types (hence the 'extends {}' above) because I wouldn't know how to pass part of e.g. a string
  • Allows any supertype of the given type
  • Incorporates knowledge of the generic parameter. Given the class declaration class ReactComponent<S extends { foo: number; }>, one is able to call setState({ foo: 3}) and setState({}) but not setState({ bar: 3 }) inside the class definition.

Comments more than welcome, I'm not a compiler expert. This is just to get the discussion going. If you think there already exists a way of typing this, please check with the original issue for a couple of failed examples.

@rogierschouten
Copy link
Author

@RyanCavanaugh what do you think?

@RyanCavanaugh
Copy link
Member

Let's say for a moment we had a type operator optionalize that took an object type and produced a new object type whose properties were all optional. What would the trade-offs of that be vs partof for solving the React setState problem?

@rogierschouten
Copy link
Author

I think that your description is more concise and clear, and I think optionalize is a very intuitively clear keyword. At first glance, I see no difference in usability between our proposals, do you?

@ahejlsberg
Copy link
Member

A more general purpose alternative would be to allow super constrains for type parameters:

class ReactComponent<S extends {}> {
    protected _state: S;

    /**
     * This method accepts any supertype of S
     */
    protected setState<T super S>(newState: T): void {
    }
}

The requirement implied by T super S is that T must be a type to which S is assignable.

@rogierschouten
Copy link
Author

@ahejlsberg that would work too, but I would avoid the word 'super' because one would not know whether you mean supertype or superclass or super-something-else. Maybe use 'S assignable T'?

However I've found a number of use cases for a type operator 'optionalize' like @RyanCavanaugh suggested - that way I can declare new types and declare variables of optionalized types.

@RyanCavanaugh
Copy link
Member

The problem with super here is that it's inaccurate in several ways:

declare function setState<T super { s1: { x: Derived; y: number; } }>(s: T);

setState( { x1: 32 }); // No error, but wrong
setState( { s1: { y: 42 } }); // No error, but wrong
setState( { s1: { x: new Base(), y: 32 } }); // No error, but wrong

@kitsonk
Copy link
Contributor

kitsonk commented Jan 28, 2016

I also want to say that this use case is important to other libraries as well (and I think the title is a bit misleading, really what the intent of this is the way to operate on a type to make all its members optional).

I think it is a common pattern to pass an object literal to a constructor (or factory) function that can be any of the public properties of the instance class, which is then mixed in upon construction. I ran into this issue when trying to type Dojo 1 Dijits. The only option, if I wanted to type the constructor functions, is to create a wholly separate interface which has all optional members.

I agree super might give the wrong semantic impression. I suspect optionalize or optional would be more semantically clear:

class A {
  foo: string;
  bar(): string { return 'bar'; };
  constructor<O optionalize this>(options: O) { };
}

type OptionalA = optionalize A;

/*or*/

class B {
  foo: string;
  bar(): string { return 'bar'; };
  constructor(options: $Optionalize<this>) { };
}

type OptionalB = $Optionalize<B>;

@ahejlsberg
Copy link
Member

Whatever we do here also applies to Object.assign that was introduced in ES2015.

@ahejlsberg ahejlsberg changed the title Proposal for supporting React component setState() typing More accurate typing of Object.assign and React component setState() Jan 28, 2016
@mhegazy mhegazy added Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels Jan 28, 2016
@Strate
Copy link

Strate commented Jan 28, 2016

Somewhere here in issues I see this proposal for similar case:

function assign<R extends A,B,C>(a:A, b:B, c:C):R

Isn't it the similar thing?

@rogierschouten
Copy link
Author

@Strate I don't think so, see the first use case of post #6218. The first use case was solved like you said but the setState() use case wasn't.

@mhegazy
Copy link
Contributor

mhegazy commented Feb 24, 2016

We'd need type operators to solve this, which is a huge work item; Workaround of specifying all-optionals in state definition, which isn't too bad.

@mhegazy mhegazy closed this as completed Feb 24, 2016
@mhegazy mhegazy added the Declined The issue was declined as something which matches the TypeScript vision label Feb 24, 2016
@jwbay
Copy link
Contributor

jwbay commented Feb 24, 2016

Are type operators even on the table, or maybe tracked in a separate issue?

@mhegazy
Copy link
Contributor

mhegazy commented Feb 24, 2016

type operators keep coming up in different context. They are not totally dismissed, just not in the short term. the concern is the effort needed to add one of these, as it goes deep in the type system.

@rogierschouten
Copy link
Author

@mhegazy you're missing the point - all-optionals is NOT a workaround, you haven't understood the problem at all. Even if you specify all optionals, the compiler will not accept the code and you will have to cast to any. This affects everybody using React and subclassing. Even if this is not on the near term roadmap please keep this issue open.

@rogierschouten
Copy link
Author

@RyanCavanaugh please help

@RyanCavanaugh
Copy link
Member

@rogierschouten with what specifically?

@Igorbek
Copy link
Contributor

Igorbek commented Feb 27, 2016

Another possible solution could be allowing having member-level constraints for types defined in containing interface/class #1290

class ReactComponent<S extends {}> {
    protected _state: S;

    /**
     * This method accepts any supertype of S
     */
    protected setState<T, S extends T>(newState: T): void {
    }
}

Currently it introduces new S generic type parameter which have no connection with S from the class. But if it'd possible, T would be inferred from newState and then S wouldn't meet the constraint.

@kitsonk
Copy link
Contributor

kitsonk commented Feb 28, 2016

Yes, I thought of some sort of variation of F-bounded polymorphism (#5949) might help solve this problem, I think the problem here is how to set a type that cannot be resolved, even recursively, until the method's type is resolved. S's type is essentially a moving target.

@joewood
Copy link

joewood commented Mar 14, 2016

Agree with @Igorbek. I thought F-bounded polymorphism would just work in this case without the need for new keywords and type operators.

@AlexGalays
Copy link

@joewood Your function code doesn't work for all cases either. If you had a legitimate optional property in your State, then you wouldn't be able to update it ( { a?: number} is not a valid subtype of { a: number } ).

@joewood
Copy link

joewood commented Aug 27, 2016

@AlexGalays true, but wouldn't it more correct to say that the state was number | undefined than making it optional?

@AlexGalays
Copy link

@joewood Nice! I was still using void as the type containing undefined, looks like the new undefined type works better.

@AlexGalays
Copy link

@joewood

Your example doesn't work with other combinations, like unions

False positive

interface IState {
    readonly foo:number;
    readonly bar: 'aa' | 'bb';
};

const s : IState = { foo:1, bar: "aa"};

// Compiler is happy, but ideally shouldn't
merge(s, { bar: 'thisShouldntBeAssignable'})

Or:

"False" negative

type A = { _isA: boolean }
type B = { _isB: boolean }

interface IState {
    readonly foo:number;
    readonly bar: A | B;
};

const s : IState = { foo:1, bar: { _isA: true }};

// Doesn't compile, as an union cannot be assigned to just one of its types
merge(s, { bar: { _isB: true }})

This is sad :( I really wanted to have usable deep JSON updates.
I think it's important not to give the TS team the wrong impression: There are no known workarounds for this.

@joewood
Copy link

joewood commented Aug 30, 2016

I think there's a couple of things going on with your examples, and agree - this really isn't ideal.

For your first example, I think this relates to the rule where mutable bindings are widened for string literal types. See these issues for the details on that one #9489 and #5185. I would argue that no widening should apply to a constant expression - it's hard to argue that the compiler is doing the right thing here.

The second example is down to the fact that type { bar : B} is not a valid super of { bar : A | B}. So the compiler is correct here and the only suggestion I have is to cast the implicit type: merge(s, { bar: { _isB: true } as A | B}).

@AlexGalays
Copy link

I completely agree with your points. Also there might be quite a few other corner cases I didn't find yet.
It just shows we badly need some kind of type modifier that takes a type and return the same type with all its properties optional, recursively; or something along those lines (A la flow's $Shape)

@kitsonk
Copy link
Contributor

kitsonk commented Sep 20, 2016

While this is declined, #4889, which is very similar and will solve this problem is being actively discussed and @RyanCavanaugh has indicated it is possible candidate for TypeScript 2.1.

@AlexGalays
Copy link

Thanks for the link, I would love for this to be in 2.1

@RyanCavanaugh RyanCavanaugh removed the Declined The issue was declined as something which matches the TypeScript vision label Sep 26, 2016
@RyanCavanaugh RyanCavanaugh reopened this Sep 26, 2016
@sfrooster
Copy link

I can't follow the entire discussion here, but this seems to be related to the reason why I can't find a construct that allows me to say:

class Message {
    public static TypeID: string;
    public TypeID: string;

    public Sequence: number;

    constructor() {
    }
}

and then:

public receive<T extends Message>(rcvType: T, handler: (msg: T) => void): void {...}

Such that the "type" I'm specifying for T will work. For example:

class SomeMessage extends Message {
    public static TypeID = "Messages.Commands.SomeMessage";
    public TypeID = "Messages.Commands.SomeMessage";

    constructor(public Payload: string){
        super();
    }
}

This is currently generating the error:

receive(SomeMessage, (msg: SomeMessage) => {
    console.log(msg.Payload);
});

The type argument for type parameter 'T' cannot be inferred from the usage. Consider specifying the type arguments explicitly.
  Type argument candidate 'typeof SomeMessage' is not a valid type argument because it is not a supertype of candidate 'SomeMessage'.
    Property 'prototype' is missing in type 'SomeMessage'.

I can shift various and sundry things around, but I cannot find a combination that:
a) doesn't complain
b) explicitly ties the two types such that they must be the same type (I can specify things in a way that two different values for rcvType and handler/msg will work, where the intent is that they are the same)

FWIW, rcvType is important because it's used internally to set things up. I'd also love if I could just not specify rcvType (but I realize type erasure precludes that).

@mDibyo
Copy link

mDibyo commented Nov 17, 2016

Looks like this has been resolved with #12114.

@sfrooster
Copy link

I'm still trying to digest this PR...

On Wed, Nov 16, 2016 at 6:17 PM, Dibyo Majumdar notifications@github.com
wrote:

Looks like this has been resolved with #12114
#12114.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#6613 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AC-vl_bEXaisDZ_JaeaRwVSdpr1lC7CPks5q-7kcgaJpZM4HL6Cd
.

@dgoldstein0
Copy link

#12793 should have solved the react problem. Is there something still open here about Object.assign, or is Pick<...> good enough for it too?

@AlexGalays
Copy link

Object.assign don't want Pick<> as you can merge any kind of objects into any kind of object.
If you want to update an object with only objects of the same shape, then you will want to code something that looks a lot like the solution in #12793 indeed.

I think the only issue with the current Object.assign type definition is when the right hand side object has a key with a different type. Instead of it completely replacing the initial type (like in the implementation) the result becomes the union of both types, which sometimes don't even make sense (e.g number & string)

@Igorbek
Copy link
Contributor

Igorbek commented Dec 28, 2016

#10727 would fix it because type of Obejct.assign(a, b) is exactly type of {...a, ...b}.

@mhegazy
Copy link
Contributor

mhegazy commented Jun 14, 2017

relevant discussion about distinguishing missing and undefined is tracked in #16524

@RyanCavanaugh RyanCavanaugh added Committed The team has roadmapped this issue and removed In Discussion Not yet reached consensus labels Aug 15, 2017
@RyanCavanaugh
Copy link
Member

Fixed with mapped types. Please log new issues if you find any shortcomings in this area

@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
Committed The team has roadmapped this issue Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests