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

Ability to decorate abstract member function #37067

Open
5 tasks done
thynson opened this issue Feb 27, 2020 · 12 comments
Open
5 tasks done

Ability to decorate abstract member function #37067

thynson opened this issue Feb 27, 2020 · 12 comments
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript

Comments

@thynson
Copy link

thynson commented Feb 27, 2020

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:

  • This wouldn't be a breaking change in existing TypeScript/JavaScript code
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.
@RyanCavanaugh RyanCavanaugh added Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript labels Feb 27, 2020
@eun-ice
Copy link

eun-ice commented Jun 22, 2020

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

@Hookyns
Copy link

Hookyns commented Oct 22, 2020

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
Copy link

ppoulard commented Jan 13, 2022

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
Copy link

ppoulard commented Feb 4, 2022

@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
Copy link
Member

@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
Copy link

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

@thynson
Copy link
Author

thynson commented Jun 24, 2023

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

@dxqwww
Copy link

dxqwww commented Apr 22, 2024

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
Copy link

R2CD commented May 19, 2024

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: #38409
, #19139 etc. May be something like // @ts-expect-error("ts(1249)")

@CAVAh
Copy link

CAVAh commented Aug 19, 2024

+1

@NicolasThierion
Copy link

NicolasThierion commented Aug 29, 2024

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.

@augustobmoura
Copy link

augustobmoura commented Dec 12, 2024

Another way of doing this is by decorating class fields and typing them as functions:

class Foo {
  @implementSomething
  getUsers!: () => Promise<User[]>

  @implementSomething
  saveUsers!: (user: User) => void
}

const foo = new Foo()

// This gets typed correctly now
foo.getUsers()
foo.saveUser(null)

@RyanCavanaugh since Decorators are now in Stage 3 and changes to the proposal are not expected (unless absolutely necessary), could we revisit this idea?

My idea is: we should implement decorated abstract methods in the same way as class fields, somewhat of a syntax sugar. Meaning when calling the decorator, value will be undefined and the kind property inside the context object will be "field". The mechanism in the compiler already, and decoration of class fields is well specified in ECMAScript. For these

Things to consider with my idea:

  • Will future proposals from ECMA clash with abstract methods decorators?
    • I couldn't think anyway that new proposals for decorators or class fields could break this idea, new proposals will always be retro-compatible and we can always stick with the current spec for this specific case
  • Are there any other (better) ways of implementing abstract methods decorators?
    • Reusing the class fields spec seems clever, but sounds a bit unorthodox, methods aren't really fields, could TS have its own decorate mechanism for this case? Would other ideas benefit from TS having its own decorators superset?
  • Last, but not least: Is this idea really necessary?
    • Having class fields decorated is already supported and it kinda solves the problem already. Sure it is bit ugly compared to abstract methods, but it is future proof and already implemented. Maybe we don't really need to support decorators for abstract methods

And since we discussing this, because SWC doesn't do any type-checking whatsoever, it does output valid JavaScript for abstract methods, and it does work in some way, although not fully compliant with the latest spec.

Edit: My first version had some grammar errors, and a special function for the field declaration, I remebered the null-assertion field syntax can be used instead field!: string

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests