microsoft / TypeScript

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

Ability to decorate abstract member function #37067

Open thynson opened 4 years ago

thynson commented 4 years ago

Search Terms

I search "decorate abstract member" in the issues list, and #20887 has memtioned this issue but closed.

Suggestion

Ability to decorate abstract function and its params.

Use Cases

To implement a retrofit-like http request library with Typescript. Examples from retrofit website:

public interface GitHubService {
  @GET("users/{user}/repos")
  Call<List<Repo>> listRepos(@Path("user") String user);
}

When translate it into Typescript, we need to decorate an abstract class since interface will be erased after compilation. But,

@MyAPI() // Okay, abstract class is decoratable
abstract class GitHubService {
  @GET("users/{user}/repos")  // ERROR: Cannot decorate an abstract class member
  abstract Call<List<Repo>> listRepos(@Path("user") user: string);  // ERROR 
}

To workaround such limitation, we cannot use abstract class, so it turns out to be

@MyAPI() // Okay, abstract class is decoratable
class GitHubService {
  @GET("users/{user}/repos") 
  Call<List<Repo>> listRepos(@Path("user") user: user) {  
    throw new Error('unimplemented');
  } 
}

This is obviousely not elegant.

I think such ability can be implemented without breaks existing valid code. Decorator function can determin wheter it's decorating an abstract member by check if value of property descriptor is undefined.

function ClassMemberDecorator(prototype: {}, name: string, pd: PropertyDescriptor) {
  if (typeof pd.value === 'undefined') { // we're decorating abstract member
  }
}

function ClassMemberParamDecorator(prototype: {}, name: string, index: number) {
   if (typeof prototype[name] === 'undefiend') { // we're decorating abstract member param
   }
}

Since when targetting ES3, PropertyDescriptor.value is always undefined, this feature shall only be supported when targetting ES5-onward.

Checklist

My suggestion meets these guidelines:

eun-ice commented 4 years ago

+1, would really love to have it. My use case is generating IPC proxies for classes in an electron app.

Hookyns commented 3 years ago

I'd love to see this implemented.

And not just for abstract classes but for interfaces too. It's handy as a hint for TypeScript compiler - custom transformers. No need to emit anything into final JavaScript code in case of interfaces.

I work on TypeScript runtime reflection which generates a lof of usefull data to metadata library you can read at runtime. You can lookup functions, interfaces, classes their constructors, methods, parameters, decorators etc. It would be useful if I could annotate interfaces so I can for example find them, find their implementations and apply something specified on interface to the implementations.

ppoulard commented 2 years ago

The ability to decorate a member function of an abstract class is not the same as for an interface : an interface is not in the value space and is erased after the compilation.

Conversely, an abstract class is just turned to a class in JS, it is even possible to instanciate it.

So far, typescript doesn't allow that, so I used such ugly code instead :

function Foo(target: any) {} // class decorator

function Bar(target: any, propertyKey?: string) {} // method decorator

@Foo
abstract class Test {
    @Bar
    existingMethod(): Date
        { throw '' } // dummy code (here is the ugly code)

}

const t : Test = new (Test as any)();
// just to show that instanciation of an abstract class is not a problem at runtime

Then, I show you how JS code is generated (copy/paste from typescript playground) :

var __decorate = (this && this.__decorate) || function (decorators, target, key, desc) {
    var c = arguments.length, r = c < 3 ? target : desc === null ? desc = Object.getOwnPropertyDescriptor(target, key) : desc, d;
    if (typeof Reflect === "object" && typeof Reflect.decorate === "function") r = Reflect.decorate(decorators, target, key, desc);
    else for (var i = decorators.length - 1; i >= 0; i--) if (d = decorators[i]) r = (c < 3 ? d(r) : c > 3 ? d(target, key, r) : d(target, key)) || r;
    return c > 3 && r && Object.defineProperty(target, key, r), r;
};
var __metadata = (this && this.__metadata) || function (k, v) {
    if (typeof Reflect === "object" && typeof Reflect.metadata === "function") return Reflect.metadata(k, v);
};
function Foo(target) { }
function Bar(target, propertyKey) { }
let Test = class Test {
    existingMethod() { throw ''; }
};
__decorate([
    Bar,
    __metadata("design:type", Function),
    __metadata("design:paramtypes", []),
    __metadata("design:returntype", Date)
], Test.prototype, "existingMethod", null);

