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

Class mixins shouldn't require a specific signature of constructors #14126

Open
justinfagnani opened this issue Feb 16, 2017 · 5 comments
Open
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript

Comments

@justinfagnani
Copy link

TypeScript Version: 2.2.0-dev.20170209

Code

Requiring mixins to have a constructor signature of constructor(...args: any[]) causes a few problems:

  1. Node v4, which is an active LTS, doesn't support rest and spread syntax. We avoid it and can run our output on Node v4 even when targeting ES6. The mixin constructor requirements prevent this.

  2. It's common to have mixins that want to provide a specific constructor. These will usually be applied last to create a concrete class. This is impossible now, so you have to create a new concrete class with only a constructor to give it the correct signature.

  3. For a mixin that does have constructor argument requirements, it's very cumbersome to extract arguments out of ...args.

Here's an approximation of a case I hit. I have two classes that that share a base class:

class Base {
  foo: string[];
}

class A extends Base {}

class B extends Base {}

Later, I want to add some functionality to via mixins to create two new classes:

type Constructor<T extends object> = new (...args: any[]) => T;
interface Magic {}
const Magic = <T extends Constructor<Base>>(superclass: T): Constructor<Magic>& T =>
  class extends superclass implements Magic {
    constructor(foo: string[]) {
      super();
      this.foo = foo;
    }
  };

const MagicA = Magic(A);
const MagicB = Magic(A);

And I want MagicA and MagicA to have a constructor that takes a single argument. I know the code will work because I know the constructors of A and B, but the type checker is unhappy.

First, the Constructor type is wrong in this case because I know the signature I want. I'd actually like to write:

type BaseConstructor = new() => Base;
type MagicConstructor = new(foo: string[]) => Magic;
interface Magic {}

const Magic = <T extends BaseConstructor>(superclass: T): MagicConstructor & T =>
  class extends superclass implements Magic {
    constructor(foo: string[]) {
      super();
      this.foo = foo;
    }
  };
const MagicA = Magic(A);
const MagicB = Magic(A);

Expected behavior:

This works

Actual behavior:

Error: "Type 'T' is not a constructor function type." on the extends clause.

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label May 24, 2017
@antoniusostermann
Copy link

antoniusostermann commented Jan 26, 2018

+1 - imho there are many use cases for this. If you need a flexible, but feature-rich mixin, you often need to specify the (constructor)-dependencies of classes using your mixin. You want to restrict your mixin to only a group of classes, for example to controller classes, and would like to access a property like the current user, without forcing the super state to contain this property. Especially if the mixin is imported from a library, you really don't want to force the library's user to change his parental class structure in order to use the mixin.

@ChristofferGersen
Copy link

+1 - My use case is also about constructor dependency injection. My mixin restricts the possible base classes, and that base class has constructor dependencies. I would like to force subclasses of the mixin to provide the correct dependencies to the constructor. Just like is possible with regular (sub)classes. And if the constructor signature of the restricted base class changes, then there should be compilation errors.

I currently use the following workaround:

class MyDependency {
}

class MyBaseType {
    // Changing the constructor signature here will not cause an error
    // inside the MyMixin constructor
    constructor(readonly dep: MyDependency) {
    }
}

type Constructor<T extends object> = new (...args: any[]) => T;

type MyBaseTypeConstructor = new (dep: MyDependency) => MyBaseType;

function MyMixin<T extends MyBaseTypeConstructor>(base: T) {
    // Ideally this cast would not be required
    // For now it solves the "Type 'T' is not a constructor function type"
    const baseConstructor: Constructor<MyBaseType> = base;

    return class extends baseConstructor {
        constructor(dep: MyDependency) {
            super(dep); // This call is not type-checked, because of ...any[] rest argument
        }
    };
}

class ExampleUsage extends MyMixin(MyBaseType) {
        constructor(dep: MyDependency) {
            super(dep); // This should be type-checked to be the same signature as MyBaseType
        }
}

With this workaround, changing only the MyBaseType constructor signature will give you compilation errors (which is what I want). However, you should not forget to change the MyMixin constructor after changing MyBaseTypeConstructor to the correct signature, since you will not get any more errors after changing just MyBaseTypeConstructor. That would be a problem, because subclasses of MyMixin should cause compilation errors at this point. Luckily, after updating the constructor inside MyMixin you will get the remaining errors which you would need to fix.

@trusktr
Copy link
Contributor

trusktr commented Jun 20, 2019

Another pattern (in JS) is to pass an options object containing properties that any constructor can read, regardless of order. Any constructor in the super() call stack can just pluck whatever it needs.

function Foo(Base = class {}) {
  return class Foo extends Base {
    constructor(options) {
      super(options)
      console.log(options.foo)
    }
  }
}

function Bar(Base = class {}) {
  return class Bar extends Base {
    constructor(options) {
      super(options)
      console.log(options.bar)
    }
  }
}

class Baz extends Foo(Bar()) {
  constructor(options) {
    super(options)
    console.log(options.whatever)
  }
}

new Baz({foo: 'foo', bar: 'bar', whatever: 'whatever'})

This is nice, most of the time it just works, as long as all classes and mixins in the hierarchy use the single options parameter convention, and don't clash on the property names (which is the same issue as clashing on instance property/method names anyways, so we should strive for unique naming).

@dragomirtitian
Copy link
Contributor

dragomirtitian commented Jun 20, 2019

@trusktr With type assertions anything is possible 😊. This works well for the users of the mixin (although authoring the mixin is a bit of dark magic):

type ComposeConstrucrtor<T, U> = 
    [T, U] extends [new (a: infer O1) => infer R1,new (a: infer O2) => infer R2] ? {
        new (o: O1 & O2): R1 & R2
    } & Pick<T, keyof T> & Pick<U, keyof U> : never
    
function Foo<T extends new (o: any) => any>(Base: T) {
    class Foo extends (Base as new (...a: any[]) => {}) {
        constructor(options: { foo: string }) {
            super(options)
            console.log(options.foo)
        }
        foo() {}
        static foo() {}
    }
    return Foo as ComposeConstrucrtor<typeof Foo, T>
}

function Bar<T extends new (o: any) => any>(Base: T) {
    class Bar extends (Base as new (...a: any[]) => {}) {
        constructor(options: { bar: string }) {
            super(options)
            console.log(options.bar)
        }
        bar() {}
        static bar() {}
    }

    return Bar as ComposeConstrucrtor<typeof Bar, T>
}

class Baz extends Foo(Bar(class { }))  {
    constructor(options: { foo: string, bar: string, whatever: string }) {
        super(options)
        console.log(options.whatever)
    }
    baz() {}
    static baz() {}
}
let baz = new Baz({ bar: "", foo: "", whatever: ""});
baz.bar();
baz.baz();
baz.foo()

Baz.foo();
Baz.bar();
Baz.baz();

@trusktr
Copy link
Contributor

trusktr commented Jun 20, 2019

@justinfagnani On a related note, have you had any problems using mixins inside of other mixins? It doesn't seem to work, saying ___ is not a constructor function when applying a mixin in the extends part of another mixin: #32004

@RyanCavanaugh RyanCavanaugh added Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript and removed Needs Investigation This issue needs a team member to investigate its status. labels Aug 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

6 participants