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

Decorator Type Mismatch? #9453

Closed
fluffywaffles opened this issue Jun 30, 2016 · 10 comments
Closed

Decorator Type Mismatch? #9453

fluffywaffles opened this issue Jun 30, 2016 · 10 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@fluffywaffles
Copy link

fluffywaffles commented Jun 30, 2016

TypeScript Version: nightly (2.0.0-dev.20160630)

Code (This is a fairly contrived example.)

function CanRawr (target: (...args: any[]) => Function) : Function {
  class Rawrable extends target {
    static rawr () : void {
      console.log(`RAWR`)
    }
  }

  return Rawrable
}

@CanRawr
export default class Dinosaur { }

Expected behavior:

Everything typechecks.

Actual behavior:

Code compiles, but logs a hard to understand error. Compiled code works as expected.

app/ex.ts(11,2): error TS2345: Argument of type 'typeof Dinosaur' is not assignable to parameter of type 'new (...args: any[]) => Function'.
  Type 'Dinosaur' is not assignable to type 'Function'.
    Property 'apply' is missing in type 'Dinosaur'.

This is strange - looking at the signature for ClassDecorator:

declare type ClassDecorator = <TFunction extends Function>(target: TFunction) => TFunction | void;

I would think that I am allowed to return a Function in place of a TFunction.

Perhaps I am doing something wrong? But the code compiles and runs, and does what I expect.

import Dinosaur from './example'

Dinosaur.rawr() // logs: 'RAWR'
@RyanCavanaugh
Copy link
Member

@rbuckton is this the expected behavior?

@rbuckton
Copy link
Member

rbuckton commented Oct 24, 2016

I don't think the signature for CanRawr is correct. The expression in the extends clause must have a construct signature whose return type is a class or interface type. The target in the example is not a constructor function type. Also, I doubt Function is the expected type of the instance returned by the constructor.

The following does work:

function CanRawr(target: new (...args: any[]) => Dinosaur) {
    class Rawrable extends target {
      static rawr () : void {
        console.log(`RAWR`)
      }
    }
    return Rawrable;
}

@CanRawr
export default class Dinosaur { }

However, you probably want something like this:

function CanRawr<TClass extends new (...args: any[]) => Object>(target: TClass) {
    class Rawrable extends (<new (...args: any[]) => Object>target) {
      static rawr() : void {
        console.log(`RAWR`)
      }
    }
    return <TClass>Rawrable;
}

Unfortunately, the cast of target is necessary, as TClass is not a constructor function type. Also, even though class decorators can return a new constructor, any new members defined are not added to the type in the type checker. As a result, you would still get the error Property 'rawr' does not exist on type 'Dinosaur'.

@fluffywaffles
Copy link
Author

fluffywaffles commented Oct 25, 2016

@rbuckton So, in other words, the current decorator spec expects you to know ahead of time what base type you are trying to extend. You cannot use decorators to apply e.g. mixins similar to the discussion that I just saw in #2919 (comment).

The use-case we were already using decorators for is applying Object.defineProperty(...) for things like:

// MonkeyPatch just calls Object.defineProperty with the correct arguments
@MonkeyPatch(Symbol.hasInstance, {
  value: (lhs) => ((typeof lhs) === 'string')
})
class MyString { /* */ }

// in some other code, where the environment supports Symbol.hasInstance...
"hello" instanceof MyString // => true

This works fine.

Then the use-case we want to support looks more like:

@Model
class User {
  @Validator({ // suppose this metadata is used by the @Model decorator to create the `validate()` method
     test (lhs) { return lhs < 10 }
  })
  uid: number
}

// Now, in some other code...

// Because TypeScript uses Structural Typing...
const exampleUser : User = {
  uid: 5
} // type-checks.

// And, while it doesn't type-check, at runtime you can have...
User.validate(exampleUser) // => true
User.validate({ uid: 10 }) // => false

This can actually be done right now using type metadata provided from the compiler with the right emitDecoratorMetadata setting, but then the resulting code (which behaves correctly!) fails to type-check. So, while the implementation of decorators allows me to write models like this, and at run-time the decorator will provide User with e.g. a validate(...) method, the compiler will complain that the Property 'validate' does not exist....

I guess the question is, then: if the codegen can, and already does, support creating mixins like this, then maybe we could look into making it type-check? I, for one, would love to see a type-using decorator-based modeling library that enforces your types both before and after compilation/codegen. While I can accomplish the latter, the former currently requires a workaround something like:

@Model
class MyModel extends SomeBaseModelIWillOverwriteMethodsOnUsingADecorator {
  /* ... */
}

Which will type-check so long as that dummy base model includes a stub for validate () {...} in it.

I'll be the first to admit that this experiment, while it works, does not seem like it has an obvious type-checking solution. In playing around, I've even tried out using intersection types and an interface like:

@Model
class IUser {
  uid: number
}

export const User = IUser as any as IModel & User

Where IModel is an interface containing validate (...). This type-checks, but that's some goofy boilerplate whereas ... extends IModel { ... }, while sounding redundant, reads much more naturally. Plus, the second example syntax is clearly a compatibility layer for creating typings for a JS library, whereas I'd rather create a well-typed TypeScript library.

Sorry for the length of the comment, but hopefully it was insightful/constructive. I get the impression that I'm using this feature in a way other than it was intended!

@fongandrew
Copy link

Are there any plans to let class decorators add new members to the type in the type checker? With TS 2.2 out, it seems very odd that this doesn't work:

function CanRawr(target: new (...args: any[]) => object) {
    class Rawrable extends target {
      rawr () : void {
        console.log(`RAWR`)
      }
    }
    return Rawrable;
}

@CanRawr
class Dinosaur { }

let d = new Dinosaur();
d.rawr(); // Not OK

But this does:

// ... same CanRawr function

class Dinosaur {}
class TRex extends CanRawr(Dinosaur) {}

let t = new TRex();
t.rawr(); // OK

@mhegazy
Copy link
Contributor

mhegazy commented Apr 27, 2017

Are there any plans to let class decorators add new members to the type in the type checker? With TS 2.2 out, it seems very odd that this doesn't work:

this is tracked by #4881

@mhegazy
Copy link
Contributor

mhegazy commented Apr 27, 2017

Support for mixin classes should be available now, see: http://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-2.html#support-for-min-in-classes

@mhegazy mhegazy added the Working as Intended The behavior described is the intended behavior; this is not a bug label Apr 27, 2017
@mhegazy
Copy link
Contributor

mhegazy commented May 22, 2017

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

@mhegazy mhegazy closed this as completed May 22, 2017
@frankwallis
Copy link
Contributor

@rbuckton I think there may still be an issue here, this code compiles in 2.4.0 but does not in 2.4.1-insiders:

function injectLocalisation<T>(Target: ComponentClass<T>): ComponentClass<T> {

}
export const withLocalisation: ClassDecorator = injectLocalisation;

Gives the error:

Scripts/ui/app/localisation/localisation-decorator.ts(120,14): error TS2322: Type '<T>(Target: ComponentClass<T>) => ComponentClass<T>'
 is not assignable to type 'ClassDecorator'.
  Types of parameters 'Target' and 'target' are incompatible.
    Type 'TFunction' is not assignable to type 'ComponentClass<{}>'.
      Type 'Function' is not assignable to type 'ComponentClass<{}>'.
        Type 'Function' provides no match for the signature 'new (props?: {}, context?: any): Component<{}, ComponentState>'.

Where ComponentClass is as defined in react.d.ts:

    interface ComponentClass<P> {
        new (props?: P, context?: any): Component<P, ComponentState>;
        propTypes?: ValidationMap<P>;
        contextTypes?: ValidationMap<any>;
        childContextTypes?: ValidationMap<any>;
        defaultProps?: Partial<P>;
        displayName?: string;
    }

@j
Copy link

j commented Jan 29, 2018

I'm confused.. this issue was closed, but @mhegazy linked to supporting mixins, not decorators. Is this still not supported?

@rbuckton
Copy link
Member

No, currently a decorator does not affect types in the class. The Decorators proposal for adoption to ECMAScript differs from the TypeScript implementation and we are unlikely to make substantive changes to our Decorators support until the official proposal reaches Stage 3. In the meantime, we do offer support for mixins which gives you the ability to augment a type (per @mhegazy's comment above).

@microsoft microsoft locked and limited conversation to collaborators Jul 3, 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

7 participants