// below, some code that would have been generated on an abstract method, if that were allowed
__decorate([
    Bar,
    __metadata("design:type", Function),
    __metadata("design:paramtypes", []),
    __metadata("design:returntype", Date)
], Test.prototype, "nonExistingMethod", null);
// end of addon

Test = __decorate([
    Foo
], Test);
const t = new Test();

I have inserted the code that would be generated if an abstract method nonExistingMethod() were set on Test class.

I also run this code, with and without the presence of Reflect.decorate (because its presence is checked in the generated code) and everything was fine.

With that abstract class and in modern javascript (typically interception with Proxy), it makes sense to have an abstract member decorated.

Please just remove in the compiler that error : A decorator can only decorate a method implementation, not an overload.ts(1249) on abstract methods.

ppoulard commented 2 years ago

@RyanCavanaugh Hi, as shown in my previous comment, it seems that decorating an abstract member function of an abstract class should be allowed ? What kind of feedback is still missing ?

RyanCavanaugh commented 2 years ago

@ppoulard we don't really want to change decorator semantics right now (especially in a way that allows previously-unallowed things) since there are proposals working through the TC39 committee to standardize them

shanwker1223 commented 2 years ago

Really would like to see the error removed. Using decorators right now to create a schema using abstract classes with dummy code.

thynson commented 1 year ago

Would this feature be available in the ECMA decorator? @RyanCavanaugh

dxqwww commented 4 months ago

Looks like now it's impossible to use it on abstract class members, then current implementation would look like this:

@ClassDecorator
abstract class DummyAbstractClass {
    protected fooValue!: number;

    protected abstract get _foo(): number; // property
    protected abstract set _foo(value: number); // property
    protected abstract _bar(): void; // method

    @memberDecorator
    public get foo(): number {
        return this._foo;
    }

    // TS error: Decorators cannot be applied to multiple get/set accessors of the same name.
    // @memberDecorator is redundant here it applied to both of setter/getter
    public set foo(value: number) {
        this._foo = value;
    }

    @memberDecorator
    public bar(): void {
        return this._bar();
    }
}

class DummyClass extends DummyAbstractClass {
    protected get _foo(): number {
        return this.fooValue;
    }

    protected set _foo(value: number) {
        this.fooValue = value;
    }

    protected _bar(): void {
        console.log('_bar called');
    }
}

Code is available on TS Playground

feature waiting room >w<

R2CD commented 3 months ago

This will be very awesome to have it!

But so far the only way (not best one, as it ignores errors to the whole code block) is having // @ts-expect-error or // @ts-ignore:

abstract class Some {
  // @ts-expect-error
  @Decorator("value")  // no errors for whole block
  abstract method(param: string): void;
}

Those way would be near perfect temporary solution if would be possible to specify certain error code to skip at // @ts-expect-error or // @ts-ignore. Issues: https://github.com/microsoft/TypeScript/issues/38409 , https://github.com/microsoft/TypeScript/issues/19139 etc. May be something like // @ts-expect-error("ts(1249)")

CAVAh commented 3 weeks ago

+1

NicolasThierion commented 2 weeks ago

Here is the "not so dirty" solution I use to call decorators on methods that should be abstract:

export const abstract = <T extends any>(): T => {
    throw new Error(
      'abstract() placeholder should be superseded by a decorator',
  }
};

interface User {
  // ...
}
abstract class SomeClass {

   @Decorator()
   method() {
        return abstract<User>()
   }
}

This is what I implemented in my library AspectJS, which I want to use exactly to design a retrofit-like library for JS.