1

Why doesn't this work? ...and what is the right way to do it?

class A {
    constructor() {
        console.log('constructin A')
    }
  public someMethod = (x: string) => {
    console.log(x)
    }
}

class B {
    private myA: A
    constructor(a: A) {
        console.log('constructin B')
        this.myA = a
    }
    // Fails with "Uncaught TypeError: Cannot read property 'someMethod' of undefined"
    anotherMethod = this.myA.someMethod;
}

const a = new A()
const b = new B(a)

b.anotherMethod('hello')

typescript playground link

Notes:

  • It is not typescript specific, I get the same error in equivalent plain js.
  • The reason I want to use this syntax anotherMethod = this.myA.someMethod, instead of say anotherMethod = (x: string) => { this.myA.someMethod(x) }, is that in my real world use case someMethod has complicated type annotations that I rather not want to duplicate on anotherMethod

EDIT (19.06.2020) - 'with decoupling'

The proposed answers and several comments, e.g. this, explains why my syntax doesn't work. As to the solutions:

Decoupling A and B: I didn't state that in my original question / example, but the reason I want B to use a method in A, is that I would like to decouple the two. In my real world use case I have several A's all conforming to the same interface of having a someMethod, but with different implementations of it. I want B to be agnostic of the implementation details, and just use whatever someMethod the specific A provides. The accepted answer picked up on that, and does an excellent job of fleshing it out in example code. It very well explains drawbacks of different solutions and provides (imho) the best, 'gotcha-free' syntax for my use case. If you are in a hurry, check out the accepted answer for this line:

public anotherMethod(...args: Parameters<A['someMethod']>): ReturnType<A['someMethod']> {
    return this.myA.someMethod(...args);
}

If you don't need decoupling this answer might be all you need.

andy-bc
  • 147
  • 1
  • 9
  • 1
    Do you want to add the method every time you construct a `B`? It seems better to just add it to the prototype once. – VLAZ Jun 18 '20 at 12:47
  • @VLAZ. It is not in my example but the `A` might be different classes, conforming to the same interface of having a `someMethod`, but the implementation of `someMethod` might differ across the `A` type classes. – andy-bc Jun 18 '20 at 12:54
  • I don't understand why do you even want to do this way? – Sagar Kulkarni Jun 18 '20 at 12:54
  • 1
    @Siraj: yes, no typescript warnings ..but check the console. – andy-bc Jun 18 '20 at 12:59
  • What happens inside `someMethod`? Does it use the instance of `A` (has `this` inside the function)? Or is it just a static method? Why do you want `anotherMethod` to point to that function when you access it with `b.myA.anotherMethod` – adiga Jun 18 '20 at 12:59
  • @adiga: `someMethod` makes a call to a remote api. Yes, `someMethod` relies on `this` because `A` in my real world use case is instantiated with some config parameters, like the url, etc. I wanted separation of concerns, so that the implementation details of calling the api is delegated to the `A` classes, and `B`is just composed with a specific `A`. – andy-bc Jun 18 '20 at 13:08

2 Answers2

3

anotherMethod will be defined before myA is set in the constructor. A possible solution to your problem would be to use a getter:

get anotherMethod() {
    return this.myA.someMethod;
}

Playground

To add to this, you can make the method in A static. This way you would not have to pass an instance of A to B at all. Just make sure B has access to A.

class A {
    constructor() {
        console.log('constructin A')
    }

    static someMethod = (x: string) => {
        console.log(x)
    }
}

class B {
    private myA: A
    constructor(a: A) {
        console.log('constructin B')
        this.myA = a
    }

    anotherMethod = A.someMethod;
}

const a = new A()
const b = new B(a)

b.anotherMethod("Hello")

Playground

