controllerAs in directives doesn't play well with isolate scope

[thorn0]

I'd like to use the 'controllerAs' option in my directives. Let me cite the reasoning from An AngularJS Style Guide for Closure Users at Google:

Why? Putting methods and properties directly onto the controller, instead of building up a scope object, fits better with the Google Closure class style. Additionally, using 'controller as' makes it obvious which controller you are accessing when multiple controllers apply to an element. Since there is always a '.' in the bindings, you don't have to worry about prototypal inheritance masking primitives.

But I can see an issue with using this approach if the directive has isolate scope bindings.

angular.module('cmw').directive('fooWidget', function() {
    return {
        controller: function() {
            this.qux = '123';
        },
        controllerAs: 'fooWidget',
        scope: {
            bar: '='
        },
        template: ' {{fooWidget.qux}}  {{bar}} '
    };
});

In this case, the bar property is attached to the scope, not to the controller, which results in a confusing inconsistent situation where different properties should be looked for in different places.

As for now I see two alternatives, both not appealing to me:

1. to create bindings manually by calling $observe for each attribute needed, or
2. to forgo using 'controllerAs' at all.

It might make sense to introduce another special character for the scope entries in directive definition objects. This character would mean that the given property is a property of the controller, not the scope. What about dot?

angular.module('cmw').directive('fooWidget', function() {
    return {
        controller: function() {
            this.qux = '123';
        },
        controllerAs: 'fooWidget',
        scope: {
            bar: '=.',
            abc: '=.xyzzy'
        },
        template: ' {{fooWidget.qux}}  {{fooWidget.bar}} '
    };
});

[caitp]

1. to create bindings manually by calling $observe for each attribute needed, or
2. to forgo using 'controllerAs' at all.

Option 3: just reference them as {{bar}} / {{abc}} / {{qux}} in template, the controller can't really do much with them anyways since they are kept in sync with the external scope anyways (barring the use of bind-once), so changes that the controller makes wouldn't be reflected, and would just give the dirty checker more work to do.

Yes, I am aware that you have a bit of a problem when child scopes are in the picture, but there's a workaround for that anyways

[thorn0]

Firstly, it's inconsistent. Secondly, we lose all the cited benefits from Google's guide.

[caitp]

You lose exactly 1) benefit: the ability to safely write the properties from a child scope without using $parent or similar.

Anyways, lets see what other people think. @btford do you have an opinion on this?

[thorn0]

I'm keen on TypeScript and prefer the class style for controllers. In the methods code, it looks messy that some properties are accessed like this.prop and other like this.scope.prop.

Also this point is something I don't want to lose:

makes it obvious which controller you are accessing when multiple controllers apply to an element

[thorn0]

Here's a plunk to play with: http://plnkr.co/edit/ruwlP0eXOl3vxQbdqkSD
@caitp you can check your objections about dirty checker etc.

[caitp]

Each time you click the button, with that service being used, you iterate through the scope traversal about 10 times --- Without it, you iterate through scope traversal about 3 times per click. (In the case of this particular plunker, we're actually checking watches 7 times per click with the service being used, and twice per click without it)

Iterating through scope traversal/dirtychecking is not always cheap, it can be expensive when listeners (or watched expressions, for that matter) perform expensive operations

Also important to note is that this plunker demo is a trivial example, whereas a real application would be hurt quite a lot more by this pattern. So don't do this to get around limitations of the compiler, it's not a very good idea

As I said, we should get opinions from other folks about this to determine if it should be WONTFIX or not. I'm not opposed to adding support for this, but I feel like it's probably just going to make the compiler or directive api even crazier than it already is

[thorn0]

Sorry, but I just can't get how this code can significantly differ in terms of performance from what we have when using scope: { bar: '=' }. That's what we need to compare.

This fork of the plunk uses standard binding to the scope: http://plnkr.co/edit/7mWyOZIhjXklndFEHnkZ
Does it show any performance gains over the original plunk?

[caitp]

thorn0: the performance issue is hard to demonstrate with small plunkers like this, because you aren't showing a pathological case.

The most pathological case would be where you are binding to non-const JSON literals with many properties doing non-trivial work processing their values, which is sometimes the case for ngClass or even ngOptions --- and you would expand on this pathological case by properly using the same comparison mechanism as the isolate scope (relying on angular.equals) --- doing this twice can potentially create model instability and end up calling the watchers multiple times, where they would be doing at least one, and possibly two angular.equals comparisons, after returning a value which would itself then be checked against the old value redundantly using angular.equals --- and it would do that not just for the isolate scope, but also for your service.

So basically what this does is it just increases the workload of the dirtychecker significantly.

Is the pathological case the most common? That depends on what kind of data is being used. You can obviously never have an assignable JSON literal, so model instability isn't going to happen. It's not as bad when only used for primitive values, but it's still extra work being done that really does not need to happen, increasing the dataset the dirtychecker deals with.

[thorn0]

Doesn't the same thing happen with the current implementation of bindings like scope: { prop: '=,,,' }?

[caitp]

@thorn0 yes, but setting up a redundant watch in a separate service compounds the issue, especially in the pathological case (which may or may not effect your application)

[thorn0]

This watcher might be redundant, but it's exactly what $compile does in the current implementation. My code of that service isn't really mine, it's basically a copy-paste from $compile, except that for the sake of code size I removed some logic that handles literal values. From the performance point of view, it just can't matter whether $watch is called from a custom service or from $compile.

[caitp]

@thorn0 well it's not what $compile does, it's a little bit cheaper because you aren't using the special case for JSON literals, so you have some slight gains there, but lose the ability to deal with certain cases.

[caitp]

Anyways, I don't want to have an argument about this. I think in most cases, it is not going to matter a whole lot to use the redundant watch, it's just that it can matter, is what I'm saying. It's not an ideal thing to have to do. If you have a lot of these, and they're doing real work, and digests are frequent, it can add up.

I don't think it would necessarily be the end of the world to add builtin support for what you're asking, so that you don't have to add a redundant watch. My issue with it though, is that the solutions I'm imagining for this will either:

1. be a breaking change for anyone currently using an isolate scope + controllerAs (if putting stuff into the controller rather than scope becomes the de facto behaviour when controllerAs is used)
2. add additional rules to the isolate scope micro-syntax, making the directive api marginally more complicated than it already is

N'either of these seem particularly good to me, but I am not totally opposed to fixing them this way (more likely via option 2 --- but in order for that to happen, we need to decide if it's worth "fixing", so I would like to hear from other people, which may not happen this week.