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

[2.8.0-rc] Mixins don't recognize static method from parent class #22788

Closed
canonic-epicure opened this issue Mar 22, 2018 · 15 comments
Closed
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@canonic-epicure
Copy link

When declaring a mixin, one can provide a "default" parent class to inherit from, using the type argument for the "Constructable" type. When this is done, TypeScript correctly finds the normal methods from the parent class, however it can not find the static methods:

export type Constructable<T = {}> = new (...args : any[]) => T

class Base {
    static someStaticMethod() : number {
        return 1
    }

    someInstanceMethod() : number {
        return 1
    }
}


const Mixin = <T extends Constructable<Base>>(base : T) => class Some1 extends base {
    static someStaticMethod () {
        return super.someStaticMethod() + 1   // !!! <----- compiler error here
    }

    someInstanceMethod () {
        return super.someInstanceMethod() + 1   // !!! <----- this compiles fine
    }
}
@canonic-epicure canonic-epicure changed the title Mixins don't recognize static method from parent class [2.8.0-rc] Mixins don't recognize static method from parent class Mar 22, 2018
@mhegazy mhegazy added the Working as Intended The behavior described is the intended behavior; this is not a bug label Mar 22, 2018
@mhegazy
Copy link
Contributor

mhegazy commented Mar 22, 2018

A class has two parts, a constructor function that also holds the static methods and properties, and an instance type that holds the instance methods and properties. the constructor function returns values of the instance type.

your function declaration takes a T that is constraint to Constructable<Base>, so that guarantees that calling the a value of type T with new will return a Base object. and hence, you can call someInstanceMethod on it.
There is nothing about the constraint that says that values of type T has a someStaticMethod. if you want to do that, make sure to add that on the constraint, e.g. someStaticMethod on the type Constructable.

@canonic-epicure
Copy link
Author

Hm.. Can you please provide an example how to do that?

@canonic-epicure
Copy link
Author

The export type Constructable<T = {}> = (new (...args : any[]) => T) & T attempt does not seem to work..

@canonic-epicure
Copy link
Author

canonic-epicure commented Mar 23, 2018

Additional information: It does seem to work, for 2nd level mixin. This is strange, shouldn't the behavior be consistent?

export type Constructable<T = {}> = new (...args : any[]) => T

class Base {
    constructor(...args : any[]) {
    }
    static someStaticMethod() : number {
        return 1
    }
    someInstanceMethod() : number {
        return 1
    }
}


const Mixin1 = <T extends Constructable<Base>>(base : T) => class Some1 extends base {
    static someStaticMethod () : number {
        return 1
        // return super.someStaticMethod() + 1  // !!! <----- compiler error here
    }
    someInstanceMethod () {
        return super.someInstanceMethod() + 1
    }
}

const Mixin2 = <T extends Constructable<Base>>(base : T) => class Some1 extends Mixin1<Constructable<Base>>(base) {
    static someStaticMethod () {
        return super.someStaticMethod() + 1     // !!! <----- this compiles fine
    }
    someInstanceMethod () {
        return super.someInstanceMethod() + 1
    }
}

@cspotcode
Copy link

cspotcode commented Mar 26, 2018

@samuraijack In Mixin1, the constraint on T does not require base to have any static methods. Therefore you get a compiler error trying to call static methods that are not guaranteed to exist.

In Mixin2, you are calling Mixin1 explicitly. This means Some1 extends from the return value of Mixin1, which is guaranteed to implement the static method. Your T constraint is not really involved.

@canonic-epicure
Copy link
Author

@cspotcode but why you say that

constraint on T does not require base to have any static methods.

as @mhegazy mentioned

T that is constraint to Constructable, so that guarantees that calling the a value of type T with new will return a Base object.

This means, that T is a constructor function of Base or its subclass, right? And therefor it should have static method, which Base itself has.

If typechecker is clever enough to figure out the presence of static method from the mixin, its should be clever enough to figure it out from the base class as well I think.

@cspotcode
Copy link

@samuraijack

T is a constructor function of Base

Correct, and Base is a type that describes instances of a class, which means the type T does not contain any information about static methods. So your generic constraint Constructor<T> doesn't make any guarantees about static methods.

There's a big difference between Base and typeof Base.

@canonic-epicure
Copy link
Author

@cspotcode Ok, I think I understand it vaguely. Any idea how to express this static methods inheritance in types?

@canonic-epicure
Copy link
Author

@cspotcode Oh, looks interesting, thanks! It seems to work, the mixin declaration becomes a bit cluttered though.

@canonic-epicure
Copy link
Author

@cspotcode Thanks for the inspiration, in fact, just changing the mixin's generic parameter to typeof Base seems to be enough:

export type Constructable<T = {}> = new (...args : any[]) => T

class Base {
    constructor(...args : any[]) {
    }
    static someStaticMethod() : number {
        return 1
    }
    someInstanceMethod() : number {
        return 1
    }
}


const Mixin1 = <T extends typeof Base>(base : T) => class Some1 extends base {
    static someStaticMethod () : number {
        return 1
        // return super.someStaticMethod() + 1  
    }
    someInstanceMethod () {
        return super.someInstanceMethod() + 1
    }
}

const Mixin2 = <T extends typeof Base>(base : T) => class Some1 extends Mixin1<typeof Base>(base) {
    static someStaticMethod () {
        return super.someStaticMethod() + 1    
    }
    someInstanceMethod () {
        return super.someInstanceMethod() + 1
    }
}

In your example the Foo class is lacking of generic constructor constructor(...args : any[]) that's why more magic is needed. It can also be reduced to:

const MyMixin = <C extends typeof Foo>(Base: C) => class MyMixin extends (Base as typeof Foo) {
    static mixinStatic() {
        super.staticMethod()
    }
}

I guess, this should be a new "default" mixins declaration for the case, when one assumes some specific base class. Deserves a blog post :)

@canonic-epicure
Copy link
Author

But, I've tried to apply the change Constructable <-> typeof Base on my existing codebase and it does not work well, probably because of #22824. Or at least it will require significant effort to cleanup all type errors. So I'll postpone the switch to the new notation until #22824 is resolved.

@cspotcode
Copy link

@samuraijack that's why my workaround uses two generic declarations and a nested function within the mixin function. I believe it should workaround that issue, right?

But yeah it's more syntax.

@canonic-epicure
Copy link
Author

Could not check, because it would have been too cumbersome to replace all mixins declarations (the Constructable <-> typeof Base change was merely a regexp search/replace) . Now waiting for #22824 for further experiments.

@typescript-bot
Copy link
Collaborator

Automatically closing this issue for housekeeping purposes. The issue labels indicate that it is unactionable at the moment or has already been addressed.

@microsoft microsoft locked and limited conversation to collaborators Jul 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

4 participants