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

Request: Class Decorator Mutation #4881

Open
Gaelan opened this Issue Sep 20, 2015 · 188 comments

Comments

Projects
None yet
@Gaelan

Gaelan commented Sep 20, 2015

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

This comment has been minimized.

g162h3 commented Sep 21, 2015

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

This comment has been minimized.

red-0ne commented Oct 28, 2015

@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

This comment has been minimized.

Contributor

HerringtonDarkholme commented Nov 2, 2015

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

@andreas-karlsson

This comment has been minimized.

andreas-karlsson commented Jan 14, 2016

+1

@masaeedu

This comment has been minimized.

Contributor

masaeedu commented Feb 23, 2016

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

This comment has been minimized.

davojan commented Mar 29, 2016

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

@masaeedu

This comment has been minimized.

Contributor

masaeedu commented Mar 30, 2016

@davojan Sure. Here you go:

class A { }
namespace A {
    export let foo = function() { console.log("foo"); }
}
A.foo();
@nevir

This comment has been minimized.

nevir commented Jun 4, 2016

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

This comment has been minimized.

joyt commented Jun 21, 2016

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

This comment has been minimized.

JakeGinnivan commented Jun 28, 2016

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

This comment has been minimized.

Contributor

masaeedu commented Jun 28, 2016

@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

This comment has been minimized.

JakeGinnivan commented Jun 28, 2016

@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

This comment has been minimized.

blai commented Jun 28, 2016

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

This comment has been minimized.

blai commented Jun 28, 2016

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

This comment has been minimized.

nevir commented Jun 28, 2016

@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

This comment has been minimized.

blai commented Jun 28, 2016

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

@blai

This comment has been minimized.

blai commented Jun 28, 2016

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

This comment has been minimized.

seansfkelley commented Jul 4, 2016

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

@CarsonF CarsonF referenced this issue Jul 11, 2016

Closed

[WIP] Typescript definition for v6 #1318

1 of 3 tasks complete
@kgtkr

This comment has been minimized.

kgtkr commented Aug 3, 2018

When will this feature become available?

@ErikSchults

This comment has been minimized.

ErikSchults commented Aug 10, 2018

I wanted to use a class decorator to take care of repeated task.
But now when initializing the class, I get the following error: Expected 0 arguments, but got 1

function Component<T extends { new(...args: any[]): {} }>(target: T) {
	return class extends target {
		public constructor(...args: any[]) {
			super(...args);
			resolveDependencies(this, args[0])
		}
	}
}

@Component
export class ExampleService {
	@Inject(ExampleDao) private exampleDao: ExampleDao;

	// @Component will automatically do this for me 
	// public constructor(deps: any) {
	//	resolveDependencies(this, deps);
	// }

	public getExample(id: number): Promise<Example | undefined> {
		return this.exampleDao.getOne(id);
	}
}

new ExampleService({ exampleDao }) // TS2554: Expected 0 arguments, but got 1.

I hope this feature will be available soon! :)

@elderapo

This comment has been minimized.

elderapo commented Aug 10, 2018

@iainreid820 did you experience any weird bugs using this approach?

@AllNamesRTaken

This comment has been minimized.

AllNamesRTaken commented Aug 31, 2018

The long wait! In the meantime, is this getting solved by anything on the current road map?
Such as issue 5453?

@bikeshedder

This comment has been minimized.

bikeshedder commented Oct 15, 2018

I'm using Material UI and just had to realize that TypeScript doesn't support the decorator syntax for withStyles: https://material-ui.com/guides/typescript/#decorating-components

Please fix that limitation in the next TypeScript release. Class decorators seem pretty useless to me right now.

@emyann

This comment has been minimized.

emyann commented Oct 15, 2018

As a maintainer of Morphism Js, this is a big limitation for this kind of library. I would love to avoid the consumer of a Function Decorator to have to specify the target Type of the Function https://github.com/nobrainr/morphism#--toclassobject-decorator, otherwise using decorators instead of HOF sounds like a bit useless 😕
Is there any plan to tackle this ? Is there a way to help making this feature happen ? Thank you in advance!

@justinfagnani

This comment has been minimized.

justinfagnani commented Oct 15, 2018

@bikeshedder that Material UI example is a class mixin use case, and you can get the right type from mixins. Instead of:

const DecoratedClass = withStyles(styles)(
  class extends React.Component<Props> {
...
}

write:

class DecoratedClass extends withStyles(styles)(React.Component<Props>) {
...
}
@G-Rath

This comment has been minimized.

G-Rath commented Oct 15, 2018

@emyann

This comment has been minimized.

emyann commented Oct 15, 2018

@G-Rath I think new () => React.Component<Props, State> instead should work ?

@G-Rath

This comment has been minimized.

G-Rath commented Oct 15, 2018

@emyann No dice. I've updated the gist with the new code, but is this what you meant?

class CardSection extends withStyles(styles)(new () => React.Component<Props, State>) {

@dantman

This comment has been minimized.

dantman commented Oct 15, 2018

No matter how you format it, extends withStyles(styles)(...) does not sound like a proper suggestion, fundamentally. withStyles is NOT a class mixin.

withStyles takes component class A and creates class B which when rendered renders class A and passes props to it + the classes prop.

If you extend the return value of withStyles then you are extending the B wrapper, instead of implementing the A class that actually receives the classes prop.

@arackaf

This comment has been minimized.

arackaf commented Oct 15, 2018

No matter how you format it, extends withStyles(styles)(...) does not sound like a proper suggestion, fundamentally. withStyles is NOT a class mixin.

Indeed. Not sure why there's so much pushback here.

That said, decorators are extremely close to stage 3, from what I hear, so I imagine TS will be getting proper support here soon.

@sagar-sm

This comment has been minimized.

sagar-sm commented Oct 26, 2018

I hear the folks who want a cleaner syntax though, esp. when using applying multiple HoCs as decorators. The temporary workaround we use is to actually wrap multiple decorators inside a pipe function and then use that pure function to decorate the component. e.g.

@flow([
  withStyles(styles),
  connect(mapStateToProps),
  decorateEverything(),
])
export class HelloWorld extends Component<Props, State> {
  ...
}

where flow is lodash.flow. A lot of util libraries provide something like this - recompose, Rx.pipe etc. but you can of course write your own simple pipe function if you don't want to install a library.

I find this easier to read than not using decorators,

export const decoratedHelloWorld = withStyles(styles)(
  connect(mapStateToProps)(
    decorateEverything(
       HelloWorld
))))

Another reason I use this, is that this pattern is easily find-and-replace-able/grep-able once the decorator spec is announced and properly supported.

@trusktr

This comment has been minimized.

trusktr commented Nov 4, 2018

@justinfagnani Oddly I get a syntax error from ESLint when I try changing extends React.Component<Props, State> to extends withStyles(styles)(React.Component<Props, State>), so I couldn't verify @G-Rath's type error in the same way.

But I did notice that if I do something like follows then a problem with new (maybe the same problem?) appears:

class MyComponent extends React.Component<Props, State> {
  /* ... */
}

const _MyComponent = withStyles(styles)(MyComponent)
const test = new _MyComponent // <--------- ERROR

and the error is:

Cannot use 'new' with an expression whose type lacks a call or construct signature.

Does this mean the type returned by Material UI is not a constructor (though it should be)?

@trusktr

This comment has been minimized.

trusktr commented Nov 4, 2018

@sagar-sm But, do you get the correct type augmented by the Material UI type? Doesn't seem like you will.

@trusktr

This comment has been minimized.

trusktr commented Nov 4, 2018

For reference, I stumbled on this because I was also trying to use Material UI's withStyles as a decorator, which didn't work so I asked this question: https://stackoverflow.com/questions/53138167

@bluelovers

This comment has been minimized.

Contributor

bluelovers commented Nov 11, 2018

need this, then we can make async function return as bluebird

@DeusProx

This comment has been minimized.

DeusProx commented Nov 15, 2018

I tried to do something similiar. ATM I'm using following workaround:

First here's my "mixin" decorator

export type Ctor<T = {}> = new(...args: any[]) => T 
function mixinDecoratorFactory<MixinInterface>() {
    return function(toBeMixed: MixinInterface) {
        return function<MixinBase extends Ctor>(MixinBase: MixinBase) {
            Object.assign(MixinBase.prototype, toBeMixed)
            return class extends MixinBase {} as MixinBase & Ctor<MixinInterface>
        }
    }
}

Create a decorator from interface

export interface ComponentInterface = {
    selector: string,
    html: string
}
export const Component = mixinDecoratorFactory<ComponentInterface>();

And that's how I use it:

@Component({
    html: "<div> Some Text </div>",
    selector: "app-test"
})
export class Test extends HTMLElement {
    test = "test test"
    constructor() {
        super()
        console.log("inner;     test:", this.test)
        console.log("inner;     html:", this.html)
        console.log("inner; selector:", this.selector)
    }
}

export interface Test extends HTMLElement, ComponentInterface {}
window.customElements.define(Test.prototype.selector, Test)


const test = new Test();
console.log("outer;     test:", test.test)
console.log("outer;     html:", test.html)
console.log("outer; selector:", test.selector)

The Trick is to also create a an interface with the same name as the class to create a merged declaration.
Still the class only shows as type Test but checkings from typescript are working.
If you would use the decorator without the @-Notation an simply invoke it as a Function you would get the right intersection-Type but lose the ability of type-checking inside the class itself since you can't use the interface trick anymore and it looks more ugly. E.g.:

let Test2Comp = Component({
    html: "<div> Some Text 2 </div>",
    selector: "app-test2"
}) (
class Test2 extends HTMLElement {
    test = "test test"
    constructor() {
        super()
        console.log("inner;     test:", this.test)
        console.log("inner;     html:", this.html)     // no
        console.log("inner; selector:", this.selector) // no
    }
})

interface Test2 extends HTMLElement, ComponentInterface {} //no
window.customElements.define(Test2Comp.prototype.selector, Test2Comp)

const test2 = new Test2Comp();
console.log("outer;     test:", test2.test)
console.log("outer;     html:", test2.html)
console.log("outer; selector:", test2.selector)

What do you say about these approaches? It's not beautiful but works as far as a workaround.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment