Skip to content

Cannot override method in subclass when superclass instance type is a mapped type #27689

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

Open
rbuckton opened this issue Oct 10, 2018 · 6 comments
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript

Comments

@rbuckton
Copy link
Member

TypeScript Version: 3.1.1

Search Terms:
defines instance member property

Code

class A {
    foo(): void {};
    bar: number;
}

class B extends (A as { new(): Pick<A, Exclude<keyof A, "foo">> & Pick<A, "foo"> }) {
    // (1)
    foo(): void {
        super.foo();
    }
    baz: number;
}

Expected behavior:
Compiles successfully

Actual behavior:

Error at (1): [ts] Class 'Pick<A, "bar"> & Pick<A, "foo">' defines instance member property 'foo', but extended class 'B' defines it as instance member function.

Playground Link: http://www.typescriptlang.org/play/index.html#src=class%20A%20%7B%0D%0A%20%20%20%20foo()%3A%20void%20%7B%7D%3B%0D%0A%20%20%20%20bar%3A%20number%3B%0D%0A%7D%0D%0A%0D%0Aclass%20B%20extends%20(A%20as%20%7B%20new()%3A%20Pick%3CA%2C%20Exclude%3Ckeyof%20A%2C%20%22foo%22%3E%3E%20%26%20Pick%3CA%2C%20%22foo%22%3E%20%7D)%20%7B%0D%0A%20%20%20%20foo()%3A%20void%20%7B%0D%0A%20%20%20%20%20%20%20%20super.foo()%3B%0D%0A%20%20%20%20%7D%0D%0A%20%20%20%20baz%3A%20number%3B%0D%0A%7D

A more realistic example of this is when using https://github.com/bterlson/strict-event-emitter-types along with subclassing a superclass with overridable methods:

import StrictEventEmitter from "strict-event-emitter-types";
import * as inspector from "inspector";

interface SessionEvents {
  "Runtime.executionContextCreated": (message: inspector.InspectorNotification<Runtime.ExecutionContextCreatedEventDataType>) => void
}
type StrictSession = StrictEventEmitter<inspector.Session, SessionEvents>;
class CustomSession extends (inspector.Session as { new (): StrictSession }) {
  connect() { // error here
    super.connect();
  }
}
@weswigham weswigham added the Needs Investigation This issue needs a team member to investigate its status. label Oct 16, 2018
@weswigham
Copy link
Member

AFAIK, this is because non-homomorphic mapped types don't retain the methodiness origin of the key, hence the issue. We can probably fix this by either always allowing overrides of any properties from non-homomorphic mapped types or by trying to heuristically track the methodiness of a property symbol a bit more completely (similarly to how we track parameternameiness for tuple members). In any case, it should probably be possible to perform the override, since I think the conservative answer we give now gets a bit too much in the way when working with mapped types.

@weswigham weswigham added the Bug A bug in TypeScript label Oct 16, 2018
@DanielRosenwasser DanielRosenwasser added Suggestion An idea for TypeScript In Discussion Not yet reached consensus and removed Bug A bug in TypeScript Needs Investigation This issue needs a team member to investigate its status. labels Oct 25, 2018
@cherryblossom000
Copy link
Contributor

Mapped types keeping the methodiness would be useful for things like this:

type Constructor<T = {}> = new (...args: any[]) => T

class Mixin {
  mixinProperty?: number

  method1() {
    console.log('method1 from Mixin')
  }

  method2() {
    console.log('method2 from Mixin')
  }

  method3() {
    console.log('method3 from Mixin')
  }

  // Applies the Mixin class to a base class, optionally ignoring some methods
  static applyToClass<T extends Constructor, TIgnore extends keyof Mixin = never>(
    Class: T,
    ignore: TIgnore[] = []
  ): new (...args: ConstructorParameters<T>) => Omit<Mixin, TIgnore> & InstanceType<T> {
    class _Class extends Class {}
    Object.getOwnPropertyNames(Mixin.prototype).forEach(name => {
      if (!ignore.includes(name as any)) {
        Object.defineProperty(
          _Class.prototype,
          name,
          Object.getOwnPropertyDescriptor(Mixin.prototype, name)!
        )
      }
    })
    return _Class as any
  }
}

class A {
  constructor(public aProperty: string) { }

  a() {
    console.log('a from A')
  }
}

// B will have property, method1, and method2 but not method3
class B extends Mixin.applyToClass(A, ['method3']) {
  // Class 'Pick<Mixin, "mixinProperty" | "method1" | "method2"> & A' defines instance member property 'method2',
  // but extended class 'B' defines it as instance member function.
  method2() {
//~~~~~~~
    console.log('method2 from B')
  }
}

const b = new B('some string')

type BMixinProperty = B['mixinProperty'] // number | undefined
console.log(b.aProperty) // some string

b.a() // a from A
b.method1() // method1 from Mixin
b.method2() // method2 from B
console.log('method3' in b) // false

(playground)

I also asked about this on SO.

@jcalz
Copy link
Contributor

jcalz commented Oct 7, 2020

Is there a useful workaround for this? I guess it's "manually write out your mapped types with method syntax" which is unfortunate.

@ahoisl
Copy link

ahoisl commented Jan 11, 2021

The workaround that worked for me is to declare the "method" as a readonly property returning a function:

readonly method2 = () => {
    console.log('method2 from B')
}

@nin-jin
Copy link

nin-jin commented Jan 1, 2024

Minimal reproduction:

class A {
    foo() {}
}

class B extends (
    A as { new(): Pick< A, 'foo' > }
) {
    foo() { }
}

https://www.typescriptlang.org/play?#code/MYGwhgzhAECC0G8CwAoa7oDMD22AUAlIgL6qkqqiQwBC0ApgB4Au9AdgCYx6oZzSRE0NvQDuhAFzQACgEtgAawA8cADTQA5DmwboAPmjkiyNBm2Eh5YkA

Any ideas?

@nin-jin
Copy link

nin-jin commented Jan 2, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

7 participants