10

Closed

Fix "wrong this" issue inside a class

description

Please consider the following enhancement request. When "this" keyword is used to point to a class member method, preserve the "this" object. Take for example this simple class:
class Class1 {
    constructor () {
        document.onmousemove = this.handler;
    }
    handler(e: MouseEvent) {
        //wrong "this" here!
    }
}
It would be great if TypeScript compiler handled such cases and generated an additional call to compiler-injected function that would preserve the context. In other languages like C# this works out of the box.
Closed Feb 26, 2013 at 9:37 PM by LukeH
This issue is the same as http://typescript.codeplex.com/workitem/468, so closing this and moving further discussion to that thread.

comments

nabog wrote Nov 22, 2012 at 1:21 PM

Just a simple extra step: this.handler.bind(this);.

I'm not really sure whether binding every single method by default is a good idea.

See also: http://typescript.codeplex.com/workitem/468

AnatolyBakirov wrote Nov 22, 2012 at 1:29 PM

I realize that there are at least two analogous suggestions posted here. So, just to be clear, I’m not suggesting to actually use “bind” to resolve this issue, instead I suggest to emit the following JavaScript for this example:

var Class1 = (function () {
function Class1() {
    document.onmousemove = __bind(this, this.handler);
}
Class1.prototype.handler = function (e) {
};
return Class1;
})();

Where __bind function would be injected by compiler similar to how you inject "__extends" function:

function __bind(thisObj, method) {
method.apply(thisObj, arguments);
}

This is what many JavaScript developers do to work around this issue.

AnatolyBakirov wrote Nov 22, 2012 at 1:33 PM

Oops... sorry, I meant like this:

function __bind(thisObj, method) {
return function () { method.apply(thisObj, arguments); }
}

AnatolyBakirov wrote Nov 22, 2012 at 1:46 PM

nadog,

Will your suggestion about bind even work? Member methods are added to prototypes and therefor shared by all instances of this class. So, if you place this "simple extra step" to constructor, you bind it to last-created instance... which is incorrect.

smithkl42 wrote Nov 22, 2012 at 3:12 PM

I don't know the best way to fix this, but I agree that it does need to be fixed. The current semantics - a weird mix of JavaScript and C# - are confusing. Unlike JS, you can normally be assured that "this" refers to the current class, not to the owner of the method - but not quite always, because if you assign the method to an event, suddenly the "this" changes. There are lots of ways to work around it, but the current way of handling it seems very inconsistent. And given that "this" is one of the most confusing parts about JS to begin with, and one of the easiest to get wrong, I'd recommend moving entirely over to C#-style semantics, where "this" inside a class method just means "this instance of this class", period, end of story, you don't have to worry about it.

clausreinke wrote Nov 22, 2012 at 3:35 PM

Free-standing method selections (obj.method) are always a danger point in JS - one needs to be sure that the method doesn't use 'this'.

A fairly readable ES6 workaround (and shorthand for the apply answer) is to expand the parameters, using spreads ( (...args)=>obj.methods(...args) ). Spreads don't seem to be fully implemented yet, but the single-parameter case does not need them:

document.onmousemove = e => this.handler(e);

I'm not so sure that registering external event handlers in a constructor is a good idea, though.

nabog wrote Nov 22, 2012 at 6:34 PM

@AnatolyBakirov, yes of course "this.handler.bind(this);" would work, and so would @clausreinke suggestion of "document.onmousemove = e => this.handler(e);".

But since you are doing it in the constructor it is rather wasteful. The correct approach would be:

var class1 = new Class1();
document.onmousemove = class1.handler.bind(class1); // you will get the correct "this" context every time

Ideally you wouldn't want to do this - and for JavaScript to use the correct "this" context, but as I said it is not too much extra work.

AnatolyBakirov wrote Nov 23, 2012 at 3:33 AM

Thanks for these workarounds, they all would work. But won't you agree that since TypeScript introduces the concept of "class" it should also resolve the problem of "this" when it is used inside of a class?

dlshryock wrote Nov 29, 2012 at 5:06 PM

Even though typescript has to live in a javascript world, and try to play nicely, I think that there is still an elegant solution to this problem.

Given this class:
class Person
{
  constructor(public name:string){}
  shout(message){ alert(this.name + " says: " + message + "!"); }
}
I think that the compiler could emit an error when trying to reference a class method:
var joe = new Person("Joe");

//compiler warning here. 
//  Warning, referencing a class method without binding can cause issues
//  when invoking a method as a function, try using 'joe::shout' instead of 'joe.shout'."
var shout = joe.shout;
shout("Hello World");

The warning could be avoided by casting the instance to "any":
var joe:any = new Person("Joe")
var shout = joe.shout; //no warning
shout("Hello World");
And I also think that some new syntax to facilitate method binding would provide a simple solution (the actual example syntax here is unimportant):
var joe = new Person("Joe");
//no warning, the code is transformed to joe.shout.bind(joe)
var shout = joe::shout; 
shout("Hello World");

LukeH wrote Dec 5, 2012 at 1:28 AM

I understand the desire to see TypeScript offer further help here. However, we consider it very important to maintain JavaScript runtime semantics and to allow interoperation with existing JavaScript coding/usage patterns. ES6 classes will not do any bind-on-extract, and so TypeScript cannot do this implicitly either.

We may be able to add warnings/errors on attempts to extract a method off a class instance. This would at least bring attention to cases where this bug might occur.

I'm also supportive of the "bind operator" proposal from Dave Herman, though I think it could be modified to offer even simpler syntax for bind that fits the most common use case of method extraction:

http://wiki.ecmascript.org/doku.php?id=strawman:bind_operator

I expect we will look at both the warning and the bind operator as options for TypeScript.

Damiano wrote Jan 6, 2013 at 2:55 PM

When needed I use this way:
class Class1 {
    private message: string;

    constructor() {
        this.message = "Hello World! ";
        document.onmousedown = (ev) => this.handler.call(this, ev);
    }
    handler(e: MouseEvent) {
        // right "this" here
        alert(this.message + e.button);
    }
}