microsoft / TypeScript

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

Suggestion: ClassMethodDecoratorContext's name property could pick up Value["name"] #54850

Open ajvincent opened 1 year ago

ajvincent commented 1 year ago

lib Update Request

Configuration Check

My compilation target is ESNext and my lib is the default.

Missing / Incorrect Definition

interface ClassMethodDecoratorContext<
    This = unknown,
    Value extends (this: This, ...args: any) => any = (this: This, ...args: any) => any,
> {
    // ...
    /** The name of the decorated class element. */
    readonly name: string | symbol;

    // ...

We could change the name field to: readonly name: Value extends { name: string } ? Value["name"] : (string | symbol);. Most of the time, Value["name"] is just string, so this would be a non-breaking change (if slightly less intuitive).

However, if I were to augment my method type definitions to have a name property, TypeScript could pick up on it:

Sample Code

export type NumberStringType = {
    repeatForward(s: string, n: number): string;
    repeatBack(n: number, s: string): string;
};

type NST_RF_raw = NumberStringType["repeatForward"]["name"]
//   ^? type NST_RF_raw = string

type MethodsOnlyType = {
  /* eslint-disable-next-line @typescript-eslint/no-explicit-any */
  [key: string | number | symbol]: (...args: any[]) => any
}

type NamedMethodsOnlyType<T extends MethodsOnlyType> = {
  /* eslint-disable-next-line @typescript-eslint/no-explicit-any */
  [Key in keyof T]: T[Key] & { name: Key }
}

type NST_RF_named = NamedMethodsOnlyType<NumberStringType>["repeatForward"]["name"];
//   ^? type NST_RF_named = "repeatForward"

type DefaultInferredName = ClassMethodDecoratorContext<
//   ^? type DefaultInferredName = string | symbol
  NumberStringType, NamedMethodsOnlyType<NumberStringType>["repeatForward"]
>["name"];

interface ClassMethodDecoratorContext_Named<
  This = unknown,
  Value extends (this: This, ...args: any) => any = (this: This, ...args: any) => any,
> {
  readonly name: Value extends { name: string } ? Value["name"] : (string | symbol);
}

type DirectName = ClassMethodDecoratorContext_Named<
//   ^? type Directname = "repeatForward"
  NumberStringType, NamedMethodsOnlyType<NumberStringType>["repeatForward"]
>["name"];

type InferredName = ClassMethodDecoratorContext_Named<
//   ^? type InferredName = string
  NumberStringType, NumberStringType["repeatForward"]
>["name"];

Obligatory Playground link

Motivation

I've been experimenting with method decorators for about a week now, to implement some aspect-oriented programming (or at least, my understanding of it). One facet I haven't yet cracked is removing the need for the method name on a decorator:

  class NST_Class extends NumberStringClass {
    @precondition<"repeatForward">(callForwardPre)
    repeatForward(s: string, n: number): string {
      return super.repeatForward(s, n);
    }
  }

Since the context has a name field of string | symbol type, I can't infer the name from that... I think.

I am perfectly willing to be wrong and have this whole ticket closed as invalid!

Documentation Link

The ECMAScript 2023 specification for Function.name says I'm possibly wrong here: it specifies the name field must be a string (specifically, "not a symbol"). This could be just a specification bug with the introduction of symbol keys, but I am not ready to assert that at this time.

RyanCavanaugh commented 1 year ago

If I'm understanding your proposal correctly, this isn't something we could reasonably do, since it would break very reasonable code with no apparent workaround:

class Foo {
    sample(s: string) { }

    doSomething(func: Foo['sample']) { }
}

function myFunc(s: string) { }

// Illegal, property 'name' of myFunc is 'myFunc', not assignable to 'sample' in target type
(new Foo()).doSomething(myFunc);
f3oall commented 1 year ago

@RyanCavanaugh, if I understood the initial suggestion right, I found myself limited by a pretty same issue with ClassFieldDecoratorContext.

Current Interface

interface ClassFieldDecoratorContext<
    This = unknown,
    Value = unknown,
> {
   // ...
    readonly name: string | symbol;
 }

Suggested Interface

interface ClassFieldDecoratorContext<
    This = unknown
    Value = unknown
    Name extends keyof This = keyof This
> {
  // ...
  readonly name: Name
}

Motivation

I found myself in need of better type for 'name' field, while I was trying to implement the following code (it won't compile currently):

export class Model<JSON extends Record<string, any>> {
  data: JSON
  constructor(data: JSON) {
    this.data = data
  }
}
interface UserJSON {
  created: string
}

class User extends Model<UserJSON> {
  @Field((json) => new Date(json.created)) created!: Date // currently, doesn't work
}

export function Field<This extends Model<any>, Value, Name extends keyof This & keyof This['data']>(
  reviver: This[Name] extends This['data'][Name] ? (json: This['data']) => This[Name] :  undefined
) {
  return function (
    target: undefined,
    context: ClassFieldDecoratorContext<This, Value, Name> // currently it has 2 generics, so error will be shown here
  ) {
      return function (this: This) {
        return reviver ? reviver(this.data) : this.data[context.name]
      }
  }
}

Let me know about your thoughts, it doesn't seem like a breaking change to me, but I surely can miss a lot of things. Thanks!

PS. Maybe I do not understand something in your example, but the parameter will be inferred to (s: string) => void anyway, so what does it have to do with fn name or especially DecoratorInterface?