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

(Vanilla) Mixins Pattern: Base constructor must have same return type #40110

Closed
jorenbroekema opened this issue Aug 18, 2020 · 9 comments
Closed
Assignees
Labels
Rescheduled This issue was previously scheduled to an earlier milestone Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@jorenbroekema
Copy link

TypeScript Version: 4.1.0-dev.20200818

Search Terms: Base constructors same return type

Code

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

declare class FooHost {
  static foo: string;
}

declare function FooMixin<T extends Constructor<HTMLElement>>(superclass: T): T & Constructor<FooHost> & typeof FooHost


class Foo extends FooMixin(HTMLElement) { }

The reason I do & typeof FooHost at the end of FooMixin is because I need to be able to access the static property foo of the FooMixin's host class. This part works. But it creates a new error:

Base constructors must all have the same return type

What is important here to note is that in my project I use allowJs + checkJs so I have to find a way that doesn't include me being forced to do something like:

class Foo extends FooMixin(HTMLElement)<Args> { }

Because I can't, my implementation is in JavaScript (where the FooMixin is applied).

Expected behavior:

No error

Actual behavior:

Base constructors must all have the same return type

Playground Link: https://www.typescriptlang.org/play?ts=4.1.0-dev.20200818#code/CYUwxgNghgTiAEAXAngBwQYQPYDsDOiMArmIljADwAqAfPALzw4gDu8AFAHTewDmeALnhQcyANoBdAJQM6VANwAoRaEiwEavHngAxLFgASWAvADei+PAJREASzBWwWdMACiEEAFsQORIKuEtji8SgC+yqrQcPAAZkQ4pLa4uvoAsrYAHkHU8CAZiD7A2tj4hCRklAZUqQAy7l4+iDQ07HhE6DCa-lRSQlTwAGTwJQTEpOQUeobGTYNIaCBYMSnTBMqKXSu5+YXaU+lZOOxVtfXevjKm8KFAA

@TechQuery
Copy link

I got the same error in the Beta version of WebCell 2.3.

If I set the value type of Sub-class Constructor same as its super, the error disappears, and the Sub-class properties disappear in Code Hinting, too...

https://github.com/EasyWebApp/WebCell/blob/92123fbcde7ea839ed67e69b9c906fda44349381/source/WebField.ts#L48

@jorenbroekema
Copy link
Author

This is still a major problem for us, as our component library is meant to be extended by subclassers, and we use mixins basically everywhere. This means we cannot possibly strongly-type our library and our users can't use our library with Typescript, because the d.ts files will error for them, since @ts-ignore / @ts-expect-error both won't show up in the generated d.ts file.

I see that this was added for 4.1.0 milestone but we're already at 4.2.0. Will this issue be picked up still?

@RyanCavanaugh RyanCavanaugh added the Rescheduled This issue was previously scheduled to an earlier milestone label Dec 11, 2020
@rbuckton
Copy link
Member

This isn't a bug. The compiler is saying that the FooHost constructor (which is basically new (): FooHost) is incompatible. Since you don't need the instance side of FooHost here, you can address this by replacing & typeof FooHost with & Pick<typeof FooHost, keyof typeof FooHost>:

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

declare class FooHost {
  static foo: string;
}

declare function FooMixin<T extends Constructor<HTMLElement>>(superclass: T): T
  & Constructor<FooHost>
  & Pick<typeof FooHost, keyof typeof FooHost>


class Foo extends FooMixin(HTMLElement) { }

Playground link

@rbuckton rbuckton added Working as Intended The behavior described is the intended behavior; this is not a bug and removed Bug A bug in TypeScript labels Dec 17, 2020
@rbuckton
Copy link
Member

You can also fix this by giving FooHost a constructor that matches the mixin pattern requirements (i.e., constructor(...args: any[]);):

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

declare class FooHost {
  static foo: string;
  constructor(...args: any[]);
}

declare function FooMixin<T extends Constructor<HTMLElement>>(superclass: T): T 
  & Constructor<FooHost>
  & typeof FooHost


class Foo extends FooMixin(HTMLElement) { }

Playground link

@jorenbroekema
Copy link
Author

That makes sense, thanks a lot for explaining and suggesting a fix!

@gund
Copy link

