Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

`this` is bound incorrectly with `$root`/`$parent`/etc. bindings #378

Open
tjcrowder opened this Issue · 15 comments

8 participants

@tjcrowder

The long version is in this thread from the forum:
https://groups.google.com/d/topic/knockoutjs/PvZsjLj_Doc/discussion

The short version:
When a click binding or similar is used with a function called on $root, e.g. from the seat reservation tutorial:

<td><a href="#" data-bind="click: $root.removeSeat">Remove</a></td>

...when removeSeat is called, this should be $root rather than the seat. This also applies to $parent and $parents[n].

In the linked thread, RPN does a good job of explaining why this is the way it is, but this needs changing (even if it's a breaking change; it doesn't have to be, though, see below). Although click: $root.removeSeat is passing a function reference to KO, KO's responsible for calling it properly and has all the information necessary to do so. Properly in this case would be ensuring that this is set to $root.

There is a workaround using bind:

<td><a href="#" data-bind="click: $root.removeSeat.bind($root)">Remove</a></td>

...but that's very anti-KO: It's verbose and clumsy; KO is concise and elegant. :-)

I haven't delved under the covers of KO enough yet to know the scope of the change required; I assume the bindings must be processed via eval or new Function or similar. This may mean that some pre-processing on the data-bind string would be needed -- but then, we know there's already pre-processing on the string (to find and parse out the various bindings).

To my mind, it's necessary that KO handle the normal case, but not that it handle every possible arbitrarily-complicated case. So for instance, handle strings containing $root.removeSeat and $root['removeSeat'] and such, but there's no need to handle strings with things like $root['remove' + 'seat'] (unless, of course, it's easy).

It's possible to avoid having this be a breaking change (for a couple of versions while people update their code, or forever): Use new keywords for it, e.g. $$root and related. Old code using $root and such would be unaffected, newer code using $$root would see the new meaning of this.

The next step beyond that would be to handle the general case of object.method rather than just handling $$root and such. Doing that without making it a breaking change would require a flag on view models, option to applyBindings, or similar.

If there's impetus for this change from the project owner, I can start kicking around the internals of KO and come up with an implementation and a pull request.

@mbest
Collaborator

If there's impetus for this change from the project owner, I can start kicking around the internals of KO and come up with an implementation and a pull request.

If you're interested, I've already worked on adding this to Knockout. I don't know when or if it will be included in the official version, but you can use my fork right now and get what you want: Readme, Download

@tjcrowder

@mbest: Very, very cool, thanks!

Would be very keen to see this folded into the mainline code.

-- T.J. :-)

@rniemeyer
Collaborator

I am probably in the minority in feeling that this doesn't need to be changed. I do agree that it can be confusing , especially to people just starting to use the framework. We are planning to augment the documentation a bit to explain it more thoroughly.

Just to explain my thoughts:

I think the way that it works now is consistent with how JavaScript behaves. Just to simplify it a bit, pretend that the click binding ultimately looks like:

var click = function(action, data) {
    ....
}

and it is being called like:

click(myRootViewModel.removeSeat, currentDataItem);

When we are dealing with action in the handler, there is no longer any inherent this associated with the function. We don't know that it came from myRootViewModel.

if we simply do:

action(data);

Then, this will be the window object. Not very helpful.

Instead we try to be helpful and do:

action.call(data, data);

Now, if the function is on the actual item, you can naturally use this. If the function is not part of the data, then you are given the data as the first argument.

As far as avoiding this problem in code, there are certainly a number of ways to avoid the problem:

1- you can bind it in the markup, as you mentioned like:

<a href="#" data-bind="click: $root.removeSeat.bind($root)">Remove</a>

2- you can call it in an anonymous function, so we are actually executing it off of the $root object like:

<a href="#" data-bind="click: function() { $root.removeSeat($data); }">Remove</a>

However, this is ugly (certainly more ugly than .bind). We used to do it this way, before $data was passed as an arg.

3- you can bind it in your view model like:

If creating the view model in an object literal, the bind needs to happen after the viewModel is created. So, the function can be created outside of the object literal or just the bind like:

var viewModel = {

};

viewModel.removeSeat = function(seat) {
    this.seats.remove(seat);
}.bind(viewModel);

If creating the view model in a function, then this is even easier:

var ViewModel = function() {
    this.removeSeat = function(seat) {
         this.seats.remove(seat);
    }.bind(this);
};

or on the prototype:

var ViewModel = function() {
    this.removeSeat = this.removeSeat.bind(this);
};

ViewModel.prototype.removeSeat = function(seat) {
    this.seats.remove(seat);
};

4- you can also keep a variable that references the correct value of this in your function and just use it. To me, this is the simplest, cleanest, and most reliable method.

var ViewModel = function() {
   var self = this;
   self.removeSeat = function(seat) {
       self.seats.remove(seat);
   }
};

Now, you can just use self in any handlers and completely avoid issues with this. Note that it certainly can be defined as this.removeSeat in the example, but I just like to use all self for cosmetic reasons.

So, just to summarize my thoughts:

  • I do think that this is confusing, especially to new users of the library
  • I personally feel that this is logical, given the way that JavaScript works.
  • If this can be changed/improved in a non-breaking way, then that would be a good thing
  • We do need to improve the docs on this topic.
  • There are many ways to deal with this and still keep the markup clean.

Thanks!

@tjcrowder

@rniemeyer:

Thanks! I understand your points. I don't agree, but respect your reasoning. Here are some counter-points:

I personally feel that this is logical, given the way that JavaScript works.

I get what you're saying, in that it's consistent with a hypothetical binding call in code, e.g.:

ko.addBinding(element, "click", $root.removeSeat);

But there are important differences:

  1. The text in the data-bind attribute is not JavaScript code. Parts of it are, but overall it isn't: It's text we're supplying to the library and asking the library to interpret. So to me, the library is acting a bit like the JavaScript interpreter here and I expect to see it set up the method call the way JS does. (Arguing against myself: That's if you see it as a call; if you see it as an assignment, then of course it's doing exactly what JS does.)

  2. The data-bind attribute is in many ways like the old DOM0 handlers (although dramatically more flexible and much more intelligent about scope!), but works differently from them: If I put onclick="$root.removeSeat(this)" on an element, the removeSeat call is made with the right this value (receiving the element as an argument). That was quite a common thing to do with object-oriented code. The difference with KO is that we're not coding a call, but it's a subtle difference and likely to trip people up.

  3. A well-designed library with code akin to the hypothetical addBinding call above should provide a means (argument or option parameter or similar) of specifying the context. For instance, jQuery's context option for ajax calls. (One of my criticisms of jQuery is that there is no equivalent of it when hooking up events.)

If this can be changed/improved in a non-breaking way, then that would be a good thing

Agreed. Michael's fork uses a flag on applyBindings, which seems like it makes good sense. People who prefer the current behavior just don't supply the flag. People who prefer the bound behavior enable it explicitly.

There are many ways to deal with this and still keep the markup clean.

