Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

ngModel is locked into the isolate scope when used on a component #1924

Closed
IgorMinar opened this issue Jan 30, 2013 · 39 comments
Closed

ngModel is locked into the isolate scope when used on a component #1924

IgorMinar opened this issue Jan 30, 2013 · 39 comments

Comments

@IgorMinar
Copy link
Contributor

When ngModel directive is used on an element that represents a component (implemented via a directive with isolate scope), ngModel is locked into this isolate scope and in order to get out and make ngModel useful the ngModel expression has to be prefixed with $parent.

so instead of:

{{my.model}}
<my-component ng-model="my.model"></my-component>

one has to write:

{{my.model}}
<my-component ng-model="$parent.my.model"></my-component>

this is non-intuitive and might be hard to debug for developer unfamiliar with what's going on.

the solution must however take into account that it's possible to use ngModel within the template of the isolated component, in which case we must not leak the model outside of the component.

@IgorMinar
Copy link
Contributor Author

btw the foodme app that we created as a tutorial app suffers from this issue: https://github.com/IgorMinar/foodme/blob/master/app/views/restaurants.html#L13

@mprobst
Copy link
Contributor

mprobst commented Jan 30, 2013

Another drawback: the use-site has to write $parent.my.model, so if you want to provide a re-usable component, the implementation of it leaks to all users of the component :-(

@jasonkuhrt
Copy link

@IgorMinar I just ran into this issue myself trying to create an angular component. Isolate scope seems to be by-far the most idiomatic looking way of writing directives but nearly 100% of the time the isolate-ness has been too agressive and made it impractical to use. Really frustrating. It seems like the problem is a design one, not a bug or anything.

Any advice regarding future plans for isolate?

@MikeMcElroy
Copy link
Contributor

FYI, when I ran into this problem, I used < ng-model="portal.myVar" portal="parentScopeVar" >
Where this was my isolate scope definition:
{
...
portal: "="
...
}

Which then made changes to (in the parent scope): parentScopeVar.myVar
Conceptually, it opens a "portal" to the parent scope through which ngModel can work.

Not a particularly elegant workaround, but it worked nicely, at least.

@jasonkuhrt
Copy link

I just ran into this again. Hard. Gah... this issue stands in the way of a cow-path I'm afraid. Its pretty critical to creating angular components.

@mhevery @IgorMinar Could $setViewValue support an optional second param for scope?

controller.$setViewValue(foo, scope.$parent);

There must be a better way than ^ though.

@caiotoon
Copy link
Contributor

caiotoon commented Apr 9, 2013

i have a suggestion. I believe all that must be done is to provide a way to the user instruct a bi-directional bind without specifying the local property name, but the name of attribute that contains the local property name.

Look, this is going to fail:

<my-component ng-model="my.model"></my-component>
scope: {
  ngModel: '=ngModel'
}

Currently, we are saying that when the user changes $scope. ngModel we'll reflect to $scope.$parent.my.model. All we got to do is to provide an attribute binding, something like this:

scope: {
  '=ngModel': '=ngModel'
}

So instead of binding local.ngModel to parent.my.model, we bind local.my.model (as ng-model="my.model") to parent.my.model. This would solve the problem and let it transparent to any directive or the outside user, requiring no refactoring at directives at all. Also, any other directive that hits the same problem can be easily circumvented with this approach.

I can try to implement it if Angular team approve this approach.

@il-juggler
Copy link

in the input.js we have:

var ngModelGet = $parse($attr.ngModel),
ngModelSet = ngModelGet.assign;

maybe explicity modifing the ngModelCtrl by asking for them in the link of the component so you can make something like
ngModel.$setContext(scope)
//or
ngModel.$setContext($scope.parent)

auto binding at the parser everything the model is setting/getting

@dobladez
Copy link

This is the workaround I currently use on my directive:
...
scope: {
id: '@', // any custom attribute
outterModel: '=ngModel' // get ahold of the model
},
...
// keep model in both scopes in sync:
scope.$watch('outterModel', function(val, old) {
scope.$eval(attrs.ngModel + ' = outterModel');
});
scope.$watch(attrs.ngModel, function(val, old) {
scope.outterModel = val;
});

I got the original idea form here:
http://stackoverflow.com/questions/15269737/why-is-ngmodel-setviewvalue-not-working-from

@gonzaloruizdevilla
Copy link
Contributor

@dobladez 's workaround has been useful for me too.

@il-juggler
Copy link

Please checkout this solution

#2904

@hon2a
Copy link

hon2a commented Jun 15, 2013

As is often the case with Angular, the easiest work-around is simple element nesting.

<div ng-model="">
   <my-component />
</div>
.directive('myComponent', function () {
   return {
      scope: {},
      require: '^ngModel'
      /* ... */
   };
});

@il-juggler
Copy link

@hon2a yeah! it is but not it enough elegant

@basarat
Copy link
Contributor

basarat commented Jul 3, 2013

Yup from : http://stackoverflow.com/a/15272359/390330

scope: {
    model: '=ngModel'
}

And then in your linking function keep both in sync:

// Bring in changes from outside: 
scope.$watch('model', function() {
    scope.$eval(attrs.ngModel + ' = model');
});

// Send out changes from inside: 
scope.$watch(attrs.ngModel, function(val) {
    scope.model = val;
});

The solution by @inzurrekt is nice
But my number 1 preference would be to have it more natively supported: #1924 (comment)

@caiotoon
Copy link
Contributor

caiotoon commented Jul 3, 2013

I'd still rather to have a native solution, that would make this sync transparent to the developer, making it eligible to further optimizations out of the box.

So I still think an additional signature would solve this problem, like here #1924 (comment)

Basically, allowing the user to bind to the same value as the attribute. This have a bonus of working for any other attribute we might need.

@basarat
Copy link
Contributor

basarat commented Jul 3, 2013

@caiotoon I agree, that would be the ultimate solution. But I would prefer a syntax more consistent with what's already there i.e. @,=,&. Perhaps ^

<my-component ng-model="my.model"></my-component>
scope: {
  model: '^ngModel'
}

From an implementation logic point of view it would mean that if you (within the directive) do:

scope.model = 123;

that would set the following for you, automatically, using $parse internally:

scope.$parent.my.model = 123; 

Additionally with ^ you would still get the behaviour you get from =

@basarat
Copy link
Contributor

basarat commented Jul 6, 2013

After a lot of thought. The simplest solution seems to be the one offered by @inzurrekt here: #2904

Basically have a $setContext function on ngModel:

ngModel.$setContext(scope.$parent);

The reason why this function is required is $scope is available via closure (and not a member of ngModel) so its not easily changable otherwise.

@basarat
Copy link
Contributor

basarat commented Jul 6, 2013

Simple modification in AngularJS

Based on @inzurrekt idea it can be solved with a simple $setScope function in ngModelController:

https://github.com/basarat/foodme/blob/master/app/lib/angular/angular.js#L11667

this.$setScope = function(context) {
     $scope = context;
}

You can see the pull request: #3148

Given that above is present in AngularJS

You can see that the directive no longer needs $parent in the html :
https://github.com/basarat/foodme/blob/master/app/views/restaurants.html#L13

<fm-rating ng-model="filter.rating"></fm-rating>

All you need to do in your directive definition is set the scope for ngModel:
https://github.com/basarat/foodme/blob/master/app/js/directives/fmRating.js#L15

// Move scope to parent for isloate scope ngModels 
ngModel.$setScope(scope.$parent);

And you can see the application still works : https://github.com/basarat/foodme

@basarat
Copy link
Contributor

basarat commented Jul 6, 2013

Since that pull request would probably (and validly since it only fixed ngModel) get rejected, here are some other ideas. Solution 2 is more work but backward compatible. Solution 1 is simpler but backward incompatible (and I feel a better solution overall)

Solution 1

If the directive has an isolate scope and requires a controller

scope :{},
require: '?ngModel', 

At the point where a required controller gets instantiated and a scope gets injected. The injector should pass scope.$parent automatically to (in this case) the ngModelController (or whatever else is required).

This would be different behaviour from what is there right now but I feel the current behaviour is actually not functional. However if you want the current behaviour to continue (e.g. to not break people that used $parent. in the expression) then having a generic method to support the a scope change in the injected directive controllers would be required (my pull request only fixed ngModel).

Which brings me to:

Solution 2

Building on previous suggestion of mine:

scope :{},
require: '=ngModel', 

= (or any other letter you want, maybe &) would mean use the scope of the parent when instantiating the ngModelController. This would depend upon scope (don't use parent if not an isolate scope).

Would the angular team agree on using something like this?

@il-juggler
Copy link

Maybe the solution (just for NgModel) is more simple. I thinked it a long time and finally can say there is no reason to use an ngModel for the isolated properties of an input, but to use for connect an "input Module" with a data model... in other words when you have an ngModel on the same node of an isolated scope directive we can assume you want to evaluate on the parent

@pheuter
Copy link

pheuter commented Aug 12, 2013

Looks like this issue has been open for half a year. Any ideas for viable solutions in the near term?

@basarat
Copy link
Contributor

basarat commented Aug 13, 2013

@pheuter The short term solution I copied out : #3148 was horrible. Sure it makes ng-model work but for ng-change / others you still need to do $parent, if you use them on the same element as the directive.

My new potential recommendation:

Basically a new option (perhaps called singleIsolate:true) that would mean do not leak the isolate scope to other directives on the same html element yet create a new one for this directive.

The location to modify would be around: https://github.com/angular/angular.js/blob/master/src/ng/compile.js?source=cc#L934-L1017

@caiotoon
Copy link
Contributor

@basarat, one of the premises is that we have only one scope per element, acting as a common medium for sharing messages, and thus, only one scope shared among all directives of a specific element. This change would also demand rethinking and maybe changing and redesigning scope() method and related features.

As we probably will need to address mixed cases (isolated and mixed properties), maybe the best solution is still to have a way of defining the two-way binding in a per property way.

@spamdaemon
Copy link

I've been bugged by this issue for the longest time. It's really annoying and it basically means you cannot easily write standalone components in angular.

I think a possible solution is to simply create a shared scope for an element if there is at least one directive
that needs its own scope (normal or isolate). Then, for each directive requiring an isolate scope an isolate scope is created whose $parent would be the shared scope.

If backwards compatibility is not an issue, then the easiest would be to always created a shared and an isolate scope for a directive that requiring a scope. This way, each directive would have a way to store private scope data and public scope data.

Would something like this not work or is there something fundamentally wrong with angular that prevents this issue from being addressed? I had really hoped this would be fixed for 1.2

@kevinsbennett
Copy link

This is not only an issue with ngModel, but with several other directives as well. Like ngShow, which has a similar thread going: #2500

In my mind, bindings placed on an element with an isolate scope should just bind to the parent scope. That's it. Why else would you be putting ngModel or ngShow on the element? No one wants them to bind to the isolate scope.

The real question to ask is if there a compelling reason NOT to have it this way?

@diegovilar
Copy link

In my mind, bindings placed on an element with an isolate scope should just bind to the parent scope.

👍

This does seem only logical. I'm with @kevinsbennett with this one so far. I can't come up with a scenario where that is not the case (then again, I didn't really give it that much of a thought). Any one? God knows the shameful hacks I've made to components with isolated scopes work together with other directives :/

@xaviershay
Copy link

I was just confused by this issue. I don't have anything to contribute re a solution, but am watching :)

@anagelcg
Copy link

anagelcg commented Oct 8, 2013

Until there is no proper solution for that problem, I also use the workaround 'dobladez' described above.

// Bring in changes from outside: 
scope.$watch('model', function() {
    scope.$eval(attrs.ngModel + ' = model');
});
// Send out changes from inside: 
scope.$watch(attrs.ngModel, function(val) {
    scope.model = val;
});

The synchronization with the two $watch-functions works well, but if you use an array-value as the
model, it breaks that concept.

<my-directive ng-model="myArray[0]"></my-directive>

When I do this, I get an excpetion when the first $watch-function evalues the expression:

TypeError: Cannot set property '0' of undefined
    at Function.extend.assign (http://localhost:9000/basTestClient/app/components/angular/angular.js:6518:59)
    at http://localhost:9000/basTestClient/app/components/angular/angular.js:6368:21
    at Object.$get.Scope.$eval (http://localhost:9000/basTestClient/app/components/angular/angular.js:8218:28)
    at Object.fn (http://localhost:9000/basCore/services/util/basDirectiveUtil.js:20:23)
    at Object.$get.Scope.$digest (http://localhost:9000/basTestClient/app/components/angular/angular.js:8097:27)
    at Object.$get.Scope.$apply (http://localhost:9000/basTestClient/app/components/angular/angular.js:8304:24)
    at done (http://localhost:9000/basTestClient/app/components/angular/angular.js:9357:20)

Does anybody have a solution for that problem?

@MikeMcElroy
Copy link
Contributor

Would a directive for ng-Model-type binding on isolate scopes be an
appropriate solution? That's what I've decided to do for the time being.
It required me to roll my own ngModelController, which was a bit of a pain
and a lot of duplication of the internal Angular implementation, but it in
effect binds the ng-model to the parent scope, which appropriately connects
as I expect it to.

I called it ngModelIsolate and an example of it can be found here:
http://plnkr.co/edit/7kzEM1Dj2llK7o1SzVQM

-MikeMac

On Tue, Oct 8, 2013 at 5:05 AM, anagelcg notifications@github.com wrote:

Until there is no proper solution for that problem, I also use the
workaround 'dobladez' described above.

// Bring in changes from outside:
scope.$watch('model', function() {
scope.$eval(attrs.ngModel + ' = model');
});

// Send out changes from inside:
scope.$watch(attrs.ngModel, function(val) {
scope.model = val;
});

The synchronization with the two $watch-functions works well, but if you
use an array-value as the
model, it breaks that concept.

When I do this, I get an excpetion when the first $watch-function evalues
the expression:

TypeError: Cannot set property '0' of undefined
at Function.extend.assign (http://localhost:9000/basTestClient/app/components/angular/angular.js:6518:59)
at http://localhost:9000/basTestClient/app/components/angular/angular.js:6368:21
at Object.$get.Scope.$eval (http://localhost:9000/basTestClient/app/components/angular/angular.js:8218:28)
at Object.fn (http://localhost:9000/basCore/services/util/basDirectiveUtil.js:20:23)
at Object.$get.Scope.$digest (http://localhost:9000/basTestClient/app/components/angular/angular.js:8097:27)
at Object.$get.Scope.$apply (http://localhost:9000/basTestClient/app/components/angular/angular.js:8304:24)
at done (http://localhost:9000/basTestClient/app/components/angular/angular.js:9357:20)

Does anybody have a solution for that problem?


Reply to this email directly or view it on GitHubhttps://github.com//issues/1924#issuecomment-25877990
.

@basarat
Copy link
Contributor

basarat commented Oct 11, 2013

I created an $isolator service as a temporary fix : https://github.com/basarat/demo-angularjs-easeljs/blob/gh-pages/typescript/ka/isolator.ts what this allows you to do is use an isloate scope within a directive but not have this isolate scope leaked to other directives on the same element, so you don't need to use $parent hack on other directives (e.g ng-model ng-show) on the same element.

See sample app : http://basarat.github.io/demo-angularjs-easeljs/
And the sample usage : https://github.com/basarat/demo-angularjs-easeljs/blob/gh-pages/typescript/directives/annotateimageDirective.ts#L16-L22

@wanjungao
Copy link

Sometimes, a widget directive needs to work with minimum effort of the developers who use it. Also it needs to have its own isolated scope to maintain its state.

for example:

<input type="text" onchange="verify()" ng-model="username" name="username">

change to

<input type="text" onchange="verify()" ng-model="username" name="username" delay="500">

to add the delayed execution behavior.

It would be too much to modify the ng-model or add additional attribute for the obj.attr workaround.

here is the workaround I came up with:

mod.directive('delay', function() {
    return {
        restrict: 'A',
        scope: { name1: "="},
        transclude: true, 
        compile:function (element, attrs, transclude) {
            if (attrs.ngModel) {
                attrs.$set("name1", attrs.ngModel, false);
                attrs.$set('ngModel', 'name1', false);
            }
        }
    };
  })

sample test here:
http://jsfiddle.net/pccXS/

@eddiemonge
Copy link
Contributor

this creates some weird things: http://plnkr.co/edit/7bad6WxZ8H18XJTAqOK3

@revolunet
Copy link
Contributor

Awesome guys Thank you! Time for real components now ;)

@spamdaemon
Copy link

This is really good news. I do have one question: what is the relationship between an isolate directive and a directive with shared scope on the same element?

@MikeMcElroy
Copy link
Contributor

Does this mean it's going into 1.2 stable when it rolls out? Pretty
significant breaking change if you developed a workaround for it.

-MikeMac

On Fri, Nov 8, 2013 at 8:16 AM, spamdaemon notifications@github.com wrote:

This is really good news. I do have one question: what is the relationship
between an isolate directive and a directive with shared scope on the same
element?


Reply to this email directly or view it on GitHubhttps://github.com//issues/1924#issuecomment-28065161
.

@basarat
Copy link
Contributor

basarat commented Nov 8, 2013

@MikeMcElroy 1.2 is out and this commit is a part of the changelog : https://github.com/angular/angular.js/blob/master/CHANGELOG.md

I thinks it's awesome :)

@basarat
Copy link
Contributor

basarat commented Nov 8, 2013

@spamdaemon same as the relationship between isolate scope and the scope of a parent element.

jamesdaily pushed a commit to jamesdaily/angular.js that referenced this issue Jan 27, 2014
Fixes issue with isolate scope leaking all over the place into other directives on the same element.

Isolate scope is now available only to the isolate directive that requested it and its template.

A non-isolate directive should not get the isolate scope of an isolate directive on the same element,
instead they will receive the original scope (which is the parent scope of the newly created isolate scope).

Paired with Tobias.

BREAKING CHANGE: Directives without isolate scope do not get the isolate scope from an isolate directive on the same element. If your code depends on this behavior (non-isolate directive needs to access state from within the isolate scope), change the isolate directive to use scope locals to pass these explicitly.

// before
<input ng-model="$parent.value" ng-isolate>

.directive('ngIsolate', function() {
  return {
    scope: {},
    template: '{{value}}'
  };
});

// after
<input ng-model="value" ng-isolate>

.directive('ngIsolate', function() {
  return {
    scope: {value: '=ngModel'},
    template: '{{value}}
  };
});

Closes angular#1924
Closes angular#2500
jamesdaily pushed a commit to jamesdaily/angular.js that referenced this issue Jan 27, 2014
Fixes issue with isolate scope leaking all over the place into other directives on the same element.

Isolate scope is now available only to the isolate directive that requested it and its template.

A non-isolate directive should not get the isolate scope of an isolate directive on the same element,
instead they will receive the original scope (which is the parent scope of the newly created isolate scope).

Paired with Tobias.

BREAKING CHANGE: Directives without isolate scope do not get the isolate scope from an isolate directive on the same element. If your code depends on this behavior (non-isolate directive needs to access state from within the isolate scope), change the isolate directive to use scope locals to pass these explicitly.

// before
<input ng-model="$parent.value" ng-isolate>

.directive('ngIsolate', function() {
  return {
    scope: {},
    template: '{{value}}'
  };
});

// after
<input ng-model="value" ng-isolate>

.directive('ngIsolate', function() {
  return {
    scope: {value: '=ngModel'},
    template: '{{value}}
  };
});

Closes angular#1924
Closes angular#2500
@shilan
Copy link

shilan commented Oct 1, 2015

thanks @pheuter for suggesting $parent thing! but I still have problem using $parent in typeaheads that I have in my templates. ng-change works fine on all inputs checkboxes but not on typeahead!

@navikale
Copy link

navikale commented Jun 3, 2016

@basarat

ngModel.$setScope(scope.$parent);

Is this even made to final commit or has been removed as of 1.5.6?
I stumbled upon same issue with typeahead and don't seem to find $setScope in source or anywhere in history for ngModel commits. Right now just using workaround by accessing child controller from parent to read the model value.

@Narretz
Copy link
Contributor

Narretz commented Jun 3, 2016

@NavnathKale This was never included. I've never seen it before, either.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests