ngModelCtrl pollutes child scope #8539

Izhaki · 2014-08-08T13:51:38Z

There seems to be a bug with ngModelController $parsers - instead of updating the model (on the parent scope), they create a new property on the local (child) scope. Oddly, this doesn't happen with an isolated scope.

With this html (plunk):

<body ng-controller='main'>
    <p>{{appCount}}</p>
    <input dir ng-model='appCount'>
</body>

And this script:

.controller( 'main', function( $scope ) {
  $scope.appCount = '0';
})

.directive( 'dir', function() {
  return {
    require: 'ngModel',

    scope: true,

    link: function( scope, element, attributes, modelCtrl ) {
      modelCtrl.$parsers.unshift( function ( aInputValue ) {
        console.log( scope );
        return aInputValue;
      });   
    }
  }  
})

The actual model value never update on the controller level. Instead, a property called 'appCount' is created on the scope of the directive.

It seems that the issue is on this line (it sets the model on the child scope, not parent scope):

      ngModelSet($scope, ctrl.$modelValue);

within the following ngModelController code:

  this.$$writeModelToScope = function() {
    var getterSetter;

    if (ctrl.$options && ctrl.$options.getterSetter &&
        isFunction(getterSetter = ngModelGet($scope))) {

      getterSetter(ctrl.$modelValue);
    } else {
      ngModelSet($scope, ctrl.$modelValue);
    }
    forEach(ctrl.$viewChangeListeners, function(listener) {
      try {
        listener();
      } catch(e) {
        $exceptionHandler(e);
      }
    });
  };

The text was updated successfully, but these errors were encountered:

pkozlowski-opensource · 2014-08-09T19:05:03Z

OK, so this is situation very similar to the one that we had with isolated scopes and that was corrected in b5af198. Here we've got exactly the same situation but for child scopes.

Here is a smaller reproduce scenario: http://plnkr.co/edit/yKMzQD?p=preview which shows that it has nothing to do with the $parsers pipeline but it is a child scope that messes up things. It "works correctly" for isolated scopes: http://plnkr.co/edit/3Sdiaq?p=preview

I guess we should unify behaviour for isolated and child scopes so this should be seen as a bug.

@Izhaki out of curiosity - what is your exact use case? Why do you need to have a child scope in a directive that works alongside ngModel? In any case you can easily work-around it today by creating your child-scope manually: http://plnkr.co/edit/6tlWam?p=preview

caitp · 2014-08-10T14:08:53Z

I don't think this is a bug --- the isolate scope doesn't get passed to every directive applied to an element, it's self-contained.

The fact that the ng-model controller doesn't use the isolate scope works as expected (but I'm not saying it doesn't confuse people)

FesterCluck · 2014-08-10T14:20:25Z

Given the ng-model is set on a directive which has it's own scope, this behavior seems completely accurate. Child scopes don't update parent scopes. For an example see http://plnkr.co/edit/5BVZiEEUWXrF2U6gVbc7 . Note that updating the first input does update the second, but only as long as the second isn't manually updated.

pkozlowski-opensource · 2014-08-10T14:54:49Z

@FesterCluck I'm not sure what you mean by "ng-model is set on a directive which has it's own scope". There is no such thing as setting ng-model on another directive. What is happening here that both directives are used on the same element and the intuition is that ng-model directive is bound to a different scope than a directive requesting a child scope. This is how it works for combination of ng-model and another directive requesting an isolated scope on the same DOM element.

@caitp yeh, I'm not sure if this is a bug or not, I guess I'm more concerned with the discrepancy among directives asking for a child scope and an isolated scope on the same element. Please compare:

ng-model + a directive for an isolate scope = "works": http://plnkr.co/edit/6tlWam?p=preview
ng-model + a directive asking for a child scope = "doesn't work": http://plnkr.co/edit/yKMzQD?p=preview

The question is this: should a directive asking for a child scope should "force" this scope on ng-model. This used to happen with directives asking for an isloated scope but was changed in b5af198. Unless I'm drawing too many parallels between behaviour of directives asking for an isolated and child scope.

In short: should a child scope be self-contained as it is the case for isolated scopes?

pkozlowski-opensource · 2014-08-10T15:13:12Z

I'm not sure if I'm making myself clear but hopefully this plunk showing 2 situations side-by-side should make it more explicit: http://plnkr.co/edit/6QZfRh?p=preview

FesterCluck · 2014-08-11T02:03:18Z

@pkozlowski-opensource Child scopes are not 2 way bound, plain and simple. As soon as the child scope defines a property locally, inheritance is lost. IMHO prototypical inheritance behavior should be retained somewhere, and this is it.

After spending a good 2 hours on this I realized how it can be confusing that a single directive can usurp the scope of all directives on an element. However, adding some config property to change this behavior seems moot when isolated scopes can perform the same behavior in a different way.

Izhaki · 2014-08-11T10:11:42Z

@pkozlowski-opensource

The directive in question is a typeahead one introduced on an input element.

Reasons for using child scope

There are three reasons why child scope is used:

Complex attributes

Historically (if I can use the term as it was very recent and short lived), the directive received an expression as a config, same as ngRepeat:

<input typeahead="option in options" ...>

Then a parser, like with ngRepeat, worked out the collection (options) and had to bind it to the scope. There is no straight-forward way of doing this with an isolated scope.

Object configs

Then, as more and more config options were added, the syntax changed to a config object:

            <input
                class        = 'form-control'
                name         = 'state'
                id           = 'local-select'
                placeholder  = 'Enter search term'
                ng-model     = 'selected'
                cb-typeahead = "{
                    inputTemplate: '{{name}}',
                    valueField:    'name'
                    }"
                cb-typeahead-options-renderer = "{
                    type:           'local',
                    collection:     'states',
                    optionTemplate: '<cb-typeahead-highlight-query>{{match.name}}</cb-typeahead-highlight-query>',
                    filter:         '{name: query}'
                    }"
                />

Now while you can bind the config object to an isolated scope property:

scope: {
    config: '=cbTypeaheadOptionsRenderer'
}

There is no way to use the native binding for object properties. This throws an error:

scope: {
    collection: '=cbTypeaheadOptionsRenderer.collection'
}

Only one isolated scope

Then comes the natural reluctancy to use isolated scopes, as only one can be defined per element.

In another directive, I wish to separate the Drag and Drop (DnD) logic from a tree logic, allowing DnD to be used only when needed, something like so:

<treeGrid treeGridDnD='{...}'>

Now which one of them should get an isolated scope? Both, ideally. But you can't - so to play it 'safe' you use child scope in both.

In other words, to make sure a directive works alongside other directive, it is safer to use a child scope.

Private scope won't work

The proposal to create a private scope (a manually created child scope) won't work in this case.

The directives has the following structure:

Input + Typeahead
    Popup
        Options + ModelSelect
            OptionsRenderer
                ModelOption

With some of the directives below typeahead needing its scope to work (eg, Popup gets its config from configs given to Typeahead). A private scope is not seen by directives below the current DOM node.

Izhaki · 2014-08-11T10:22:09Z

@FesterCluck

Child scopes are not 2 way bound, plain and simple.

Well, it can be well useful if they would, and the directive in question does it manually. See Expose isolated scope binding API.

I personally don't see any justifications for binding to be a trait of an isolated scope only - you bind on a scope, so any scope should offer that, isolated or not.

Izhaki · 2014-08-11T10:34:40Z

@FesterCluck @caitp

I believe I understand your claim this works as excepted, particularly if one considers the case of isolated scope (isolated scope works just fine - it's child scopes that don't).

As this issue demonstrates, my understanding of scopes is incomplete, although I understand it better now then I did when I filed the referenced issue.