For non-trivial code, I'd have to say most of them aren't best practices. The self thing is great for trivial models, but cannot participate properly in inheritance. Most of the others are just various flavors of remembering to use bind, which bloats the code and invites bugs when you forget or do it slightly incorrectly. :-) (Your first example under # 3, for instance, is missing an assignment or something; now, naturally you didn't double-check it nor should you have, it was just an off-the-cuff example, not production code, but it's the kind of mistake it's easy to make.) Libraries are here to help us avoid repeating ourselves like that. As KO is already doing a call to the function and specifying a specific this value, going through another layer of indirection (and having to pepper our markup or code with a bunch of bind calls) is (to me) non-optimal. (And yes, that's the same criticism I have of jQuery's bind and on functions.)

Anyway, totally get and respect your point that it's like an assignment in many ways. I just think that the more useful, and (I hate this phrase) less-surprising behavior, would be to set up calls to model object functions where this is the object in question, rather than something else.

Best,

-- T.J.

@rniemeyer
Collaborator

I updated the sample.

Like I said, I am probably in the minority on this one, but I really don't oppose the change.

I think that using some flavor of bind is a fact of life in most libraries. In KO, we have extra parameters for computed observables and manual subscriptions to do this for you. We don't for functions, because they are not our construct (and why bind and things like $.proxy already exist). You certainly could write a wrapper to do this for you or use a helper to bind all of the functions on a view model (or an array of functions to be bound is common in other libraries).

I don't necessarily agree with your statement about best practices, but that is just subjective. Of course self is not the best if there are going to be many instances of an object. Putting them on the prototype and then creating bound versions on the instance is common for callbacks/handlers.

Part of this comes down to Knockout not forcing the developer into using a certain pattern for the view model. KO does not provide or make you define something like a ko.model that handles some of this plumbing for you. I think that there is plenty of room in that area for improvement, but it would likely come in the form of plugins on top of KO.

So, as I said originally, it would really only be helpful to have this supported in a non-breaking way. The use case of relying on this to be something other than the object that holds the function is not necessary in KO 2.0, as the data is now passed as an argument (prior to 2.0, it was somewhat common for people to call $root.removeSeat and rely on this being the item, as long as you had a proper reference to the seats array). Relying on this to be an item in a function on the view model is really not a good choice and is no longer necessary.

I am in favor of a change like this in a non-breaking way short-term and in a major version it could possibly be the default.

@rossisdead

I'd really like it if this change were to go through. I have a strong preference for using prototype inheritance, especially in memory constrained areas(like mobile devices). Having to create new copies of each function(either by creating them in the view model constructor, or using the bind method) that takes place in a binding can easily use up a lot more memory than necessary.

@mbest
Collaborator

Let's expand this to cover any callback function from bindings:

  1. event and click
  2. submit
  3. template: name, afterRender, afterAdd, and beforeRemove
  4. options: optionsValue and optionsText
@paglias

Any step forward on this?

@davidaurelio

Any news on this?

I just started with the interactive tutorial, and the context of event handlers like $root.arbitrary felt really counter-intuitive. As a beginner, I expected object.property to be meaningful in a sense that knockout would understand that this should be a method invocation on an object.

I understand the point about it being a function reference, and while it is technically correct it just does not seem to make any sense from the user’s view: why would I want to invoke a method of one object in the context of another? Especially if I’ll get that object/value passed as argument?

To regard such expressions as function reference is an internal viewpoint. Knockout performing a method call seemed to be the most obvious thing to me as a new user.

Solving it with an additional self just feels like overkill. It feels like I have to do it for knockout, not for me. The same applies to the use of bind. I feel that the framework should invoke the method in the right context for me. I really don’t want to do all that extra work. Also, bind is not supported in legacy browsers.

@mbest
Collaborator

I'm not sure when this will be changed in Knockout, but for now you can use my Knockout.Punches plugin to add this functionality to Knockout 3.0.

@rniemeyer
Collaborator

I was going to point out the same about Michael's plugin: http://mbest.github.io/knockout.punches/#wrapped-event-callbacks

We may need to revisit where we stand on this issue overall for Knockout core. At this point, I could see Michael's functionality moving into KO at some point (of course, it would be a breaking change now).

One other note: knockout adds a polyfill if the browser does not support bind, so you are safe to use bind in a KO application.

I tend to use this plugin (https://github.com/rniemeyer/knockout-delegatedEvents) in my applications and it handles event delegation as well as using the correct value of this when it finds the appropriate method to call.

@davidaurelio

Thanks for the hints, @mbest and @rniemeyer. Much appreciated!

@jbblanchet

I was looking at a similar problem in a component and I couldn't help but think that what I really wanted was to be able to set my own scope for the callback, similar to what I do on a jQuery ajax call. Something like:

<div data-bind="click: { method: $root.doSomething, scope: $root }" />

This could also be a solution to #907 too. This wouldn't change the default current behavior, and would offer a lot more flexibility.

Is that something that has already been discussed?

@stalniy

@jbblanchet You can bind function to this inside constructor and then no need to use complicated declaration for click binding

function Dog(name) {
   this.name = name;
   this.bark = this.bark.bind(this);
}

Dog.prototype.bark = function() {
   console.log(this.name, 'barks');
};
<a href data-bind="click: $root.bark">bark</a>
@jbblanchet

Oh I know that. I can also use a "self" closure and even do the bind call inside the data-bind tag. I'm not looking for a workaround. But these are hacks built around the fact that I can't control the scope of this on my callbacks. This forces me to change my view-model to work around a limitation of the framework. And this also doesn't work nicely with TypeScript code (once again I know that there are workarounds).

You call my binding complex, but to me, it's a more elegant solution: it's straightforward, self-explanatory, documented, and I don't have to look around my code to understand were the scope is coming from. I'm not a fan of the "scope" name, but "bind", "binding" and "context" already have a meaning in knockout, and "this" seems risky.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.