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

fix for #11343 - bindToController for multiple directives #11345

Closed
wants to merge 2 commits into from
Closed

fix for #11343 - bindToController for multiple directives #11345

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
551adec
fix for #11343 - bindToController for multiple directives
jtorbicki Mar 17, 2015
3c608dd
fix for #11343 - bindToController for multiple directives - test added
jtorbicki Mar 18, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 14 additions & 15 deletions src/ng/compile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1970,26 +1970,25 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
}
if (elementControllers) {
// Initialize bindToController bindings for new/isolate scopes
var scopeDirective = newIsolateScopeDirective || newScopeDirective;
var bindings;
var controllerForBindings;
if (scopeDirective && elementControllers[scopeDirective.name]) {
bindings = scopeDirective.$$bindings.bindToController;
controller = elementControllers[scopeDirective.name];

if (controller && controller.identifier && bindings) {
controllerForBindings = controller;
thisLinkFn.$$destroyBindings =
initializeDirectiveBindings(scope, attrs, controller.instance,
bindings, scopeDirective);
}
}
for (i in elementControllers) {
var scopeDirective = newIsolateScopeDirective || controllerDirectives[i];
Copy link
Contributor

Choose a reason for hiding this comment

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

Why have you changed this from checking newScopeDirective to checking controllerDirectives[i]?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh hold on, you are actually calculating the controller directive here - probably should use a different name for the var.

var bindings;
var controllerForBindings;
if (scopeDirective && elementControllers[scopeDirective.name]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think is probably not quite right any more, since elementControllers[scopeDirective.name] is not related to the current iteration of this for loop

Copy link
Contributor

Choose a reason for hiding this comment

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

I see now that the scopeDirective is actually the current controller directive

bindings = scopeDirective.$$bindings.bindToController;
controller = elementControllers[scopeDirective.name];
if (controller && controller.identifier && bindings) {
controllerForBindings = controller;
thisLinkFn.$$destroyBindings =
initializeDirectiveBindings(scope, attrs, controller.instance,
bindings, scopeDirective);
}
}
controller = elementControllers[i];
var controllerResult = controller();
if (controllerResult !== controller.instance) {
controller.instance = controllerResult;
$element.data('$' + directive.name + 'Controller', controllerResult);
$element.data('$' + controllerDirectives[i].name + 'Controller', controllerResult);
if (controller === controllerForBindings) {
// Remove and re-install bindToController bindings
thisLinkFn.$$destroyBindings();
Expand Down
62 changes: 62 additions & 0 deletions test/ng/compileSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4033,6 +4033,68 @@ describe('$compile', function() {
});


it('should bind to multiple directives controllers via object notation (new scope)', function() {
var controllerCalled = false;
var secondControllerCalled = false;
module(function($compileProvider, $controllerProvider) {
$controllerProvider.register('myCtrl', function() {
expect(this.data).toEqualData({
'foo': 'bar',
'baz': 'biz'
});
expect(this.str).toBe('Hello, world!');
expect(this.fn()).toBe('called!');
controllerCalled = true;
});
$controllerProvider.register('secondCtrl', function() {
expect(this.data).toEqualData({
'foo2': 'bar2',
'baz2': 'biz2'
});
expect(this.str).toBe('Hello, second world!');
expect(this.fn()).toBe('second called!');
secondControllerCalled = true;
});
$compileProvider.directive('fooDir', valueFn({
bindToController: {
'data': '=dirData',
'str': '@dirStr',
'fn': '&dirFn'
},
scope: true,
controller: 'myCtrl as myCtrl'
}));
$compileProvider.directive('barDir', valueFn({
bindToController: {
'data': '=barData',
'str': '@barStr',
'fn': '&barFn'
},
scope: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

technically, you're not supposed to have multiple "new scope" directives on the same element, so this doesn't really work in practice (if it does work, it's a bug!).

I'm a bit uncomfortable with this because, given the above, it could become unclear what's happening if you have directives which implicitly depend on there being a new scope directive on the same element to work correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@caitp This is not what the issue is about. The "scope: true" functionality creating the common scope that's being shared among the directives is already in the Angular. Those directives can interact with one another by reading and writing to the scope directly without the need of bindToController. Whether this is good or not is probably up for another discussion. This issue is about bindToController binding the data for only single directive leaving all others in the undefined state. That's obviously not correct. It should either bind for all, or bindToController should be disabled for "scope: true".
On a side note: In my view there is nothing wrong with multiple directives sharing the same common scope, especially with the bindToController option that binds the data for different controllers in all directives, so there is no danger of polluting the scope.

Copy link
Contributor

Choose a reason for hiding this comment

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

Believe it or not, I understand what this issue is about --- The problem is the "scope: true" behaviour being shared by multiple directives on the same element. This was never meant to be supported, there is a WONTFIX'd bug about this, because it's explicitly not supported, it's just that we fail to throw the error sometimes (depending on types of scopes used, and directive priorities). This is never something that should be depended on

Copy link
Contributor

Choose a reason for hiding this comment

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

We do support multiple directives asking for new normal scope (i.e. not isolate) as can be seen in the docs (https://docs.angularjs.org/api/ng/service/$compile#-scope-):

The scope property can be true, an object or a falsy value:

true: A new child scope that prototypically inherits from its parent will be created for the directive's element. If multiple directives on the same element request a new scope, only one new scope is created. The new scope rule does not apply for the root of the template since the root of the template always gets a new scope.

So this is a valid bug and the fix looks pretty good to me too. I will give it a review and land this week.

controller: 'secondCtrl as secondCtrl'
}));
});
inject(function($compile, $rootScope) {
$rootScope.fn = valueFn('called!');
$rootScope.whom = 'world';
$rootScope.remoteData = {
'foo': 'bar',
'baz': 'biz'
};
$rootScope.fn2 = valueFn('second called!');
$rootScope.whom2 = 'second world';
$rootScope.remoteData2 = {
'foo2': 'bar2',
'baz2': 'biz2'
};
element = $compile('<div foo-dir dir-data="remoteData" dir-str="Hello, {{whom}}!" dir-fn="fn()" bar-dir bar-data="remoteData2" bar-str="Hello, {{whom2}}!" bar-fn="fn2()" ></div>')($rootScope);
$rootScope.$digest();
expect(controllerCalled).toBe(true);
expect(secondControllerCalled).toBe(true);
});
});


it('should put controller in scope when controller identifier present but not using controllerAs', function() {
var controllerCalled = false;
var myCtrl;
Expand Down