microsoft / TypeScript

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

Allow mapped type to declare functions #48125

Open rockwalrus opened 2 years ago

rockwalrus commented 2 years ago

Suggestion

🔍 Search Terms

✅ Viability Checklist

My suggestion meets these guidelines:

⭐ Suggestion

📃 Motivating Example

In our universe, there are two kinds of animals, dogs and cats.

type Animal = 'dog' | 'cat';

Some people can handle dogs. Some people can handle cats. Some people can even handle both. But if you can't handle dogs or cats, you're just not a handler. We can express this generically in TypeScript.

type Handler<A extends Animal> = {[animal in A as `handle${Capitalize<animal>}`]: (animal: animal) => void};

Let's say you have a dog handler that doesn't do anything interesting.

class BoringDogHandler implements Handler<'dog'> {
    handleDog(dog : `dog`) {}
}

You could also have someone who can handle both and keeps a record of how many animals handled.

class CountingUniversalAnimalHandler implements Handler<Animal> {
    count = 0;

    handleDog() {this.count++;}
    handleCat() {this.count++;}
}

Some dog handlers are well known for broadcasting to the world whenever they are handling a dog. Since this is such a common thing, we'd like to create a mixin for it.

function NoisyDogHandler<H extends {new (...args: any[]): Handler<'dog'>}>(base: H) {
    return class extends base {
        handleDog(dog : `dog`) {
            console.log("Handling dog");
            return super.handleDog(dog);
        }
    };
}

Looks innocuous enough. But this doesn't work! Typescript informs us that

Class Handler<"dog"> defines instance member property handleDog, but extended class (Anonymous class) defines it as instance member function. (2425)

If we were allowed to declare that the handle${Capitalize<A>} members were not mere properties, but actually methods, TypeScript would not fear letting us override them. The natural syntax would look like this, which is currently not allowed:

type Handler<A extends Animal> = {[animal in A as `handle${Capitalize<animal>}`](animal: animal): void};

A subtle distinction for a subtle distinction.

Playground Example

💻 Use Cases

What do you want to use this for?

Mix-ins are powerful, and when they work they're fantastic, but they're currently somewhat brittle to work with.

