microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
98.32k stars 12.21k forks source link

Request: Class Decorator Mutation #4881

Open Gaelan opened 8 years ago

Gaelan commented 8 years ago

If we can get this to type check properly, we would have perfect support for boilerplate-free mixins:

declare function Blah<T>(target: T): T & {foo: number}

@Blah
class Foo {
    bar() {
        return this.foo; // Property 'foo' does not exist on type 'Foo'
    }
}

new Foo().foo; // Property 'foo' does not exist on type 'Foo'
g162h3 commented 8 years ago

Same would be useful for methods:

class Foo {
  @async
  bar(x: number) {
    return x || Promise.resolve(...);
  }
}

The async decorator is supposed to change the return type to Promise<any>.

red-0ne commented 8 years ago

@Gaelan, this is exactly what we are needing here! It would make mixins just natural to work with.

class asPersistent {
  id: number;
  version: number;
  sync(): Promise<DriverResponse> { ... }
  ...
}

function PersistThrough<T>(driver: { new(): Driver }): (t: T) => T & asPersistent {
  return (target: T): T & asPersistent {
    Persistent.call(target.prototype, driver);
    return target;
  }
}

@PersistThrough(MyDBDriver)
Article extends TextNode {
  title: string;
}

var article = new Article();
article.title = 'blah';

article.sync() // Property 'sync' does not exist on type 'Article'
HerringtonDarkholme commented 8 years ago

+1 for this. Though I know this is hard to implement, and probably harder to reach an agreement on decorator mutation semantics.

andreas-karlsson commented 8 years ago

+1

masaeedu commented 8 years ago

If the primary benefit of this is introducing additional members to the type signature, you can already do that with interface merging:

interface Foo { foo(): number }
class Foo {
    bar() {
        return this.foo();
    }
}

Foo.prototype.foo = function() { return 10; }

new Foo().foo();

If the decorator is an actual function that the compiler needs to invoke in order to imperatively mutate the class, this doesn't seem like an idiomatic thing to do in a type safe language, IMHO.

davojan commented 8 years ago

@masaeedu Do you know any workaround to add a static member to the decorated class?

masaeedu commented 8 years ago

@davojan Sure. Here you go:

class A { }
namespace A {
    export let foo = function() { console.log("foo"); }
}
A.foo();
nevir commented 7 years ago

It would also be useful to be able to introduce multiple properties to a class when decorating a method (for example, a helper that generates an associated setter for a getter, or something along those lines)

joyt commented 7 years ago

The react-redux typings for connect takes a component and returns a modified component whose props don't include the connected props received through redux, but it seems TS doesn't recognize their connect definition as a decorator due to this issue. Does anyone have a workaround?

JakeGinnivan commented 7 years ago

I think the ClassDecorator type definition needs changing.

Currently it's declare type ClassDecorator = <TFunction extends Function>(target: TFunction) => TFunction | void;. Maybe it could be changed to

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

Obviously the naming sucks and I have no idea if this sort of thing will work (I am just trying to convert a Babel app over to typescript and am hitting this).

masaeedu commented 7 years ago

@joyt Could you provide a playground reconstruction of the problem? I don't use react-redux, but as I've mentioned before, I think any extensions you desire to the shape of a type can be declared using interface merging.

JakeGinnivan commented 7 years ago

@masaeedu here is a basic breakdown of the moving parts..

Basically the decorator provides a bunch of the props to the React component, so the generic type of the decorator is a subset of the decorated component, not a superset.

Not sure if this is helpful, but tried to put together a non-runnable sample to show you the types in play.

// React types
class Component<TProps> {
    props: TProps
}
class ComponentClass<TProps> {
}
interface ComponentDecorator<TOriginalProps, TOwnProps> {
(component: ComponentClass<TOriginalProps>): ComponentClass<TOwnProps>;
}

// Redux types
interface MapStateToProps<TStateProps, TOwnProps> {
    (state: any, ownProps?: TOwnProps): TStateProps;
}

// Fake react create class
function createClass(component: any, props: any): any {
}

// Connect wraps the decorated component, providing a bunch of the properies
// So we want to return a ComponentDecorator which exposes LESS than
// the original component
function connect<TStateProps, TOwnProps>(
    mapStateToProps: MapStateToProps<TStateProps, TOwnProps>
): ComponentDecorator<TStateProps, TOwnProps> {
    return (ComponentClass) => {
        let mappedState = mapStateToProps({
            bar: 'bar value'
        })
        class Wrapped {
            render() {
                return createClass(ComponentClass, mappedState)
            }
        }

        return Wrapped
    }
}

// App Types
interface AllProps {
    foo: string
    bar: string
}

interface OwnProps {
    bar: string
}

// This does not work...
// @connect<AllProps, OwnProps>(state => state.foo)
// export default class MyComponent extends Component<AllProps> {
// }

// This does
class MyComponent extends Component<AllProps> {
}
export default connect<AllProps, OwnProps>(state => state.foo)(MyComponent)
//The type exported should be ComponentClass<OwnProps>,
// currently the decorator means we have to export ComponentClass<AllProps>

If you want a full working example I suggest pulling down https://github.com/jaysoo/todomvc-redux-react-typescript or another sample react/redux/typescript project.

blai commented 7 years ago

According to https://github.com/wycats/javascript-decorators#class-declaration, my understanding is that the proposed declare type WrappingClassDecorator = <TFunction extends Function, TDecoratorFunction extends Function>(target: TFunction) => TDecoratorFunction; is invalid.

blai commented 7 years ago

The spec says:

@F("color")
@G
class Foo {
}

is translate to:

var Foo = (function () {
  class Foo {
  }

  Foo = F("color")(Foo = G(Foo) || Foo) || Foo;
  return Foo;
})();

So if I understand it correctly, the following should be true:

declare function F<T>(target: T): void;

@F
class Foo {}

let a: Foo = new Foo(); // valid
class X {}
declare function F<T>(target: T): X;

@F
class Foo {}

let a: X = new Foo(); // valid
let b: Foo = new Foo(); // INVALID
declare function F<T>(target: T): void;
declare function G<T>(target: T): void;

@F
@G
class Foo {}

let a: Foo = new Foo(); // valid
class X {}
declare function F<T>(target: T): void;
declare function G<T>(target: T): X;

@F
@G
class Foo {}

@G
class Bar {}

@F
class Baz {}

let a: Foo = new Foo(); // valid
let b: X = new Foo(); // INVALID
let c: X = new Bar(); // valid
let d: Bar = new Bar(); // INVALID
let e: Baz = new Baz(); // valid
class X {}
declare function F<T>(target: T): X;
declare function G<T>(target: T): void;

@F
@G
class Foo {}

@G
class Bar {}

@F
class Baz {}

let a: X = new Foo(); // valid
let b: Bar = new Bar(); // valid
let c: X = new Baz(); // valid
let d: Baz = new Baz(); // INVALID
nevir commented 7 years ago

@blai

For your example:

class X {}
declare function F<T>(target: T): X;

@F
class Foo {}

let a: X = new Foo(); // valid
let b: Foo = new Foo(); // INVALID

I'm assuming you mean that F returns a class that conforms to X (and is not an instance of X)? E.g:

declare function F<T>(target: T): typeof X;

For that case, the assertions should be:

let a: X = new Foo(); // valid
let b: Foo = new Foo(); // valid

The Foo that is in scope of those let statements has been mutated by the decorator. The original Foo is no longer reachable. It's effectively equivalent to:

let Foo = F(class Foo {});
blai commented 7 years ago

@nevir Yep, you are right. Thanks for clarification.

blai commented 7 years ago

On a side note, it seems like turning off the check to invalidate mutated class types is relatively easy:

diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts
index 06591a7..2320aff 100644
--- a/src/compiler/checker.ts
+++ b/src/compiler/checker.ts
@@ -11584,10 +11584,6 @@ namespace ts {
           */
         function getDiagnosticHeadMessageForDecoratorResolution(node: Decorator) {
             switch (node.parent.kind) {
-                case SyntaxKind.ClassDeclaration:
-                case SyntaxKind.ClassExpression:
-                    return Diagnostics.Unable_to_resolve_signature_of_class_decorator_when_called_as_an_expression;
-
                 case SyntaxKind.Parameter:
                     return Diagnostics.Unable_to_resolve_signature_of_parameter_decorator_when_called_as_an_expression;
         }

         /** Check a decorator */
        function checkDecorator(node: Decorator): void {
             const signature = getResolvedSignature(node);
             const returnType = getReturnTypeOfSignature(signature);
             if (returnType.flags & TypeFlags.Any) {
@@ -14295,9 +14291,7 @@ namespace ts {
             let errorInfo: DiagnosticMessageChain;
             switch (node.parent.kind) {
                 case SyntaxKind.ClassDeclaration:
-                    const classSymbol = getSymbolOfNode(node.parent);
-                    const classConstructorType = getTypeOfSymbol(classSymbol);
-                    expectedReturnType = getUnionType([classConstructorType, voidType]);
+                    expectedReturnType = returnType;
                     break;

                 case SyntaxKind.Parameter:
         }

But I am not knowledgable enough to make the compiler output the correct type definitions of the mutated class. I have the following test:

tests/cases/conformance/decorators/class/decoratorOnClass10.ts

// @target:es5
// @experimentaldecorators: true
class X {}
class Y {}

declare function dec1<T>(target: T): T | typeof X;
declare function dec2<T>(target: T): typeof Y;

@dec1
@dec2
export default class C {
}

var c1: X | Y = new C();
var c2: X = new C();
var c3: Y = new C();

It generates tests/baselines/local/decoratorOnClass10.types

=== tests/cases/conformance/decorators/class/decoratorOnClass10.ts ===
class X {}
>X : X

class Y {}
>Y : Y

declare function dec1<T>(target: T): T | typeof X;
>dec1 : <T>(target: T) => T | typeof X
>T : T
>target : T
>T : T
>T : T
>X : typeof X

declare function dec2<T>(target: T): typeof Y;
>dec2 : <T>(target: T) => typeof Y
>T : T
>target : T
>T : T
>Y : typeof Y

@dec1
>dec1 : <T>(target: T) => T | typeof X

@dec2
>dec2 : <T>(target: T) => typeof Y

export default class C {
>C : C
}

var c1: X | Y = new C();
>c1 : X | Y
>X : X
>Y : Y
>new C() : C
>C : typeof C

var c2: X = new C();
>c2 : X
>X : X
>new C() : C
>C : typeof C

var c3: Y = new C();
>c3 : Y
>Y : Y
>new C() : C
>C : typeof C

I was expecting >C: typeof C to be >C: typeof X | typeof Y

seansfkelley commented 7 years ago

For those interested in react-redux's connect as a case study for this feature, I've filed https://github.com/DefinitelyTyped/DefinitelyTyped/issues/9951 to track the issue in one place.

alex-bel commented 7 years ago

I've read all comments on this issue and got an idea that decorator's signature doesn't actually shows what it can do with wrapped class.

Consider this one:

function decorator(target) {
    target.prototype.someNewMethod = function() { ... };
    return new Wrapper(target);
}

It should be typed in that way: declare function decorator<T>(target: T): Wrapper<T>;

But this signature doesn't tell us that decorator has added new things to the target's prototype.

On the other hand, this one doesn't tell us that decorator has actually returned a wrapper: declare function decorator<T>(target: T): T & { someMethod: () => void };

jeffijoe commented 7 years ago

Any news on this? This would be super powerful for metaprogramming!

HerringtonDarkholme commented 7 years ago

What about a simpler approach to this problem? For a decorated class, we bind the class name to the decorator return value, as a syntactic sugar.

declare function Blah<T>(target: T): T & {foo: number}

@Blah
class Foo {
    bar() {
        return this.foo; // Property 'foo' does not exist on type 'Foo'
    }
}

// is desugared to
const Foo = Blah(class Foo {
  // this.foo is not available here
})

new Foo.foo // foo is available here.

Implementation-wise, this will introduce one synthetic symbol for decorated class. And the original class name is only bound to class body scope.

aluanhaddad commented 7 years ago

@HerringtonDarkholme I think that would be a nicely pragmatic approach that would provide most of the expressiveness desired. Great Idea!

I definitely want to see this someday

I often write a class for Angular 2 or for Aurelia, that looks like this:

import {Http} from 'aurelia-fetch-client';
import {User} from 'models';

// accesses backend routes for 'api/user'
@autoinject export default class UserService {
  constructor(readonly http : Http) { }

  readonly resourceUrl = 'api/users';

  async get(id: number) {
    const response = await this.http.fetch(this.resourceUrl);
    const user = await response.json() as User;
    return user;
  }

  async post(id: number, model: { [K in keyof User]?: User[K] }) {
    const response = await this.http.post(`${this.resourceUrl}/`${id}`, model);
    return await response.json();
  }
}

What I want to write is something like decorators/api-client.ts

import {Http} from 'aurelia-fetch-client';

export type Target = { name; new (...args): { http: Http }};

export default function apiClient<T extends { id: string }>(resourceUrl: string) {
  return (target: Target)  => {
    type AugmentedTarget = Target & { get(id: number): Promise<T>, post(id, model: Partial<T>) };
    const t = target as AugmentedTarget;
    t.prototype.get = async function (id: number) {
      const response = await this.http.fetch(resourceUrl);
      return await response.json() as T;
    }
  }
}

and then I could generically apply it like

import {Http} from 'aurelia-fetch-client';
import apiClient from ./decorators/api-client
import {User} from 'models';

@apiClient<User>('api/users') export default class UserService {
  constructor(readonly http : Http) { }
}

with no loss of typesafety. This would be a boon for writing clean, expressive code.

shlomiassaf commented 7 years ago

Reviving this issue.

Now that #13743 is out and mixin support is in the language this is a super useful feature.

@HerringtonDarkholme is less suitable for this case though, having to declare the return type of the decorator looses some dynamic features...

@ahejlsberg, @mhegazy Do you think this is doable?

codeandcats commented 7 years ago

I have another usage scenario I'm not sure is yet covered by this conversation but probably falls under the same umbrella.

I would like to implement a method decorator that changes the type of the method entirely (not the return type or parameters but the entire function). e.g.

type AsyncTask<Method extends Function> = {
    isRunning(): boolean;
} & Method;

// Decorator definition...
function asyncTask(target, methodName, descriptor) {
    ...
}

class Order {
    @asyncTask
    async save(): Promise<void> {
        // Performs an async task and returns a promise
        ...
    }
}

const order = new Order();

order.save();
order.save.isRunning(); // Returns true

Totally possible in JavaScript, that's not the problem obviously, but in TypeScript I need the asyncTask decorator to change the type of the decorated method from () => Promise<void> to AsyncTask<() => Promise<void>>.

Pretty sure this isn't possible now and probably falls under the umbrella of this issue?

jeffijoe commented 7 years ago

@codeandcats your example is the exact same use case I am here for!

codeandcats commented 7 years ago

Hi @ohjames, forgive me, I'm having trouble groking your example, any chance you could rewrite into something that works as is in the playground?

zajrik commented 7 years ago

Any progress on this? I had this in my head all day, unaware of this issue, went to go implement it only to find out that the compiler doesn't pick up on it. I have a project that could use a better logging solution so I wrote a quick singleton to later expand into a full-fledged logger that I was going to attach to classes via a decorator like

@loggable
class Foo { }

and I wrote the necessary code for it

type Loggable<T> = T & { logger: Logger };

function loggable<T extends Function>(target: T): Loggable<T>
{
    Object.defineProperty(target.prototype, 'logger',
        { value: Logger.instance() });
    return <Loggable<T>> target;
}

and the logger property is definitely present at runtime but regrettably not picked up by the compiler.

I would love to see some resolution to this issue, especially since a runtime construct like this should absolutely be able to be properly represented at compile-time.

I ended up settling for a property decorator just to get me by for now:

function logger<T>(target: T, key: string): void
{
    Object.defineProperty(target, 'logger',
        { value: Logger.instance() });
}

and attaching it to classes like

class Foo {
    @logger private logger: Logger;
    ...

but this is far more boilerplate per class utilizing the logger than a simple @loggable class decorator. I suppose I could feasibly typecast like (this as Loggable<this>).logger but this is also pretty far from ideal, especially after doing it a handful of times. It'd get tiresome very quickly.

jeffijoe commented 7 years ago

I had to can TS for an entire app mainly cause I was unable to get https://github.com/jeffijoe/mobx-task working with decorators. I hope this will be addressed soon. 😄

insidewhy commented 7 years ago

It's very irritating in the Angular 2 ecosystem where decorators and TypeScript are treated as first class citizens. Yet the minute you try to add a property with a decorator the TypeScript compiler says no. I would have thought the Angular 2 team would show some interest in this issue.

justinfagnani commented 7 years ago

@zajrik you can accomplish what you want with class mixins that have been supported with proper typing since TS 2.2:

Define your Loggable mixin like so:

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

interface Logger {}

// You don't strictly need this interface, type inference will determine the shape of Loggable,
// you only need it if you want to refer to Loggable in a type position.
interface Loggable {
  logger: Logger;
}

function Loggable<T extends Constructor<object>>(superclass: T) {
  return class extends superclass {
    logger: Logger;
  };
}

and then you can use it in a few ways. Either in the extends clause of a class declaration:

class Foo {
  superProperty: string;
}

class LoggableFoo extends Loggable(Foo) {
  subProperty: number;
}

TS knows that instances of LoggableFoo have superProperty, logger, and subProperty:

const o = new LoggableFoo();
o.superProperty; // string
o.logger; // Logger
o.subProperty; // number

You can also use a mixin as an expression that returns the concrete class you want to use:

const LoggableFoo = Loggable(Foo);

You can also use a class mixin as a decorator, but it has some slightly different semantics, mainly that is subclasses your class, rather than allowing your class to subclass it.

Class mixins have several advantages over decorators, IMO:

  1. They create a new superclass, so that the class you apply them to has a change to override them
  2. They type check now, without any additional features from TypeScript
  3. They work well with type inference - you don't have to type the return value of the mixin function
  4. They work well with static analysis, especially jump-to-definition - Jumping to the implementation of logger takes you to the mixin implementation, not the interface.
zajrik commented 7 years ago

@justinfagnani I hadn't even considered mixins for this so thank you. I'll go ahead and write up a Loggable mixin tonight to make my Logger attachment syntax a bit nicer. The extends Mixin(SuperClass) route is my preferred as it's how I've used mixins so far since the release of TS 2.2.

I do prefer the idea of decorator syntax to mixins, however, so I do still hope some resolution can be had for this specific issue. Being able to create boilerplate-free mixins using decorators would be a huge boon to cleaner code, in my opinion.

justinfagnani commented 7 years ago

@zajrik glad the suggestion helped, I hope

I still don't quite understand how mixins have more boilerplate than decorators. They're nearly identical in syntactic weight:

Class Mixin:

class LoggableFoo extends Loggable(Foo) {}

vs Decorator:

@Loggable
class LoggableFoo extends Foo {}

In my opinion, the mixin is way more clear about its intention: it's generating a superclass, and superclasses define members of a class, so the mixin is probably defining members as well.

Decorators will be used for so many things that you can't assume that is or isn't defining members. It could simply be registering the class for something, or associating some metadata with it.

codeandcats commented 7 years ago

To be fair I think what @zajrik wants is:

@loggable
class Foo { }

Which is undeniably, if ever so slightly, less boilerplate.

That said, I love the mixin solution. I keep forgetting that mixins are a thing.

insidewhy commented 7 years ago

If all you care about is adding properties to the current class then mixins are basically equivalent to decorators with one significant annoyance... if your class doesn't already have a superclass you need to create an empty superclass to use them. Also the syntax seems heavier in general. Also it isn't clear if parametric mixins are supported (is extends Mixin(Class, { ... }) allowed).

insidewhy commented 7 years ago

@justinfagnani in your list of reasons, points 2-4 are actually deficiencies in TypeScript not advantages of mixins. They don't apply in a JS world.

insidewhy commented 7 years ago

I think we should all be clear that a mixin based solution to OPs problem would involve adding two classes to the prototype chain, one of which is useless. This reflects the semantic differences of mixins Vs decorators though, mixins give you a chance to intercept the parent class chain. However 95% of the time this isn't what people want to do, they want to decorate this class. Whilst mixins have their limited uses i think promoting them as an alternative to decorators and higher order classes is semantically inappropriate.

justinfagnani commented 7 years ago

Mixins are basically equivalent to decorators with one significant annoyance... if your class doesn't already have a superclass you need to create an empty superclass to use them

This isn't necessarily true:

function Mixin(superclass = Object) { ... }

class Foo extends Mixin() {}

Also the syntax seems heavier in general.

I just don't see how this is so, so we'll have to disagree.

Also it isn't clear if parametric mixins are supported (is extends Mixin(Class, { ... }) allowed).

They very much are. Mixins are just functions.

in your list of reasons, points 2-4 are actually deficiencies in TypeScript not advantages of mixins. They don't apply in a JS world.

This is a TypeScript issue, so they apply here. In the JS world decorators don't actually exist yet.

I think we should all be clear that a mixin based solution to OPs problem would involve adding two classes to the prototype chain, one of which is useless.

I'm not clear where you get two. It's one, just like the decorator might do, unless it's patching. And which prototype is useless? The mixin application presumably adds a property/method, that's not useless.

This reflects the semantic differences of mixins Vs decorators though, mixins give you a chance to intercept the parent class chain. However 95% of the time this isn't what people want to do, they want to decorate this class.

I'm no so sure this is true. Usually when defining a class you expect it to be at the bottom of the inheritance hierarchy, with the ability to override superclass methods. Decorators either have to patch the class, which has numerous problems, including not working with super(), or extend it, in which case the decorated class does not have an ability to override the extension. This can be useful in some cases, like a decorators that overrides every defined method of the class for performance/debugging tracing, but it's far from the usual inheritance model.

Whilst mixins have their limited uses i think promoting them as an alternative to decorators and higher order classes is semantically inappropriate.

When a developer wants to add members to the prototype chain, mixins are exactly semantically appropriate. In every case that I've seen someone want to use decorators for mixins, using class mixins would accomplish the same task, with the semantics that they're actually expecting out of decorators, more flexibility due to working property with super calls, and of course they work now.

Mixins are hardly inappropriate when they directly address the use case.

insidewhy commented 7 years ago

When a developer wants to add members to the prototype chain

That's exactly my point, the OP doesn't want to add anything to the prototype chain. He just wants to mutate a single class, and mostly when people use decorators they don't even have a parent class other than Object. And for some reason Mixin(Object) doesn't work in TypeScript so you have to add a dummy empty class. So now you have a prototype chain of 2 (not including Object) when you don't need it. Plus there is a non-trivial cost to adding new classes to the prototype chain.

insidewhy commented 7 years ago

As for the syntax compare Mixin1(Mixin2(Mixin3(Object, { ... }), {... }), {...}). The parameters for each mixin are as far from the mixin-class as they could be. Decorator syntax is clearly more readable.

masaeedu commented 7 years ago

While decorator syntax per-se doesn't type check, you can just use regular function invocation to get what you want:

class Logger { static instance() { return new Logger(); } }
type Loggable<T> = T & { logger: Logger };
function loggable<T, U>(target: { new (): T } & U): { new (): Loggable<T> } & U
{
    // ...
}

const Foo = loggable(class {
    x: string
});

let foo = new Foo();
foo.logger; // Logger
foo.x; // string

It's just a little annoying that you have to declare your class as const Foo = loggable(class {, but aside from that it all works.

nevir commented 7 years ago

@ohjames (cc @justinfagnani) you have to be careful when extending builtins such as Object (since they bash over your subclass's prototype in instances): https://github.com/Microsoft/TypeScript/wiki/FAQ#why-doesnt-extending-built-ins-like-error-array-and-map-work

insidewhy commented 7 years ago

@nevir yep, I already tried @justinfagnani's suggestion of using a mixin with a default Object parameter in the past with TypeScript 2.2 and tsc rejects the code.

nevir commented 7 years ago

@ohjames it still works, you just have to be careful about the prototype in the default case (see that FAQ entry).

Though, it's generally easier to rely on tslib.__extend's behavior [when passed null](https://www.typescriptlang.org/play/index.html#src=type%20Constructor%20%3D%20%7B%20new%20(...args%3A%20any%5B%5D)%20%7D%3B%0A%0Afunction%20LoggerMixin(Superclass%3A%20Constructor%20%3D%20null)%20%7B%0A%20%20%2F%2F%20We%20have%20to%20be%20careful%20with%20our%20default%20case%20due%20to%0A%20%20%2F%2F%20https%3A%2F%2Fgithub.com%2FMicrosoft%2FTypeScript%2Fwiki%2FFAQ%23why-doesnt-extending-built-ins-like-error-array-and-map-work%0A%20%20console.log('super%3A'%2C%20Superclass)%3B%0A%0A%20%20class%20LoggerMixinClass%20extends%20Superclass%20%7B%0A%20%20%20%20log(message%3A%20string)%20%7B%0A%20%20%20%20%20%20console.log(message)%3B%0A%20%20%20%20%20%20%2F%2F%20For%20easy%20viewing%20when%20you%20hit%20%22run%22%0A%20%20%20%20%20%20document.write(%60%3Cp%3E%24%7Bmessage%7D%3C%2Fp%3E%60)%3B%0A%20%20%20%20%7D%0A%20%20%7D%0A%20%20%2F%2F%20If%20we%20passed%20null%20to%20__extends%2C%20we%20need%20to%20fix%20up%20the%20prototype%20of%0A%20%20%2F%2F%20the%20constructor.%0A%20%20if%20(Superclass%20%3D%3D%3D%20null)%20%7B%0A%20%20%20%20Object.setPrototypeOf(LoggerMixinClass%2C%20Function.prototype)%3B%0A%20%20%7D%0A%0A%20%20return%20LoggerMixinClass%3B%0A%7D%0A%0A%0A%2F%2F%20Without%20a%20base%20class.%0A%2F%2F%20%0A%2F%2F%20Prototype%20chain%20is%20Loggable%20-%3E%20mixin%20-%3E%20Object%0Aclass%20Loggable%20extends%20LoggerMixin()%20%7B%0A%0A%20%20doSomething()%20%7B%0A%20%20%20%20this.log('doing%20things%20without%20a%20base')%3B%0A%20%20%7D%0A%0A%7D%0A%0A%0Aclass%20Base%20%7B%0A%20%20base%20%3D%20123%3B%0A%7D%0A%0A%2F%2F%20With%20a%20base%20class.%0A%2F%2F%0A%2F%2F%20Prototype%20chain%20is%20Loggable%20-%3E%20mixin%20-%3E%20Base%20-%3E%20Object%0Aclass%20LoggableWithBase%20extends%20LoggerMixin(Base)%20%7B%0A%0A%20%20doSomething()%20%7B%0A%20%20%20%20this.log(%60doing%20things%3A%20%24%7Bthis.base%7D%60)%3B%0A%20%20%7D%0A%0A%7D%0A%0A%0A%2F%2F%20Check%20it%20out%20in%20the%20console%20when%20running%20it%3A%0Aconst%20withoutBase%20%3D%20new%20Loggable%3B%0AwithoutBase.doSomething()%3B%20%2F%2F%20%22doing%20things%20without%20a%20base%22%0A%0Aconst%20withBase%20%3D%20new%20LoggableWithBase%3B%0AwithBase.doSomething()%3B%20%2F%2F%20%22doing%20things%3A%20123%22)

aight8 commented 7 years ago

Any plans to focus this issue in the next iteration step? The benefits of this feature is extreme high accross so much libraries.

TomMarius commented 7 years ago

I just ran into this issue - it forces me to write a lot of unneeded code. Resolving this issue would be a huge help to any decorator-based framework/library.

masaeedu commented 7 years ago

@TomMarius As I've mentioned earlier, classes wrapped in decorator functions already type check properly, you just can't use the @ syntax sugar. Instead of doing:

@loggable
class Foo { }

you just need to do:

const Foo = loggable(class { });

You can even compose a bunch of decorator functions together before wrapping a class in them. While making the syntax sugar work properly is valuable, it doesn't seem like this should be such a huge pain point as things are.

zajrik commented 7 years ago

@masaeedu Really the issue is not external but internal type support. Being able to use the properties the decorator adds within the class itself without compilation errors is the desired result, at least for me. The example you've provided would only give Foo the loggable type but would not afford the type to the class definition itself.

masaeedu commented 7 years ago

@zajrik A decorator returns a new class from an original class, even when you use the built in @ syntax. Obviously JS doesn't enforce purity, so you're free to mutate the original class you are passed, but this is incongruent with idiomatic use of the decorator concept. If you're tightly coupling functionality that you're adding via decorators to the class internals, they may as well be internal properties.

Can you give me an example of a use case for a class internally consuming API that is added at some later point via decorators?

nevir commented 7 years ago

The Logger example above is a good example of a common want for being able to manipulate the internals of the decorated class. (And is familiar to people coming from other languages with class decoration; such as Python)

That said, @justinfagnani's class mixin suggestion seems like a good alternative for that case

justinfagnani commented 7 years ago

If you want to be able to define the internals of a class, the structured way to do this isn't to patch the class, or define a new subclass, both which TypeScript will have a hard time reasoning about in the context of the class itself, but to either just define things in the class itself, or create a new superclass that has the needed properties, which TypeScript can reason about.

Decorators really shouldn't change the shape of a class in a way that's visible to the class or most consumers. @masaeedu is right on here.