But allow me to provide my point of view, if not only for the sake of revealing how others may think how things should work.

Consider the following:

<div ng-init="count = 0">Parent
    <div myDirective ng-model="count">Child</div>
</div>

Parent scope

Now if myDirective uses parent scope (no scope definition), it should be clear that ngModel should get/set the model (count) from the parent scope.

Isolated scope

If myDirective uses isolated scope (scope: {}), it also makes sense for ngModel not to affect the isolated scope and get/set the model from the parent scope.

If I understand correctly by looking at nodeLinkFn in $compile, the reason this works is that the isolated scope is created, given to the directive in question controller/link functions, but that scope is unseen by the ngModel directive, which still refers to, gets and sets, from/to the parent scope.

Child scope

If myDirective uses child scope (scope: true), why on earth ngModel will set/get from that child scope? What is the logic behind it? That scope shouldn't have that model as a property.

The only reason get works for ngModel is due to prototypical inheritance. But would it work if the child scope defined a property with the same name as the model? Is one prohibited from choosing a child scope property with the same name as the parent property specified by ngModel?

In other words, my argument is that ngModel should always get/set from/on the parent scope, not any other scope that might be defined on that node.

Again, look at the code:

<div ng-init="count = 0">Parent
    <div myDirective ng-model="count">Child</div>
</div>

Why should ngModel work on any scope defined on the child node? It should always use the parent scope of the node it matches (like it currently does with an isolated scope).

FesterCluck · 2017-03-09T04:44:55Z

Could someone check if this still needs discussing?

I believe the core description of this discussion could be "Directive child scopes prototypically inherit from one another with parent scope as the prototype. The behavior of base owned properties can be confusing. Should something be done?"

My vote is no and that this issue be closed.

Izhaki · 2017-03-09T10:25:35Z

Seeing as this has been around for nearly 3 years with little activity - closing.

pkozlowski-opensource added component: $compile labels Aug 9, 2014

pkozlowski-opensource added this to the Backlog milestone Aug 9, 2014

Izhaki changed the title ~~ngModelCtrl $parsers pollute child scope~~ ngModelCtrl pollute child scope Aug 10, 2014

Izhaki changed the title ~~ngModelCtrl pollute child scope~~ ngModelCtrl pollutes child scope Aug 11, 2014

pkozlowski-opensource mentioned this issue Aug 22, 2014

directives on an element see the 'new' scope created by a sibling directive #8724

Closed

Izhaki closed this as completed Mar 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ngModelCtrl pollutes child scope #8539

ngModelCtrl pollutes child scope #8539

Izhaki commented Aug 8, 2014

pkozlowski-opensource commented Aug 9, 2014

caitp commented Aug 10, 2014

FesterCluck commented Aug 10, 2014

pkozlowski-opensource commented Aug 10, 2014

pkozlowski-opensource commented Aug 10, 2014

FesterCluck commented Aug 11, 2014

Izhaki commented Aug 11, 2014

Izhaki commented Aug 11, 2014

Izhaki commented Aug 11, 2014

FesterCluck commented Mar 9, 2017

Izhaki commented Mar 9, 2017

ngModelCtrl pollutes child scope #8539

ngModelCtrl pollutes child scope #8539

Comments

Izhaki commented Aug 8, 2014

pkozlowski-opensource commented Aug 9, 2014

caitp commented Aug 10, 2014

FesterCluck commented Aug 10, 2014

pkozlowski-opensource commented Aug 10, 2014

pkozlowski-opensource commented Aug 10, 2014

FesterCluck commented Aug 11, 2014

Izhaki commented Aug 11, 2014

Reasons for using child scope

Complex attributes

Object configs

Only one isolated scope

Private scope won't work

Izhaki commented Aug 11, 2014

Izhaki commented Aug 11, 2014

Parent scope

Isolated scope

Child scope

FesterCluck commented Mar 9, 2017

Izhaki commented Mar 9, 2017