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

Commit

Permalink
fix($compile): make isolate scope truly isolate
Browse files Browse the repository at this point in the history 
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 #1924
Closes #2500
  • Loading branch information
@vojtajina @IgorMinar
vojtajina authored and IgorMinar committed Nov 8, 2013
1 parent 3662140 commit 909cabd
Show file tree
Hide file tree
Showing 3 changed files with 114 additions and 29 deletions.
56 changes: 31 additions & 25 deletions src/ng/compile.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -901,7 +901,7 @@ function $CompileProvider($provide) {
return linkFnFound ? compositeLinkFn : null;

function compositeLinkFn(scope, nodeList, $rootElement, boundTranscludeFn) {
var nodeLinkFn, childLinkFn, node, childScope, childTranscludeFn, i, ii, n;
var nodeLinkFn, childLinkFn, node, $node, childScope, childTranscludeFn, i, ii, n;

// copy nodeList so that linking doesn't break due to live list updates.
var stableNodeList = [];
Expand All @@ -913,11 +913,13 @@ function $CompileProvider($provide) {
node = stableNodeList[n];
nodeLinkFn = linkFns[i++];
childLinkFn = linkFns[i++];
$node = jqLite(node);

if (nodeLinkFn) {
if (nodeLinkFn.scope) {
childScope = scope.$new(isObject(nodeLinkFn.scope));
jqLite(node).data('$scope', childScope);
childScope = scope.$new();
$node.data('$scope', childScope);
safeAddClass($node, 'ng-scope');
} else {
childScope = scope;
}
Expand Down Expand Up @@ -1155,10 +1157,8 @@ function $CompileProvider($provide) {
assertNoDuplicate('new/isolated scope', newIsolateScopeDirective, directive,
$compileNode);
if (isObject(directiveValue)) {
safeAddClass($compileNode, 'ng-isolate-scope');
newIsolateScopeDirective = directive;
}
safeAddClass($compileNode, 'ng-scope');
}
}

Expand Down Expand Up @@ -1291,7 +1291,7 @@ function $CompileProvider($provide) {

}

nodeLinkFn.scope = newScopeDirective && newScopeDirective.scope;
nodeLinkFn.scope = newScopeDirective && newScopeDirective.scope === true;
nodeLinkFn.transclude = transcludeDirective && childTranscludeFn;

// might be normal or delayed nodeLinkFn depending on if templateUrl is present
Expand All @@ -1303,11 +1303,13 @@ function $CompileProvider($provide) {
if (pre) {
if (attrStart) pre = groupElementsLinkFnWrapper(pre, attrStart, attrEnd);
pre.require = directive.require;
if (newIsolateScopeDirective === directive) pre.isolateScope = true;
preLinkFns.push(pre);
}
if (post) {
if (attrStart) post = groupElementsLinkFnWrapper(post, attrStart, attrEnd);
post.require = directive.require;
if (newIsolateScopeDirective === directive) post.isolateScope = true;
postLinkFns.push(post);
}
}
Expand Down Expand Up @@ -1348,7 +1350,7 @@ function $CompileProvider($provide) {


function nodeLinkFn(childLinkFn, scope, linkNode, $rootElement, boundTranscludeFn) {
var attrs, $element, i, ii, linkFn, controller;
var attrs, $element, i, ii, linkFn, controller, isolateScope;

if (compileNode === linkNode) {
attrs = templateAttrs;
Expand All @@ -1359,8 +1361,11 @@ function $CompileProvider($provide) {

if (newIsolateScopeDirective) {
var LOCAL_REGEXP = /^\s*([@=&])(\??)\s*(\w*)\s*$/;
var $linkNode = jqLite(linkNode);

var parentScope = scope.$parent || scope;
isolateScope = scope.$new(true);
$linkNode.data('$isolateScope', isolateScope);
safeAddClass($linkNode, 'ng-isolate-scope');

forEach(newIsolateScopeDirective.scope, function(definition, scopeName) {
var match = definition.match(LOCAL_REGEXP) || [],
Expand All @@ -1370,19 +1375,19 @@ function $CompileProvider($provide) {
lastValue,
parentGet, parentSet;

scope.$$isolateBindings[scopeName] = mode + attrName;
isolateScope.$$isolateBindings[scopeName] = mode + attrName;

switch (mode) {

case '@':
attrs.$observe(attrName, function(value) {
scope[scopeName] = value;
isolateScope[scopeName] = value;
});
attrs.$$observers[attrName].$$scope = parentScope;
attrs.$$observers[attrName].$$scope = scope;
if( attrs[attrName] ) {
// If the attribute has been provided then we trigger an interpolation to ensure
// the value is there for use in the link fn
scope[scopeName] = $interpolate(attrs[attrName])(parentScope);
isolateScope[scopeName] = $interpolate(attrs[attrName])(scope);
}
break;

Expand All @@ -1393,23 +1398,23 @@ function $CompileProvider($provide) {
parentGet = $parse(attrs[attrName]);
parentSet = parentGet.assign || function() {
// reset the change, or we will throw this exception on every $digest
lastValue = scope[scopeName] = parentGet(parentScope);
lastValue = isolateScope[scopeName] = parentGet(scope);
throw $compileMinErr('nonassign',
"Expression '{0}' used with directive '{1}' is non-assignable!",
attrs[attrName], newIsolateScopeDirective.name);
};
lastValue = scope[scopeName] = parentGet(parentScope);
scope.$watch(function parentValueWatch() {
var parentValue = parentGet(parentScope);
lastValue = isolateScope[scopeName] = parentGet(scope);
isolateScope.$watch(function parentValueWatch() {
var parentValue = parentGet(scope);

if (parentValue !== scope[scopeName]) {
if (parentValue !== isolateScope[scopeName]) {
// we are out of sync and need to copy
if (parentValue !== lastValue) {
// parent changed and it has precedence
lastValue = scope[scopeName] = parentValue;
lastValue = isolateScope[scopeName] = parentValue;
} else {
// if the parent can be assigned then do so
parentSet(parentScope, parentValue = lastValue = scope[scopeName]);
parentSet(scope, parentValue = lastValue = isolateScope[scopeName]);
}
}
return parentValue;
Expand All @@ -1418,8 +1423,8 @@ function $CompileProvider($provide) {

case '&':
parentGet = $parse(attrs[attrName]);
scope[scopeName] = function(locals) {
return parentGet(parentScope, locals);
isolateScope[scopeName] = function(locals) {
return parentGet(scope, locals);
};
break;

Expand All @@ -1435,7 +1440,7 @@ function $CompileProvider($provide) {
if (controllerDirectives) {
forEach(controllerDirectives, function(directive) {
var locals = {
$scope: scope,
$scope: directive === newIsolateScopeDirective ? isolateScope : scope,
$element: $element,
$attrs: attrs,
$transclude: boundTranscludeFn
Expand Down Expand Up @@ -1467,21 +1472,22 @@ function $CompileProvider($provide) {
for(i = 0, ii = preLinkFns.length; i < ii; i++) {
try {
linkFn = preLinkFns[i];
linkFn(scope, $element, attrs,
linkFn(linkFn.isolateScope ? isolateScope : scope, $element, attrs,
linkFn.require && getControllers(linkFn.require, $element));
} catch (e) {
$exceptionHandler(e, startingTag($element));
}
}

// RECURSION
childLinkFn && childLinkFn(scope, linkNode.childNodes, undefined, boundTranscludeFn);
// TODO(vojta): only pass isolate if the isolate directive has template
childLinkFn && childLinkFn(isolateScope || scope, linkNode.childNodes, undefined, boundTranscludeFn);

// POSTLINKING
for(i = postLinkFns.length - 1; i >= 0; i--) {
try {
linkFn = postLinkFns[i];
linkFn(scope, $element, attrs,
linkFn(linkFn.isolateScope ? isolateScope : scope, $element, attrs,
linkFn.require && getControllers(linkFn.require, $element));
} catch (e) {
$exceptionHandler(e, startingTag($element));
Expand Down
87 changes: 83 additions & 4 deletions test/ng/compileSpec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1375,7 +1375,7 @@ describe('$compile', function() {
return function (scope, element) {
iscope = scope;
log(scope.$id);
expect(element.data('$scope')).toBe(scope);
expect(element.data('$isolateScope')).toBe(scope);
};
}
};
Expand Down Expand Up @@ -1416,7 +1416,7 @@ describe('$compile', function() {
return function (scope, element) {
iscope = scope;
log(scope.$id);
expect(element.data('$scope')).toBe(scope);
expect(element.data('$isolateScope')).toBe(scope);
};
}
};
Expand Down Expand Up @@ -1535,7 +1535,7 @@ describe('$compile', function() {
expect(function(){
$compile('<div class="iscope-a; scope-b"></div>');
}).toThrowMinErr('$compile', 'multidir', 'Multiple directives [iscopeA, scopeB] asking for new/isolated scope on: ' +
'<div class="iscope-a; scope-b ng-isolate-scope ng-scope">');
'<div class="iscope-a; scope-b">');
})
);

Expand Down Expand Up @@ -2085,7 +2085,7 @@ describe('$compile', function() {


describe('isolated locals', function() {
var componentScope;
var componentScope, regularScope;

beforeEach(module(function() {
directive('myComponent', function() {
Expand All @@ -2112,6 +2112,23 @@ describe('$compile', function() {
scope: { attr: 'xxx' }
};
});
directive('storeScope', function() {
return {
link: function(scope) {
regularScope = scope;
}
}
});
}));

it('should give other directives the parent scope', inject(function($rootScope) {
compile('<div><input type="text" my-component store-scope ng-model="value"></div>');
$rootScope.$apply(function() {
$rootScope.value = 'from-parent';
});
expect(element.find('input').val()).toBe('from-parent');
expect(componentScope).not.toBe(regularScope);
expect(componentScope.$parent).toBe(regularScope)
}));

describe('attribute', function() {
Expand Down Expand Up @@ -2376,6 +2393,68 @@ describe('$compile', function() {
});


it('should require controller of an isolate directive from a non-isolate directive on the ' +
'same element', function() {
var IsolateController = function() {};
var isolateDirControllerInNonIsolateDirective;

module(function() {
directive('isolate', function() {
return {
scope: {},
controller: IsolateController
};
});
directive('nonIsolate', function() {
return {
require: 'isolate',
link: function(_, __, ___, isolateDirController) {
isolateDirControllerInNonIsolateDirective = isolateDirController;
}
};
});
});

inject(function($compile, $rootScope) {
element = $compile('<div isolate non-isolate></div>')($rootScope);

expect(isolateDirControllerInNonIsolateDirective).toBeDefined();
expect(isolateDirControllerInNonIsolateDirective instanceof IsolateController).toBe(true);
});
});


it('should require controller of a non-isolate directive from an isolate directive on the ' +
'same element', function() {
var NonIsolateController = function() {};
var nonIsolateDirControllerInIsolateDirective;

module(function() {
directive('isolate', function() {
return {
scope: {},
require: 'nonIsolate',
link: function(_, __, ___, nonIsolateDirController) {
nonIsolateDirControllerInIsolateDirective = nonIsolateDirController;
}
};
});
directive('nonIsolate', function() {
return {
controller: NonIsolateController
};
});
});

inject(function($compile, $rootScope) {
element = $compile('<div isolate non-isolate></div>')($rootScope);

expect(nonIsolateDirControllerInIsolateDirective).toBeDefined();
expect(nonIsolateDirControllerInIsolateDirective instanceof NonIsolateController).toBe(true);
});
});


it('should support controllerAs', function() {
module(function() {
directive('main', function() {
Expand Down
Empty file added xx
View file
Empty file.

5 comments on commit 909cabd

@scamden
Copy link

@scamden scamden commented on 909cabd Nov 8, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is amazing.

@forivall
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your work! Because of this problem, I minimally started a project to be able to rip out the isolate scope binding behaviour (with ast parsing and the like), and allow a controller to have its own scope, but this is definitely the better solution.

@rozzerhmq
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the good work!

@rgraffconnect
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works very VERY well now!

@ninjasort
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do validations work with these models? Do these model changes still populate through the $parsers array if you require the ngModelController?

I'm running into issues with something like this: (a contrived example, but it would be with multiple sub-models)

{{obj.prop.$error.validRes}} <---- this isn't working

<input ng-model="obj" ng-isolate>

.directive('ngIsolate', function() {
  return {
   require: 'ngModel',
    scope: {model: '=ngModel'},
    link: function (scope, el, attrs, ctrl) {
       var compiled = $compile('<input type="text" ng-model="model.prop">')(scope);
       el.replaceWith(compiled);
       scope.$watch(scope.model, function (val) {
          if (!val) {
           ctrl.$setValidity('validRes', false);
          }
       });
    }

  };
});

Please sign in to comment.