gund commented Dec 30, 2022

I'm sorry but can we reopen this as there was really no explanation as to why is this considered "not a bug" and why constructors were "considered" incompatible if they are actually compatible (there are no conflicting props/methods etc.)?

I'm trying to build a form associated mixin for web components with full type safety but this bug is basically destroying typing system.
Here is a minimal demo of the issue.

Basically TS complains at the time I'm using (extending) my mixin with error "Base constructors must all have the same return type" even though technically they are compatible.

Also worth noting that I've specifically avoided "mixin-style constructor signature" as in this particular case it's not allowed to pass any constructor arguments as this are web components which have strictly no arguments by design, so having ...args: any[] would be lying and shutting the type safety down, which makes this issues at least related if not relevant.

@jorenbroekema
Copy link
Author

I tend to agree that ...args: any[] is a bad solution, because it implies you can pass arguments to the class which extends HTMLElement, which is false (even though it doesn't throw an error). It feels like a hacky solution at best.

As for Pick<typeof FooHost, keyof typeof FooHost> instead of typeof FooHost, I don't see as much of an issue with that, even though it also feels a little bit strange that the 2nd one would throw an error. I agree with @gund that it's not clear in this issue why the constructors were considered incompatible in the first place..

#36821 this issue is somewhat related I think because it also is a case of where TypeScript is pretty constraining/conservative towards Mixin patterns, and the suggestion here is that when this is the case, and the code is not objectively wrong, @ts-ignore is your escape hatch.

The biggest issue with this escape-hatch is that it means that your consumers must put "skipLibCheck": true in their tsconfig if your library is a JavaScript library where the types are inferred from generated .d.ts files from JSDocs comments. This is because if you put @ts-ignore and generate .d.ts files, there will not be a @ts-ignore anymore in the output types and the consumer's TS will error on the generated type defintions of your library. Most consumers don't know about this and will just complain in your library's github issues "wtf plz fix ur types". A TypeScript configuration option/opt-out flag is understandably not a good solution, as they said, they regret having added every single one of them, but @ts-ignore is not great either.

@clshortfuse
Copy link

clshortfuse commented Mar 14, 2023

I came across this because I used a function to modify the prototype of a class:

 set<T1 extends typeof ICustomElement, T2 extends object>
    (this: T1, source: T2 & ThisType<T2 & InstanceType<T1>>, options?: Partial<PropertyDescriptor>)
    : T1 & (new (...args: ConstructorParameters<T1>) => InstanceType<T1> & T2)

Basically to do something like MenuItem = ListItem.extend().set({type: 'menu'}) . The actual use case is for observables, but the set() example is clearer.

I found out the problem is that the above type will not override the constructor, but overload it. That means calling class ExtendedMenuItem extends MenuItem() {} would throw the error in question. That's because because TS sees MenuItem as:

new (): (...args: ConstructorParameters<typeof ListItem>) => ListItem;
new (): (...args: ConstructorParameters<typeof ListItem>) => ListItem & { type: 'menu'} ;

It makes sense because T1 & does mean to extend it, not override. The solution I worked up was to strip the new from the base class by repeating all the properties with {[P in keyof T1]: T1[P]} & :

 set<T1 extends typeof ICustomElement, T2 extends object>
    (this: T1, source: T2 & ThisType<T2 & InstanceType<T1>>, options?: Partial<PropertyDescriptor>)
    : {[P in keyof T1]: T1[P]} & (new (...args: ConstructorParameters<T1>) => InstanceType<T1> & T2)

It's technically not a bug, just one of the quirks of trying to override a Constructor in TS.

A utility type (eg: type ExtendPrototype<T1, T2> = {[P in keyof T1]: T1[P]} & (new (...args: ConstructorParameters<T1>) => InstanceType<T1> & T2)) didn't work too well since in my tests, I hit "Type instantiation is excessively deep and possibly infinite." too often. Using Pick<T1, keyof T1> would result in the same.

{[P in keyof T1]: T1[P]} & works pretty well because I think it flattens it down to static types. It would be nice for way to override a constructor more easily --- maybe something TS can do internally and provide a utility type instead of us having to resolve to custom user types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Rescheduled This issue was previously scheduled to an earlier milestone Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

6 participants