This is one of the pieces of the puzzle needed for classes extending constructors of mapped types to override methods (#27689). The other piece needed is to be able to keep the member function status of methods obtained from type indexing (#38496, c.f. #35416, and #46802).

What shortcomings exist with current approaches?

For small enough use cases, you can do the mapping "by hand":

type Animal = 'dog' | 'cat';

type DogHandler = {handleDog(dog: 'dog'): void};
type CatHandler = {handleCat(cat: 'cat'): void};
type BothHandler = DogHandler & CatHandler;

type Handler = DogHandler | CatHandler;
type HandlerFor<A extends Animal> = ('dog' extends A ? DogHandler : {}) & ('cat' extends A ? CatHandler : {})

Obviously this doesn't scale well if you create multiple methods with Animal instead of just one, or Animal has many more cases. Less obviously, there isn't a really satisfactory way to implement a generic method that takes an animal and handler, calls the right handle method, ensures at compile time that the handler can handle the animal, and doesn't involve casting. See this playground for several attempts.

RyanCavanaugh commented 2 years ago

Based on the use case I'm inclined to just discard the ... defines instance member property ..., but extended class ... defines it as instance member function. error when the base class property is created through a mapped type. Thoughts?

fatcerberus commented 2 years ago

Keep in mind that, assuming this proposed syntax does create methods, TS would likely treat them bivariantly, meaning you could potentially unsoundly assign a CatHandler to a DogAndCatHandler (I bring this up only because the expected subtyping relationship for handlers is explicitly mentioned in the OP).

See https://github.com/microsoft/TypeScript/wiki/FAQ#why-are-function-parameters-bivariant and note that strictFunctionTypes enables contravariant parameters for function types, but not methods.

rockwalrus commented 2 years ago

Based on the use case I'm inclined to just discard the ... defines instance member property ..., but extended class ... defines it as instance member function. error when the base class property is created through a mapped type. Thoughts?

That sounds reasonable to me.

rockwalrus commented 2 years ago

Oh, but if you do discard the error message, make sure it discards it for subclasses as well.

frank-weindel commented 2 years ago

Dealing with this as well. Removing the error for this situation sounds good. I'd go as far to say that anytime an instance member property can be either a single method signature (or undefined) then it should be overridable as if it was a method.

eliasm307 commented 2 years ago

Yes would be good if this wasnt an error.

I'm creating a util to infer class types from some legacy code I'm working with that creates custom ES6 compatible classes.

The util uses mapped types and this error means a lot of ts-ignores need to be used when overriding methods in ES6 classes from these inferred classes in TS

derekdon commented 1 year ago

Have the same problem.

adrien2p commented 7 months ago

I have the same problem as well 👍

adrien2p commented 7 months ago

Here is my case

export type AbstractModuleService<
  TContainer,
  TMainModelDTO,
  TOtherModelNamesAndAssociatedDTO extends OtherModelsConfigTemplate
> = AbstractModuleServiceBase<TContainer, TMainModelDTO> & {
  [K in keyof TOtherModelNamesAndAssociatedDTO as `retrieve${ExtractSingularName<
    TOtherModelNamesAndAssociatedDTO,
    K
  > &
    string}`]: (
    id: string,
    config?: FindConfig<any>,
    sharedContext?: Context
  ) => Promise<TOtherModelNamesAndAssociatedDTO[K & string]["dto"]>
} & {
  [K in keyof TOtherModelNamesAndAssociatedDTO as `list${ExtractPluralName<
    TOtherModelNamesAndAssociatedDTO,
    K
  > &
    string}`]: (
    filters?: any,
    config?: FindConfig<any>,
    sharedContext?: Context
  ) => Promise<TOtherModelNamesAndAssociatedDTO[K & string]["dto"][]>
} & {
  [K in keyof TOtherModelNamesAndAssociatedDTO as `listAndCount${ExtractPluralName<
    TOtherModelNamesAndAssociatedDTO,
    K
  > &
    string}`]: {
    (filters?: any, config?: FindConfig<any>, sharedContext?: Context): Promise<
      [TOtherModelNamesAndAssociatedDTO[K & string]["dto"][], number]
    >
  }
} & {
  [K in keyof TOtherModelNamesAndAssociatedDTO as `delete${ExtractPluralName<
    TOtherModelNamesAndAssociatedDTO,
    K
  > &
    string}`]: {
    (
      primaryKeyValues: string[] | object[],
      sharedContext?: Context
    ): Promise<void>
  }
} & {
  [K in keyof TOtherModelNamesAndAssociatedDTO as `softDelete${ExtractPluralName<
    TOtherModelNamesAndAssociatedDTO,
    K
  > &
    string}`]: {
    <TReturnableLinkableKeys extends string>(
      primaryKeyValues: string[] | object[],
      config?: SoftDeleteReturn<TReturnableLinkableKeys>,
      sharedContext?: Context
    ): Promise<Record<string, string[]> | void>
  }
} & {
  [K in keyof TOtherModelNamesAndAssociatedDTO as `restore${ExtractPluralName<
    TOtherModelNamesAndAssociatedDTO,
    K
  > &
    string}`]: {
    <TReturnableLinkableKeys extends string>(
      productIds: string[] | object[],
      config?: RestoreReturn<TReturnableLinkableKeys>,
      sharedContext?: Context
    ): Promise<Record<string, string[]> | void>
  }
}

Where I expect the mapped types to be functions of the class and not properties as it is only possible to define them as of now, the objective is to allow classic override of the method from the child class. But being properties (since there is a limitation), the only way I found around it is to override it like the following

  @InjectManager("baseRepository_")
  async listVariants_(
    filters: ProductTypes.FilterableProductVariantProps = {},
    config: FindConfig<ProductTypes.ProductVariantDTO> = {},
    @MedusaContext() sharedContext: Context = {}
  ): Promise<ProductTypes.ProductVariantDTO[]> {
    const variants = await this.productVariantService_.list(
      filters,
      config,
      sharedContext
    )

    return JSON.parse(JSON.stringify(variants))
  }

  get listVariants() {
    return this.listVariants_
  }

any news in terms of advancement on that topic?

BalaM314 commented 3 weeks ago

Same issue, would be nice to just remove that error.