Tim VN
  • 1,183
  • 1
  • 7
  • 19
  • 3
    It's important to note why it doesn't work as expected, when typescript is transpiled back into javascript, any variable assignments outside of the constructor are moved back into the top of the constructor. What this means is that `anotherMethod = this.myA.someMethod;` is assigned before `myA` has any value (is undefined). This results in the following: `undefined.someMethod` and hence the error. – Dane Brouwer Jun 18 '20 at 12:56
  • @DaneBrouwer: Thanks! @Tim VN: Perfect! the `get` syntax works for my use case. I also get type inference from `someMethod` in `anotherMethod`, so I don't have to specify the types twice. – andy-bc Jun 18 '20 at 13:24
  • 1
    Turning the method static is not a very good solution. Now `B` is coupled to the *implementation*, not the interface. It means that you cannot swap to anything else without also changing the code for `B` and it gets messy. See the final code block in my answer - what happens if `B` needs to be passed a `Y` instead of an `X`? You need to modify `B` for that. And if you need to support `Z` that also needs another modification. Another problem is that `someMethod` might need some instance data from `myA` and a static method cannot access or use that. – VLAZ Jun 18 '20 at 13:33
  • @Tim @VLAZ: what about the Tim's first solution with `get`? Does it decouple `B` from `A` implementations? I realize that 'decoupling' wasn't part of my original question, thanks for taking that into concern! – andy-bc Jun 18 '20 at 13:52
  • 1
    @andy-bc yes, the `get` works...in some cases. It might lead to [a lost `this` context](https://stackoverflow.com/questions/20279484/how-to-access-the-correct-this-inside-a-callback/) however. It's OK if the methods are bound to instances or don't use instance data. However, if they are regular prototype methods - `return this.myA.someMethod;` will return a function reference that is disassociated from `this.myA` and when called will use the `B` instance for `this`. [Here is an example](https://repl.it/repls/DefiantInfiniteEngine#index.ts) – VLAZ Jun 18 '20 at 14:10
  • @VLAZ Thanks! again spot on. I ran into precisely that in my specific use case. My workaround was to explicitly `bind` inside `get anotherMethod()` , e.g. `return this.myA.someMethod.bind(this.myA)`. So, I'm torn about which answer to accept as 'the answer'. The `get` solution above, more simple, but with the caveat of 'lost this context' or the [solution you provided](https://stackoverflow.com/a/62451091/1269280) using a a more explicit wrapper, e.g. `public anotherMethod(...args: Parameters) ...` – andy-bc Jun 18 '20 at 15:02
  • @andy-bc feel free to wait - maybe somebody comes in with an even better solution. I try to give questions about 24 hours or so before accepting an answer. To give a chance for more people to attempt to answer. – VLAZ Jun 18 '20 at 15:04
  • @andy-bc I added a rather long section to my answer for `get`. Might be of interest to read as it discusses potential problems (some of which I already covered in a comment). I think it's of value to understand what is happening and the potential problems. It's important to stress that the issues are not likely to always show up. But knowing about them at least lets you take better decision. And hopefully it's at least showing off something you might not have known. – VLAZ Jun 18 '20 at 15:49
  • Honestly, @VLAZ has gone into much more depth, the answer is much more concise. I'd personally go with his answer, also for potential future people having the same issue. – Tim VN Jun 19 '20 at 07:40
  • 1
    @VLAZ and Tim.. I have accepted VLAZ answer and edited the question to clarify my additional 'decoupling' intentions, for which VLAZ answer provides the best, gotcha-free, solution. Thanks to both of you! – andy-bc Jun 19 '20 at 10:03
2

The reason you experience an issue:

The problem is that anotherMethod = this.myA.someMethod; is executed before this.myA = a;. Here is the JavaScript code that the TypeScript compiler produces:

class B {
    constructor(a) {
        this.anotherMethod = this.myA.someMethod;
        console.log('constructin B');
        this.myA = a;
    }
}

So, there is no way for this not to produce an error.

If you need to attach the method at construction time, you need to do it yourself in the constructor:

class A {
    constructor() {
        console.log('constructin A')
    }
  public someMethod = (x: string) => {
    console.log(x)
    }
}

class B {
    private myA: A

    //declare the anotherMethod interface to match the  someMethod interface
    public anotherMethod: typeof A.prototype.someMethod

    constructor(a: A) {
        console.log('constructin B')
        this.myA = a

        //assign the method. Note: `this.anotherMethod = a.someMethod` is equivalent
        this.anotherMethod = this.myA.someMethod;
    }
}

const a = new A()
const b = new B(a)

b.anotherMethod('hello') //OK
b.anotherMethod(42)      //error - does not accept numbers

Playground Link

And here is how this can look in pure JavaScript

class A {
  constructor() {
    console.log('constructin A')
  }
  someMethod = (x) => {
    console.log(x)
  }
}

class B {
  constructor(a) {
    console.log('constructin B')
    this.myA = a;
    this.anotherMethod = this.myA.someMethod;
  }
}

const a = new A()
const b = new B(a)

b.anotherMethod('hello')

A better solution

In comments the question asker said:

I wanted separation of concerns, so that the implementation details of calling the api is delegated to the A classes, and B is just composed with a specific A.

For such a use case, I would suggest not directly attaching the methods but instead just declare a single method in B that delegates all calls to myA:

//method with the same signature as A.someMethod
public anotherMethod(...args: Parameters<A['someMethod']>): ReturnType<A['someMethod']> {
    //use the delegate for the call
    return this.myA.someMethod(...args);
}

This is much cleaner than assigning the methods to instances at construction time. The ...args: Parameters<A['someMethod']> will always be equal to the parameters that someMethod takes. Conversely, ReturnType<A['someMethod']> resolves to the type that someMethod returns. If that is void, then it's still valid.

Here is how this can look - I've turned A into an interface that is implemented by different classes. B now only knows about the interface and doesn't care how it's implemented:

interface A {
    someMethod(x: string): void;
}

class X implements A {
    constructor() {
        console.log('constructin X')
    }
  public someMethod = (str: string) => {
      console.log("X", str);
    }
}

class Y implements A {
    constructor() {
        console.log('constructin Y')
    }
  public someMethod = (str: string) => {
      console.log("Y", str);
    }
}
class Z implements A {
    constructor() {
        console.log('constructin Z')
    }
  public someMethod = (str: string) => {
      console.log("Z", str);
    }
}

class B {
    private myA: A

    constructor(a: A) {
        console.log('constructin B')
        this.myA = a
    }

    public anotherMethod(...args: Parameters<A['someMethod']>): ReturnType<A['someMethod']> {
        return this.myA.someMethod(...args);
    }
}

const x = new X()
const y = new Y()
const z = new Z()
const b1 = new B(x)
const b2 = new B(y)
const b3 = new B(z)

b1.anotherMethod('hello') //OK
b2.anotherMethod('hello') //OK
b3.anotherMethod('hello') //OK

b1.anotherMethod(42)      //error - does not accept numbers

Playground Link

You can stop reading here.


(Optional reading to the end) Why I suggest this solution

I would like to address why using a method for delegation works better as opposed to using in B. This is going in depth with potential pitfalls.

get anotherMethod() {
    return this.myA.someMethod;
}

In the above case the getter will always return a function reference. This allows for calling

b.anotherMethod("hello") and this will compile correctly, it will have the correct method signature, and can be called. However, there is a subtle yet very widely encountered problem the this context will be lost. Recommended reading:

How does the "this" keyword work?

Here is the short version - the value for this is determined at the time of calling a function. To keep it simple, here is a rule of thumb: the value is whatever is before the final dot when calling:

 //calling quux()
 foo.bar.baz.quux() // -> this = foo.bar.baz
 ^^^^^^^^^^^                     ^^^^^^^^^^^
      |                               |
      ---------------------------------
 //calling baz()
 foo.bar.baz() // -> this = foo.bar
 ^^^^^^^                    ^^^^^^^
    |                          |
    ----------------------------
 //calling bar()
 foo.bar() // -> this = foo
 ^^^                    ^^^
  |                      |
  ------------------------
 //calling foo()
 foo() // -> this = undefined
^
|
nothing

For more information, I suggest reading the links I have provided.

This is important because the call is what determines this. A very common problem in JavaScript is losing the context by doing the following:

"use strict";

const foo = {
  value: 42,
  bar() {
    return this.value;
  }
}

console.log(foo.bar()); //this = foo; foo.value = 42

//get a function reference
const func = foo.bar;

console.log(func()); // this = undefined; undefined.value = error

This is the very basic form of the error. It can manifest in many different ways, such as when passing callbacks:

functionThatTakesCallback(foo.bar);

and it's going to manifest when the get is used. It's actually more insidious, since you would call the function but with a new context:

class B {
    private myA: A
    /* simplified for brevity */
    get anotherMethod() {
        return this.myA.someMethod;
    }
}

const b = new B(a);
b.anotherMethod("hello"); // this = b;

Now there is no way to ensure that anotherMerhod (which is an alias for someMethod) will work correctly.

  1. If someMethod doesn't use any instance data (no this inside it), then it will work. But there is no way to verify that.

  2. If someMethod is a regular method (or function - the difference is negligible here) and uses this then calling b.anotherMethod("hello") is almost guaranteed to produce the wrong result or even an error.

  3. If someMethod is an "arrow method" (will go in details below) then the this context will be automatically bound and thus the result is correct. Again, there is no convenient way to verify that.

What is an "arrow method"? It's this construct:

class X implements A {
    private myProp: string;
    /* simplified for brevity */
    public someMethod = (str: string) => { //"arrow method"
        console.log(this.myProp, str);
    }
}

In reality, this is an an arrow function assigned as a class property. The difference is that it's automatically added to the instance and this would be lexically bound inside, thus it is always going to point to the current instance.

By contrast, a regular method looks like this:

class Y implements A {
    private myProp: string;
    /* simplified for brevity */
    public someMethod(str: string) { //regular method
        console.log(this.myProp, str);
    }
}

It is shared between all instances as it exists on the prototype. This ensures that there is only one copy of this function in memory, the arrow function class property creates one for each instance of X created. This doesn't need to be a problem, but if there are going to be a lot of X objects created, it leads to increased memory usage. The drawback of Y is the potential loss of this. So, it's a bit of a balancing act.

Here is an example of how things can go wrong:

interface A {
    someMethod(x: string): void;
}

class X implements A {
    //instance property
    private myProp: string;

    constructor(name: string) {
        console.log('constructin X')

        this.myProp = name;
    }
    public someMethod = (str: string) => { //"arrow method"
        console.log(this.myProp, str);
    }
}

class Y implements A {
    //instance property
    private myProp: string;

    constructor(name: string) {
        console.log('constructin Y')

        this.myProp = name;
    }
    public someMethod(str: string) { //regular method
        console.log(this.myProp, str);
    }
}
class B {
    private myA: A

    constructor(a: A) {
        console.log('constructin B')
        this.myA = a
    }

    get anotherMethod() {
        return this.myA.someMethod;
    }
}

const x = new X("Foo")
const y = new Y("Bar")
const b1 = new B(x)
const b2 = new B(y)

b1.anotherMethod('hello'); //Foo hello
b2.anotherMethod('hello'); //undefined hello

//this.myProp is taken from B. Illustration:
(b2 as any).myProp = "This is B"
b2.anotherMethod('hello')  //This is B hello

Playground Link

With all this said, there is a way to patch the get approach. The value of this can be permanently set with Function#bind

class B {
    private myA: A
    /* simplified for brevity */
    get anotherMethod() {
        return this.myA.someMethod.bind(this.myA);
    }
}

The problem now is that .bind() returns a new function every time. So

b.anotherMethod("hello");
b.anotherMethod("hello");

will have the correct value for this but this actually creates two functions, executes them, then discards them. In this case memory isn't a problem, as the functions will simply be garbage collected, but garbage collection itself might be an issue. A code that calls the getter many times, for example like this:

for(let i = 0; i < 9000; i++) {
  b.anotherMethod("hello");
}

will lead to many runs of the garbage collector which can hamper performance. Again, not necessarily a problem but something that needs to be mentioned.

Another potential issue is if you ever need to compare functions for equality

b.anotherMethod === b.anotherMethod //false

Playground Link

Every b.anotherMethod produces a different function. It is rare that you would need to compare functions but it might come up if you use a Set or a Map, for example:

const set = new Set();
set.add(b.anotherMethod);

//later

set.has(b.anotherMethod); //false

I will just mention an even more complex way of making sure the get works is keeping

get anotherMethod() {
    return this.myA.someMethod;
}

and then using a Proxy to wrap X instances and intercept any invocations of anotherMethod and instead execute is using Function.call or Function#appy in order to preserve the context.

However, in my view these are all solutions that have issues of their own. Especially the Proxy approach is very much overengineering. This is why I prefer a simple method that delegates the call to this.myA:

public anotherMethod(...args: Parameters<A['someMethod']>): ReturnType<A['someMethod']> {
    return this.myA.someMethod(...args);
}
  • it's easy to read - you know exactly what's being done.
  • easy to understand - the intention is clear
  • there are no hidden drawbacks:
    • this.myA.someMethod(...args) will always use this.myA as the value of this for someMethod regardless of how that method is defined.
    • it's a regular method thus there is only one copy of it in memory.
VLAZ
  • 26,331
  • 9
  • 49
  • 67