From b89320a5f11b3ff3a4d0af5aa6fa1857bddfc91f Mon Sep 17 00:00:00 2001 From: James Talmage Date: Fri, 13 Mar 2015 22:20:35 -0400 Subject: [PATCH 1/5] test(compile): failing test - respect controller return value failing test for angular/angular.js#11147 Controllers instantiated for directives by the $compile service do not respect explicit return values from the controller function. --- test/ng/compileSpec.js | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index d1f97246774c..451a5fa7fa3e 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -4197,6 +4197,27 @@ describe('$compile', function() { }); + it('should respect controller return value', function() { + module(function() { + directive('logControllerProp', function(log) { + return { + controller: function($scope) { + this.foo = 'baz'; // value should not be used. + return {foo: 'bar'}; + }, + link: function(scope, element, attrs, controller) { + log(controller.foo); + } + }; + }); + }); + inject(function(log, $compile, $rootScope) { + element = $compile('')($rootScope); + expect(log).toEqual('bar'); + }); + }); + + it('should get required parent controller', function() { module(function() { directive('nested', function(log) { From 57fe7014c7187cb796af4d483e913c3a0dd29d38 Mon Sep 17 00:00:00 2001 From: James Talmage Date: Sat, 14 Mar 2015 00:37:35 -0400 Subject: [PATCH 2/5] fix(compile): respect explicit return values from controller functions When controller functions return an explicit value that value should be what is passed to the linking functions, and to any child/sibling controllers that `require` it. It should also be bound to the data store on the dom element. Closes #11147 --- src/ng/compile.js | 15 +++++++++------ test/ng/compileSpec.js | 1 + 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index 3de0d53f0388..00da91da14b5 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -1967,13 +1967,16 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { for (i in elementControllers) { controller = elementControllers[i]; var controllerResult = controller(); - if (controllerResult !== controller.instance && - controller === controllerForBindings) { - // Remove and re-install bindToController bindings - thisLinkFn.$$destroyBindings(); - thisLinkFn.$$destroyBindings = + if (controllerResult !== controller.instance && (isObject(controllerResult) || isFunction(controllerResult))) { + controller.instance = controllerResult; + $element.data('$' + directive.name + 'Controller', controllerResult); + if (controller === controllerForBindings) { + // Remove and re-install bindToController bindings + thisLinkFn.$$destroyBindings(); + thisLinkFn.$$destroyBindings = initializeDirectiveBindings(scope, attrs, controllerResult, - bindings, scopeDirective); + bindings, scopeDirective); + } } } } diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index 451a5fa7fa3e..dcdbe875615f 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -4214,6 +4214,7 @@ describe('$compile', function() { inject(function(log, $compile, $rootScope) { element = $compile('')($rootScope); expect(log).toEqual('bar'); + expect(element.data('$logControllerPropController').foo).toEqual('bar'); }); }); From d0f1285a3fe4f8857a2d172d8de2a8137b4cabe3 Mon Sep 17 00:00:00 2001 From: James Talmage Date: Mon, 16 Mar 2015 15:04:28 -0400 Subject: [PATCH 3/5] test(compile): additional tests for controllers with explicit return values. Includes tests for controllers that: - explicitly return primitives - are attached to scope using `controllerAs` - transclude contents --- src/ng/compile.js | 3 +- test/ng/compileSpec.js | 103 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 104 insertions(+), 2 deletions(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index 00da91da14b5..85c6cc4f97e9 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -1974,8 +1974,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { // Remove and re-install bindToController bindings thisLinkFn.$$destroyBindings(); thisLinkFn.$$destroyBindings = - initializeDirectiveBindings(scope, attrs, controllerResult, - bindings, scopeDirective); + initializeDirectiveBindings(scope, attrs, controllerResult, bindings, scopeDirective); } } } diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index dcdbe875615f..e76edd272566 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -4219,6 +4219,28 @@ describe('$compile', function() { }); + it('primitive controller return values are ignored', function() { + module(function() { + directive('logControllerProp', function(log) { + return { + controller: function($scope) { + this.foo = 'baz'; // value *will* be used. + return 'bar'; + }, + link: function(scope, element, attrs, controller) { + log(controller.foo); + } + }; + }); + }); + inject(function(log, $compile, $rootScope) { + element = $compile('')($rootScope); + expect(log).toEqual('baz'); + expect(element.data('$logControllerPropController').foo).toEqual('baz'); + }); + }); + + it('should get required parent controller', function() { module(function() { directive('nested', function(log) { @@ -4238,6 +4260,28 @@ describe('$compile', function() { }); + it('should get explicit return value of required parent controller', function() { + module(function() { + directive('nested', function(log) { + return { + require: '^^?nested', + controller: function() { + return {foo: 'bar'}; + }, + link: function(scope, element, attrs, controller) { + log(!!controller && controller.foo); + } + }; + }); + }); + inject(function(log, $compile, $rootScope) { + element = $compile('
')($rootScope); + + expect(log).toEqual('bar; false'); + }); + }); + + it('should get required parent controller when the question mark precedes the ^^', function() { module(function() { directive('nested', function(log) { @@ -4546,6 +4590,30 @@ describe('$compile', function() { }); + it('should respect explicit controller return value when using controllerAs', function() { + module(function() { + directive('main', function() { + return { + templateUrl: 'main.html', + transclude: true, + scope: {}, + controller: function() { + this.name = 'lucas'; + return {name: 'george'}; + }, + controllerAs: 'mainCtrl' + }; + }); + }); + inject(function($templateCache, $compile, $rootScope) { + $templateCache.put('main.html', 'template:{{mainCtrl.name}}
'); + element = $compile('
transclude:{{mainCtrl.name}}
')($rootScope); + $rootScope.$apply(); + expect(element.text()).toBe('template:george transclude:'); + }); + }); + + it('should support controller alias', function() { module(function($controllerProvider) { $controllerProvider.register('MainCtrl', function() { @@ -5547,6 +5615,41 @@ describe('$compile', function() { }); }); + it('should copy the explicit return value to all clones', function() { + module(function() { + directive('makeThree', valueFn({ + transclude: 'content', + controller: function($transclude) { + this.foo = 'baz'; + return {transclude:$transclude, foo: 'bar'}; + }, + link: function(scope, el, attr, ctrl) { + ctrl.transclude(cloneAttach); + ctrl.transclude(cloneAttach); + ctrl.transclude(cloneAttach); + + function cloneAttach(clone) { + el.append(clone); + } + } + })); + + directive('nested', function(log) { + return { + require: '^^makeThree', + link: function(scope, element, attrs, controller) { + log(controller.foo); + } + }; + }); + }); + inject(function(log, $compile) { + element = $compile('
')($rootScope); + $rootScope.$apply(); + expect(log).toEqual('bar; bar; bar'); + }); + }); + it('should provide the $transclude controller local as 5th argument to the pre and post-link function', function() { var ctrlTransclude, preLinkTransclude, postLinkTransclude; module(function() { From 0266a1cd3531abfcf05bcce7877cf12d3cededfe Mon Sep 17 00:00:00 2001 From: James Talmage Date: Mon, 16 Mar 2015 15:19:06 -0400 Subject: [PATCH 4/5] refactor(compile): remove duplicate type checks The `controller()` callback already performs the required type checks and guarantees that `controller() !== controller.instance` in the event that the function returns an explicit value. --- src/ng/compile.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index 85c6cc4f97e9..df5185020f5e 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -1967,7 +1967,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { for (i in elementControllers) { controller = elementControllers[i]; var controllerResult = controller(); - if (controllerResult !== controller.instance && (isObject(controllerResult) || isFunction(controllerResult))) { + if (controllerResult !== controller.instance) { controller.instance = controllerResult; $element.data('$' + directive.name + 'Controller', controllerResult); if (controller === controllerForBindings) { From f6f0117dc1ca3aa519bdb4a1f361bcf317859e96 Mon Sep 17 00:00:00 2001 From: James Talmage Date: Mon, 16 Mar 2015 18:28:36 -0400 Subject: [PATCH 5/5] test(compile): apply discussed changes from #11326 --- test/ng/compileSpec.js | 163 ++++++++++++++++++++--------------------- 1 file changed, 81 insertions(+), 82 deletions(-) diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index e76edd272566..b30ab8631d9a 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -4197,7 +4197,7 @@ describe('$compile', function() { }); - it('should respect controller return value', function() { + it('should respect explicit return value from controller', function() { module(function() { directive('logControllerProp', function(log) { return { @@ -4219,65 +4219,123 @@ describe('$compile', function() { }); - it('primitive controller return values are ignored', function() { + it('should get explicit return value of required parent controller', function() { module(function() { - directive('logControllerProp', function(log) { + directive('nested', function(log) { return { - controller: function($scope) { - this.foo = 'baz'; // value *will* be used. - return 'bar'; + require: '^^?nested', + controller: function() { + return {foo: 'bar'}; }, link: function(scope, element, attrs, controller) { - log(controller.foo); + log(!!controller && controller.foo); } }; }); }); inject(function(log, $compile, $rootScope) { - element = $compile('')($rootScope); - expect(log).toEqual('baz'); - expect(element.data('$logControllerPropController').foo).toEqual('baz'); + element = $compile('
')($rootScope); + + expect(log).toEqual('bar; false'); }); }); - it('should get required parent controller', function() { + it('should respect explicit controller return value when using controllerAs', function() { module(function() { + directive('main', function() { + return { + templateUrl: 'main.html', + scope: {}, + controller: function() { + this.name = 'lucas'; + return {name: 'george'}; + }, + controllerAs: 'mainCtrl' + }; + }); + }); + inject(function($templateCache, $compile, $rootScope) { + $templateCache.put('main.html', 'template:{{mainCtrl.name}}'); + element = $compile('
')($rootScope); + $rootScope.$apply(); + expect(element.text()).toBe('template:george'); + }); + }); + + + it('transcluded children should receive explicit return value of parent controller', function() { + var expectedController; + module(function() { + directive('nester', valueFn({ + transclude: 'content', + controller: function($transclude) { + this.foo = 'baz'; + expectedController = {transclude:$transclude, foo: 'bar'}; + return expectedController; + }, + link: function(scope, el, attr, ctrl) { + ctrl.transclude(cloneAttach); + function cloneAttach(clone) { + el.append(clone); + } + } + })); directive('nested', function(log) { return { - require: '^^?nested', - controller: function($scope) {}, + require: '^^nester', link: function(scope, element, attrs, controller) { - log(!!controller); + expect(controller).toBe(expectedController); + log('done'); + } + }; + }); + }); + inject(function(log, $compile) { + element = $compile('
')($rootScope); + $rootScope.$apply(); + expect(log).toEqual('done'); + }); + }); + + + it('explicit controller return values are ignored if they are primitives', function() { + module(function() { + directive('logControllerProp', function(log) { + return { + controller: function($scope) { + this.foo = 'baz'; // value *will* be used. + return 'bar'; + }, + link: function(scope, element, attrs, controller) { + log(controller.foo); } }; }); }); inject(function(log, $compile, $rootScope) { - element = $compile('
')($rootScope); - expect(log).toEqual('true; false'); + element = $compile('')($rootScope); + expect(log).toEqual('baz'); + expect(element.data('$logControllerPropController').foo).toEqual('baz'); }); }); - it('should get explicit return value of required parent controller', function() { + it('should get required parent controller', function() { module(function() { directive('nested', function(log) { return { require: '^^?nested', - controller: function() { - return {foo: 'bar'}; - }, + controller: function($scope) {}, link: function(scope, element, attrs, controller) { - log(!!controller && controller.foo); + log(!!controller); } }; }); }); inject(function(log, $compile, $rootScope) { element = $compile('
')($rootScope); - - expect(log).toEqual('bar; false'); + expect(log).toEqual('true; false'); }); }); @@ -4590,30 +4648,6 @@ describe('$compile', function() { }); - it('should respect explicit controller return value when using controllerAs', function() { - module(function() { - directive('main', function() { - return { - templateUrl: 'main.html', - transclude: true, - scope: {}, - controller: function() { - this.name = 'lucas'; - return {name: 'george'}; - }, - controllerAs: 'mainCtrl' - }; - }); - }); - inject(function($templateCache, $compile, $rootScope) { - $templateCache.put('main.html', 'template:{{mainCtrl.name}}
'); - element = $compile('
transclude:{{mainCtrl.name}}
')($rootScope); - $rootScope.$apply(); - expect(element.text()).toBe('template:george transclude:'); - }); - }); - - it('should support controller alias', function() { module(function($controllerProvider) { $controllerProvider.register('MainCtrl', function() { @@ -5615,41 +5649,6 @@ describe('$compile', function() { }); }); - it('should copy the explicit return value to all clones', function() { - module(function() { - directive('makeThree', valueFn({ - transclude: 'content', - controller: function($transclude) { - this.foo = 'baz'; - return {transclude:$transclude, foo: 'bar'}; - }, - link: function(scope, el, attr, ctrl) { - ctrl.transclude(cloneAttach); - ctrl.transclude(cloneAttach); - ctrl.transclude(cloneAttach); - - function cloneAttach(clone) { - el.append(clone); - } - } - })); - - directive('nested', function(log) { - return { - require: '^^makeThree', - link: function(scope, element, attrs, controller) { - log(controller.foo); - } - }; - }); - }); - inject(function(log, $compile) { - element = $compile('
')($rootScope); - $rootScope.$apply(); - expect(log).toEqual('bar; bar; bar'); - }); - }); - it('should provide the $transclude controller local as 5th argument to the pre and post-link function', function() { var ctrlTransclude, preLinkTransclude, postLinkTransclude; module(function() {