From ee11ced808108fb6cfcd4a07883c8fa75e3f0430 Mon Sep 17 00:00:00 2001 From: Caitlin Potter Date: Mon, 15 Dec 2014 12:30:19 -0500 Subject: [PATCH 1/4] feat($compile): allow using bindToController as object, support both new/isolate scopes bindToController is now able to be specified as a convenient object notation: ``` bindToController: { text: '@text', obj: '=obj', expr: '&expr' }, scope: {} ``` It can also be used in conjunction with new scopes, rather than exclusively isolate scopes: ``` bindToController: { text: '@text', obj: '=obj', expr: '&expr' }, scope: true ``` Closes #10420 --- docs/content/error/$compile/noctrl.ngdoc | 68 +++++++ src/.jshintrc | 3 + src/ng/compile.js | 246 +++++++++++++++-------- src/ng/controller.js | 15 +- test/ng/compileSpec.js | 174 ++++++++++++++++ 5 files changed, 416 insertions(+), 90 deletions(-) create mode 100644 docs/content/error/$compile/noctrl.ngdoc diff --git a/docs/content/error/$compile/noctrl.ngdoc b/docs/content/error/$compile/noctrl.ngdoc new file mode 100644 index 000000000000..ce35974fb96a --- /dev/null +++ b/docs/content/error/$compile/noctrl.ngdoc @@ -0,0 +1,68 @@ +@ngdoc error +@name $compile:noctrl +@fullName Controller is required. +@description + +When using the `bindToController` feature of AngularJS, a directive is required +to have a Controller, in addition to a controller identifier. + +For example, the following directives are valid: + +```js +// OKAY, because controller is a string with a label component. +directive("okay", function() { + return { + bindToController: true, + controller: "myCtrl as $ctrl" + scope: { + text: "@text" + } + }; +}); + + +// OKAY, because the directive uses the controllerAs property to override +// the controller identifier. +directive("okay2", function() { + return { + bindToController: true, + controllerAs: "$ctrl", + controller: function() { + + }, + scope: { + text: "@text" + } + }; +}); +``` + +While the following are invalid: + +```js +// BAD, because the controller property is a string with no identifier. +directive("bad", function() { + return { + bindToController: true, + controller: "unlabeledCtrl", + scope: { + text: "@text" + } + }; +}); + + +// BAD because the controller is not a string (therefore has no identifier), +// and there is no controllerAs property. +directive("bad2", function() { + return { + bindToController: true, + controller: function noControllerAs() { + + }, + scope: { + text: "@text" + } + }; +}); +``` diff --git a/src/.jshintrc b/src/.jshintrc index 16db88d8501b..d0561483c9ad 100644 --- a/src/.jshintrc +++ b/src/.jshintrc @@ -152,6 +152,9 @@ "urlResolve": false, "urlIsSameOrigin": false, + /* ng/controller.js */ + "identifierForController": false, + /* ng/compile.js */ "directiveNormalize": false, diff --git a/src/ng/compile.js b/src/ng/compile.js index bef8873816d1..f1a355f624ce 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -712,7 +712,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { // 'on' and be composed of only English letters. var EVENT_HANDLER_ATTR_REGEXP = /^(on[a-z]+|formaction)$/; - function parseIsolateBindings(scope, directiveName) { + function parseIsolateBindings(scope, directiveName, isController) { var LOCAL_REGEXP = /^\s*([@&]|=(\*?))(\??)\s*(\w*)\s*$/; var bindings = {}; @@ -722,9 +722,11 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { if (!match) { throw $compileMinErr('iscp', - "Invalid isolate scope definition for directive '{0}'." + + "Invalid {3} for directive '{0}'." + " Definition: {... {1}: '{2}' ...}", - directiveName, scopeName, definition); + directiveName, scopeName, definition, + (isController ? "controller bindings definition" : + "isolate scope definition")); } bindings[scopeName] = { @@ -738,6 +740,37 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { return bindings; } + function parseDirectiveBindings(directive, directiveName) { + var bindings = { + isolateScope: null, + bindToController: null + }; + if (isObject(directive.scope)) { + if (directive.bindToController === true) { + bindings.bindToController = parseIsolateBindings(directive.scope, + directiveName, true); + bindings.isolateScope = {}; + } else { + bindings.isolateScope = parseIsolateBindings(directive.scope, + directiveName, false); + } + } + if (isObject(directive.bindToController)) { + bindings.bindToController = + parseIsolateBindings(directive.bindToController, directiveName, true); + } + if (isObject(bindings.bindToController)) { + var controller = directive.controller; + var controllerAs = directive.controllerAs; + if (!directive.controller || !identifierForController(controller, controllerAs)) { + throw $compileMinErr('noctrl', + "Cannot bind to controller without directive '{0}'s controller.", + directiveName); + } + } + return bindings; + } + /** * @ngdoc method * @name $compileProvider#directive @@ -775,8 +808,10 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { directive.name = directive.name || name; directive.require = directive.require || (directive.controller && directive.name); directive.restrict = directive.restrict || 'EA'; - if (isObject(directive.scope)) { - directive.$$isolateBindings = parseIsolateBindings(directive.scope, directive.name); + var bindings = directive.$$bindings = + parseDirectiveBindings(directive, directive.name); + if (isObject(bindings.isolateScope)) { + directive.$$isolateBindings = bindings.isolateScope; } directives.push(directive); } catch (e) { @@ -1338,6 +1373,16 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { if (nodeLinkFn.scope) { childScope = scope.$new(); compile.$$addScopeInfo(jqLite(node), childScope); + var onDestroyed = nodeLinkFn.$$onScopeDestroyed; + if (onDestroyed) { + nodeLinkFn.$$onScopeDestroyed = null; + childScope.$on('$destroyed', function() { + for (var i=0, ii = onDestroyed.length; i < ii; ++i) { + onDestroyed[i](); + } + onDestroyed = null; + }); + } } else { childScope = scope; } @@ -1357,7 +1402,8 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { childBoundTranscludeFn = null; } - nodeLinkFn(childLinkFn, childScope, node, $rootElement, childBoundTranscludeFn); + nodeLinkFn(childLinkFn, childScope, node, $rootElement, childBoundTranscludeFn, + nodeLinkFn); } else if (childLinkFn) { childLinkFn(scope, node.childNodes, undefined, parentBoundTranscludeFn); @@ -1838,7 +1884,8 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { } - function nodeLinkFn(childLinkFn, scope, linkNode, $rootElement, boundTranscludeFn) { + function nodeLinkFn(childLinkFn, scope, linkNode, $rootElement, boundTranscludeFn, + thisLinkFn) { var i, ii, linkFn, controller, isolateScope, elementControllers, transcludeFn, $element, attrs; @@ -1895,89 +1942,29 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { } if (newIsolateScopeDirective) { + // Initialize isolate scope bindings for new isolate scope directive. compile.$$addScopeInfo($element, isolateScope, true, !(templateDirective && (templateDirective === newIsolateScopeDirective || templateDirective === newIsolateScopeDirective.$$originalDirective))); compile.$$addScopeClass($element, true); - - var isolateScopeController = controllers && controllers[newIsolateScopeDirective.name]; - var isolateBindingContext = isolateScope; - if (isolateScopeController && isolateScopeController.identifier && - newIsolateScopeDirective.bindToController === true) { - isolateBindingContext = isolateScopeController.instance; - } - - forEach(isolateScope.$$isolateBindings = newIsolateScopeDirective.$$isolateBindings, function(definition, scopeName) { - var attrName = definition.attrName, - optional = definition.optional, - mode = definition.mode, // @, =, or & - lastValue, - parentGet, parentSet, compare; - - switch (mode) { - - case '@': - attrs.$observe(attrName, function(value) { - isolateBindingContext[scopeName] = value; - }); - 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 - isolateBindingContext[scopeName] = $interpolate(attrs[attrName])(scope); - } - break; - - case '=': - if (optional && !attrs[attrName]) { - return; - } - parentGet = $parse(attrs[attrName]); - if (parentGet.literal) { - compare = equals; - } else { - compare = function(a, b) { return a === b || (a !== a && b !== b); }; - } - parentSet = parentGet.assign || function() { - // reset the change, or we will throw this exception on every $digest - lastValue = isolateBindingContext[scopeName] = parentGet(scope); - throw $compileMinErr('nonassign', - "Expression '{0}' used with directive '{1}' is non-assignable!", - attrs[attrName], newIsolateScopeDirective.name); - }; - lastValue = isolateBindingContext[scopeName] = parentGet(scope); - var parentValueWatch = function parentValueWatch(parentValue) { - if (!compare(parentValue, isolateBindingContext[scopeName])) { - // we are out of sync and need to copy - if (!compare(parentValue, lastValue)) { - // parent changed and it has precedence - isolateBindingContext[scopeName] = parentValue; - } else { - // if the parent can be assigned then do so - parentSet(scope, parentValue = isolateBindingContext[scopeName]); - } - } - return lastValue = parentValue; - }; - parentValueWatch.$stateful = true; - var unwatch; - if (definition.collection) { - unwatch = scope.$watchCollection(attrs[attrName], parentValueWatch); - } else { - unwatch = scope.$watch($parse(attrs[attrName], parentValueWatch), null, parentGet.literal); - } - isolateScope.$on('$destroy', unwatch); - break; - - case '&': - parentGet = $parse(attrs[attrName]); - isolateBindingContext[scopeName] = function(locals) { - return parentGet(scope, locals); - }; - break; - } - }); + isolateScope.$$isolateBindings = + newIsolateScopeDirective.$$isolateBindings; + initializeDirectiveBindings(scope, attrs, isolateScope, + isolateScope.$$isolateBindings, + newIsolateScopeDirective, isolateScope); } if (controllers) { + // Initialize bindToController bindings for new/isolate scopes + var scopeDirective = newIsolateScopeDirective || newScopeDirective; + if (scopeDirective && controllers[scopeDirective.name]) { + var bindings = scopeDirective.$$bindings.bindToController; + controller = controllers[scopeDirective.name]; + + if (controller && controller.identifier && bindings) { + thisLinkFn.$$onScopeDestroyed = + initializeDirectiveBindings(scope, attrs, controller.instance, + bindings, scopeDirective); + } + } forEach(controllers, function(controller) { controller(); }); @@ -2240,7 +2227,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { childBoundTranscludeFn = boundTranscludeFn; } afterTemplateNodeLinkFn(afterTemplateChildLinkFn, scope, linkNode, $rootElement, - childBoundTranscludeFn); + childBoundTranscludeFn, afterTemplateNodeLinkFn); } linkQueue = null; }); @@ -2257,7 +2244,8 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { if (afterTemplateNodeLinkFn.transcludeOnThisElement) { childBoundTranscludeFn = createBoundTranscludeFn(scope, afterTemplateNodeLinkFn.transclude, boundTranscludeFn); } - afterTemplateNodeLinkFn(afterTemplateChildLinkFn, scope, node, rootElement, childBoundTranscludeFn); + afterTemplateNodeLinkFn(afterTemplateChildLinkFn, scope, node, rootElement, childBoundTranscludeFn, + afterTemplateNodeLinkFn); } }; } @@ -2504,6 +2492,90 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { $exceptionHandler(e, startingTag($element)); } } + + + // Set up $watches for isolate scope and controller bindings. This process + // only occurs for isolate scopes and new scopes with controllerAs. + function initializeDirectiveBindings(scope, attrs, destination, bindings, + directive, newScope) { + var onNewScopeDestroyed; + forEach(bindings, function(definition, scopeName) { + var attrName = definition.attrName, + optional = definition.optional, + mode = definition.mode, // @, =, or & + lastValue, + parentGet, parentSet, compare; + + switch (mode) { + + case '@': + attrs.$observe(attrName, function(value) { + destination[scopeName] = value; + }); + 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 + destination[scopeName] = $interpolate(attrs[attrName])(scope); + } + break; + + case '=': + if (optional && !attrs[attrName]) { + return; + } + parentGet = $parse(attrs[attrName]); + if (parentGet.literal) { + compare = equals; + } else { + compare = function(a, b) { return a === b || (a !== a && b !== b); }; + } + parentSet = parentGet.assign || function() { + // reset the change, or we will throw this exception on every $digest + lastValue = destination[scopeName] = parentGet(scope); + throw $compileMinErr('nonassign', + "Expression '{0}' used with directive '{1}' is non-assignable!", + attrs[attrName], directive.name); + }; + lastValue = destination[scopeName] = parentGet(scope); + var parentValueWatch = function parentValueWatch(parentValue) { + if (!compare(parentValue, destination[scopeName])) { + // we are out of sync and need to copy + if (!compare(parentValue, lastValue)) { + // parent changed and it has precedence + destination[scopeName] = parentValue; + } else { + // if the parent can be assigned then do so + parentSet(scope, parentValue = destination[scopeName]); + } + } + return lastValue = parentValue; + }; + parentValueWatch.$stateful = true; + var unwatch; + if (definition.collection) { + unwatch = scope.$watchCollection(attrs[attrName], parentValueWatch); + } else { + unwatch = scope.$watch($parse(attrs[attrName], parentValueWatch), null, parentGet.literal); + } + if (newScope) { + newScope.$on('$destroy', unwatch); + } else { + onNewScopeDestroyed = (onNewScopeDestroyed || []); + onNewScopeDestroyed.push(unwatch); + } + break; + + case '&': + parentGet = $parse(attrs[attrName]); + destination[scopeName] = function(locals) { + return parentGet(scope, locals); + }; + break; + } + }); + return onNewScopeDestroyed; + } }]; } diff --git a/src/ng/controller.js b/src/ng/controller.js index cf7e964c2b14..863eb87c9a4c 100644 --- a/src/ng/controller.js +++ b/src/ng/controller.js @@ -2,6 +2,17 @@ var $controllerMinErr = minErr('$controller'); + +var CNTRL_REG = /^(\S+)(\s+as\s+(\w+))?$/; +function identifierForController(controller, ident) { + if (ident && isString(ident)) return ident; + if (isString(controller)) { + var match = CNTRL_REG.exec(controller); + if (match) return match[3]; + } +} + + /** * @ngdoc provider * @name $controllerProvider @@ -14,9 +25,7 @@ var $controllerMinErr = minErr('$controller'); */ function $ControllerProvider() { var controllers = {}, - globals = false, - CNTRL_REG = /^(\S+)(\s+as\s+(\w+))?$/; - + globals = false; /** * @ngdoc method diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index 5df9bddb79c5..aebd8ab6ebee 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -3809,6 +3809,180 @@ describe('$compile', function() { expect(controllerCalled).toBe(true); }); }); + + + it('should throw noctrl when missing controller', function() { + module(function($compileProvider) { + $compileProvider.directive('noCtrl', valueFn({ + templateUrl: 'test.html', + scope: { + 'data': '=dirData', + 'str': '@dirStr', + 'fn': '&dirFn' + }, + controllerAs: 'test', + bindToController: true + })); + }); + inject(function($compile, $rootScope) { + expect(function() { + $compile('
')($rootScope); + }).toThrowMinErr('$compile', 'noctrl', + 'Cannot bind to controller without directive \'noCtrl\'s controller.'); + }); + }); + + + it('should throw noctrl when missing controllerAs label', function() { + module(function($compileProvider) { + $compileProvider.directive('noCtrl', valueFn({ + templateUrl: 'test.html', + scope: { + 'data': '=dirData', + 'str': '@dirStr', + 'fn': '&dirFn' + }, + controller: function() {}, + bindToController: true + })); + }); + inject(function($compile, $rootScope) { + expect(function() { + $compile('
')($rootScope); + }).toThrowMinErr('$compile', 'noctrl', + 'Cannot bind to controller without directive \'noCtrl\'s controller.'); + }); + }); + + + it('should throw noctrl when missing controller label', function() { + module(function($compileProvider, $controllerProvider) { + $controllerProvider.register('myCtrl', function() {}); + $compileProvider.directive('noCtrl', valueFn({ + templateUrl: 'test.html', + scope: { + 'data': '=dirData', + 'str': '@dirStr', + 'fn': '&dirFn' + }, + controller: 'myCtrl', + bindToController: true + })); + }); + inject(function($compile, $rootScope) { + expect(function() { + $compile('
')($rootScope); + }).toThrowMinErr('$compile', 'noctrl', + 'Cannot bind to controller without directive \'noCtrl\'s controller.'); + }); + }); + + + it('should bind to controller via object notation (isolate scope)', function() { + var controllerCalled = 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; + }); + $compileProvider.directive('fooDir', valueFn({ + templateUrl: 'test.html', + bindToController: { + 'data': '=dirData', + 'str': '@dirStr', + 'fn': '&dirFn' + }, + scope: {}, + controller: 'myCtrl as myCtrl' + })); + }); + inject(function($compile, $rootScope, $templateCache) { + $templateCache.put('test.html', '

isolate

'); + $rootScope.fn = valueFn('called!'); + $rootScope.whom = 'world'; + $rootScope.remoteData = { + 'foo': 'bar', + 'baz': 'biz' + }; + element = $compile('
')($rootScope); + $rootScope.$digest(); + expect(controllerCalled).toBe(true); + }); + }); + + + it('should bind to controller via object notation (new scope)', function() { + var controllerCalled = 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; + }); + $compileProvider.directive('fooDir', valueFn({ + templateUrl: 'test.html', + bindToController: { + 'data': '=dirData', + 'str': '@dirStr', + 'fn': '&dirFn' + }, + scope: true, + controller: 'myCtrl as myCtrl' + })); + }); + inject(function($compile, $rootScope, $templateCache) { + $templateCache.put('test.html', '

isolate

'); + $rootScope.fn = valueFn('called!'); + $rootScope.whom = 'world'; + $rootScope.remoteData = { + 'foo': 'bar', + 'baz': 'biz' + }; + element = $compile('
')($rootScope); + $rootScope.$digest(); + expect(controllerCalled).toBe(true); + }); + }); + + + it('should put controller in scope when labelled but not using controllerAs', function() { + var controllerCalled = false; + var myCtrl; + module(function($compileProvider, $controllerProvider) { + $controllerProvider.register('myCtrl', function() { + controllerCalled = true; + myCtrl = this; + }); + $compileProvider.directive('fooDir', valueFn({ + templateUrl: 'test.html', + bindToController: {}, + scope: true, + controller: 'myCtrl as theCtrl' + })); + }); + inject(function($compile, $rootScope, $templateCache) { + $templateCache.put('test.html', '

isolate

'); + element = $compile('
')($rootScope); + $rootScope.$digest(); + expect(controllerCalled).toBe(true); + var childScope = element.children().scope(); + expect(childScope).not.toBe($rootScope); + expect(childScope.theCtrl).toBe(myCtrl); + }); + }); }); From 4916d27d2e46fb691ce58176c7a25f8b15ce23a2 Mon Sep 17 00:00:00 2001 From: Caitlin Potter Date: Wed, 17 Dec 2014 10:05:11 -0500 Subject: [PATCH 2/4] fix($compile): respect return value from controller constructor The return value of the controller constructor is now respected in all cases. If controllerAs is used, the controller will be re-bound to scope. If bindToController is used, the previous binding $watches (if any) will be unwatched, and bindings re-installed on the new controller. --- src/ng/compile.js | 49 ++++++++++------- src/ng/controller.js | 12 ++++- test/ng/compileSpec.js | 116 +++++++++++++++++++++++++++++++++++++++-- 3 files changed, 152 insertions(+), 25 deletions(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index f1a355f624ce..8783c44b8f4b 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -1373,15 +1373,10 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { if (nodeLinkFn.scope) { childScope = scope.$new(); compile.$$addScopeInfo(jqLite(node), childScope); - var onDestroyed = nodeLinkFn.$$onScopeDestroyed; - if (onDestroyed) { - nodeLinkFn.$$onScopeDestroyed = null; - childScope.$on('$destroyed', function() { - for (var i=0, ii = onDestroyed.length; i < ii; ++i) { - onDestroyed[i](); - } - onDestroyed = null; - }); + var destroyBindings = nodeLinkFn.$$destroyBindings; + if (destroyBindings) { + nodeLinkFn.$$destroyBindings = null; + childScope.$on('$destroyed', destroyBindings); } } else { childScope = scope; @@ -1955,18 +1950,29 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { if (controllers) { // Initialize bindToController bindings for new/isolate scopes var scopeDirective = newIsolateScopeDirective || newScopeDirective; + var bindings; + var controllerForBindings; if (scopeDirective && controllers[scopeDirective.name]) { - var bindings = scopeDirective.$$bindings.bindToController; + bindings = scopeDirective.$$bindings.bindToController; controller = controllers[scopeDirective.name]; if (controller && controller.identifier && bindings) { - thisLinkFn.$$onScopeDestroyed = + controllerForBindings = controller; + thisLinkFn.$$destroyBindings = initializeDirectiveBindings(scope, attrs, controller.instance, bindings, scopeDirective); } } forEach(controllers, function(controller) { - controller(); + var result = controller(); + if (result !== controller.instance && + controller === controllerForBindings) { + // Remove and re-install bindToController bindings + thisLinkFn.$$destroyBindings(); + thisLinkFn.$$destroyBindings = + initializeDirectiveBindings(scope, attrs, result, + bindings, scopeDirective); + } }); controllers = null; } @@ -2558,12 +2564,8 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { } else { unwatch = scope.$watch($parse(attrs[attrName], parentValueWatch), null, parentGet.literal); } - if (newScope) { - newScope.$on('$destroy', unwatch); - } else { - onNewScopeDestroyed = (onNewScopeDestroyed || []); - onNewScopeDestroyed.push(unwatch); - } + onNewScopeDestroyed = (onNewScopeDestroyed || []); + onNewScopeDestroyed.push(unwatch); break; case '&': @@ -2574,7 +2576,16 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { break; } }); - return onNewScopeDestroyed; + var destroyBindings = onNewScopeDestroyed ? function destroyBindings() { + for (var i = 0, ii = onNewScopeDestroyed.length; i < ii; ++i) { + onNewScopeDestroyed[i](); + } + } : noop; + if (newScope && destroyBindings !== noop) { + newScope.$on('$destroy', destroyBindings); + return noop; + } + return destroyBindings; } }]; } diff --git a/src/ng/controller.js b/src/ng/controller.js index 863eb87c9a4c..130142091573 100644 --- a/src/ng/controller.js +++ b/src/ng/controller.js @@ -133,8 +133,16 @@ function $ControllerProvider() { addIdentifier(locals, identifier, instance, constructor || expression.name); } - return extend(function() { - $injector.invoke(expression, instance, locals, constructor); + var instantiate; + return instantiate = extend(function() { + var result = $injector.invoke(expression, instance, locals, constructor); + if (result !== instance && (isObject(result) || isFunction(result))) { + instance = result; + if (identifier) { + // If result changed, re-assign controllerAs value to scope. + addIdentifier(locals, identifier, instance, constructor || expression.name); + } + } return instance; }, { instance: instance, diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index aebd8ab6ebee..87a15d10d7c7 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -3910,8 +3910,8 @@ describe('$compile', function() { 'baz': 'biz' }; element = $compile('
')($rootScope); + 'dir-str="Hello, {{whom}}!" ' + + 'dir-fn="fn()">
')($rootScope); $rootScope.$digest(); expect(controllerCalled).toBe(true); }); @@ -3950,8 +3950,8 @@ describe('$compile', function() { 'baz': 'biz' }; element = $compile('
')($rootScope); + 'dir-str="Hello, {{whom}}!" ' + + 'dir-fn="fn()">
')($rootScope); $rootScope.$digest(); expect(controllerCalled).toBe(true); }); @@ -3983,6 +3983,114 @@ describe('$compile', function() { expect(childScope.theCtrl).toBe(myCtrl); }); }); + + + it('should re-install controllerAs and bindings for returned value from controller (new scope)', function() { + var controllerCalled = false; + var myCtrl; + + function MyCtrl() { + } + MyCtrl.prototype.test = function() { + expect(this.data).toEqualData({ + 'foo': 'bar', + 'baz': 'biz' + }); + expect(this.str).toBe('Hello, world!'); + expect(this.fn()).toBe('called!'); + } + + module(function($compileProvider, $controllerProvider) { + $controllerProvider.register('myCtrl', function() { + controllerCalled = true; + myCtrl = this; + return new MyCtrl(); + }); + $compileProvider.directive('fooDir', valueFn({ + templateUrl: 'test.html', + bindToController: { + 'data': '=dirData', + 'str': '@dirStr', + 'fn': '&dirFn' + }, + scope: true, + controller: 'myCtrl as theCtrl' + })); + }); + inject(function($compile, $rootScope, $templateCache) { + $templateCache.put('test.html', '

isolate

'); + $rootScope.fn = valueFn('called!'); + $rootScope.whom = 'world'; + $rootScope.remoteData = { + 'foo': 'bar', + 'baz': 'biz' + }; + element = $compile('
')($rootScope); + $rootScope.$digest(); + expect(controllerCalled).toBe(true); + var childScope = element.children().scope(); + expect(childScope).not.toBe($rootScope); + expect(childScope.theCtrl).not.toBe(myCtrl); + expect(childScope.theCtrl.constructor).toBe(MyCtrl); + childScope.theCtrl.test(); + }); + }); + + + it('should re-install controllerAs and bindings for returned value from controller (isolate scope)', function() { + var controllerCalled = false; + var myCtrl; + + function MyCtrl() { + } + MyCtrl.prototype.test = function() { + expect(this.data).toEqualData({ + 'foo': 'bar', + 'baz': 'biz' + }); + expect(this.str).toBe('Hello, world!'); + expect(this.fn()).toBe('called!'); + } + + module(function($compileProvider, $controllerProvider) { + $controllerProvider.register('myCtrl', function() { + controllerCalled = true; + myCtrl = this; + return new MyCtrl(); + }); + $compileProvider.directive('fooDir', valueFn({ + templateUrl: 'test.html', + bindToController: true, + scope: { + 'data': '=dirData', + 'str': '@dirStr', + 'fn': '&dirFn' + }, + controller: 'myCtrl as theCtrl' + })); + }); + inject(function($compile, $rootScope, $templateCache) { + $templateCache.put('test.html', '

isolate

'); + $rootScope.fn = valueFn('called!'); + $rootScope.whom = 'world'; + $rootScope.remoteData = { + 'foo': 'bar', + 'baz': 'biz' + }; + element = $compile('
')($rootScope); + $rootScope.$digest(); + expect(controllerCalled).toBe(true); + var childScope = element.children().scope(); + expect(childScope).not.toBe($rootScope); + expect(childScope.theCtrl).not.toBe(myCtrl); + expect(childScope.theCtrl.constructor).toBe(MyCtrl); + childScope.theCtrl.test(); + }); + }); }); From 4ed70afa16ec8928bbb3e5bdac305e90ea30ad1f Mon Sep 17 00:00:00 2001 From: Caitlin Potter Date: Mon, 19 Jan 2015 13:27:20 -0500 Subject: [PATCH 3/4] docs($compile): create new error for "missing controller identifier" --- docs/content/error/$compile/noctrl.ngdoc | 66 ++------------------- docs/content/error/$compile/noident.ngdoc | 71 +++++++++++++++++++++++ src/ng/compile.js | 8 ++- test/ng/compileSpec.js | 26 ++++----- 4 files changed, 96 insertions(+), 75 deletions(-) create mode 100644 docs/content/error/$compile/noident.ngdoc diff --git a/docs/content/error/$compile/noctrl.ngdoc b/docs/content/error/$compile/noctrl.ngdoc index ce35974fb96a..ddbe15838081 100644 --- a/docs/content/error/$compile/noctrl.ngdoc +++ b/docs/content/error/$compile/noctrl.ngdoc @@ -4,65 +4,9 @@ @description When using the `bindToController` feature of AngularJS, a directive is required -to have a Controller, in addition to a controller identifier. +to have a Controller. A controller may be specified by adding a "controller" +property to the directive definition object. Its value should be either a +string, or an invokable object (a function, or an array whose last element is a +function). -For example, the following directives are valid: - -```js -// OKAY, because controller is a string with a label component. -directive("okay", function() { - return { - bindToController: true, - controller: "myCtrl as $ctrl" - scope: { - text: "@text" - } - }; -}); - - -// OKAY, because the directive uses the controllerAs property to override -// the controller identifier. -directive("okay2", function() { - return { - bindToController: true, - controllerAs: "$ctrl", - controller: function() { - - }, - scope: { - text: "@text" - } - }; -}); -``` - -While the following are invalid: - -```js -// BAD, because the controller property is a string with no identifier. -directive("bad", function() { - return { - bindToController: true, - controller: "unlabeledCtrl", - scope: { - text: "@text" - } - }; -}); - - -// BAD because the controller is not a string (therefore has no identifier), -// and there is no controllerAs property. -directive("bad2", function() { - return { - bindToController: true, - controller: function noControllerAs() { - - }, - scope: { - text: "@text" - } - }; -}); -``` +For more information, see the {@link guide/directive directives guide}. diff --git a/docs/content/error/$compile/noident.ngdoc b/docs/content/error/$compile/noident.ngdoc new file mode 100644 index 000000000000..428629b5d7be --- /dev/null +++ b/docs/content/error/$compile/noident.ngdoc @@ -0,0 +1,71 @@ +@ngdoc error +@name $compile:noident +@fullName Controller identifier is required. +@description + +When using the `bindToController` feature of AngularJS, a directive is required +to have a Controller identifier, which is initialized in scope with the value of +the controller instance. This can be supplied using the "controllerAs" property +of the directive object, or alternatively by adding " as IDENTIFIER" to the controller +name. + +For example, the following directives are valid: + +```js +// OKAY, because controller is a string with an identifier component. +directive("okay", function() { + return { + bindToController: true, + controller: "myCtrl as $ctrl" + scope: { + text: "@text" + } + }; +}); + + +// OKAY, because the directive uses the controllerAs property to override +// the controller identifier. +directive("okay2", function() { + return { + bindToController: true, + controllerAs: "$ctrl", + controller: function() { + + }, + scope: { + text: "@text" + } + }; +}); +``` + +While the following are invalid: + +```js +// BAD, because the controller property is a string with no identifier. +directive("bad", function() { + return { + bindToController: true, + controller: "noIdentCtrl", + scope: { + text: "@text" + } + }; +}); + + +// BAD because the controller is not a string (therefore has no identifier), +// and there is no controllerAs property. +directive("bad2", function() { + return { + bindToController: true, + controller: function noControllerAs() { + + }, + scope: { + text: "@text" + } + }; +}); +``` diff --git a/src/ng/compile.js b/src/ng/compile.js index 8783c44b8f4b..26ab57ccf820 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -762,10 +762,16 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { if (isObject(bindings.bindToController)) { var controller = directive.controller; var controllerAs = directive.controllerAs; - if (!directive.controller || !identifierForController(controller, controllerAs)) { + if (!controller) { + // There is no controller, there may or may not be a controllerAs property throw $compileMinErr('noctrl', "Cannot bind to controller without directive '{0}'s controller.", directiveName); + } else if (!identifierForController(controller, controllerAs)) { + // There is a controller, but no identifier or controllerAs property + throw $compileMinErr('noident', + "Cannot bind to controller without identifier for directive '{0}'.", + directiveName); } } return bindings; diff --git a/test/ng/compileSpec.js b/test/ng/compileSpec.js index 87a15d10d7c7..5651647a75bc 100755 --- a/test/ng/compileSpec.js +++ b/test/ng/compileSpec.js @@ -3833,9 +3833,9 @@ describe('$compile', function() { }); - it('should throw noctrl when missing controllerAs label', function() { + it('should throw noident when missing controllerAs directive property', function() { module(function($compileProvider) { - $compileProvider.directive('noCtrl', valueFn({ + $compileProvider.directive('noIdent', valueFn({ templateUrl: 'test.html', scope: { 'data': '=dirData', @@ -3848,17 +3848,17 @@ describe('$compile', function() { }); inject(function($compile, $rootScope) { expect(function() { - $compile('
')($rootScope); - }).toThrowMinErr('$compile', 'noctrl', - 'Cannot bind to controller without directive \'noCtrl\'s controller.'); + $compile('
')($rootScope); + }).toThrowMinErr('$compile', 'noident', + 'Cannot bind to controller without identifier for directive \'noIdent\'.'); }); }); - it('should throw noctrl when missing controller label', function() { + it('should throw noident when missing controller identifier', function() { module(function($compileProvider, $controllerProvider) { $controllerProvider.register('myCtrl', function() {}); - $compileProvider.directive('noCtrl', valueFn({ + $compileProvider.directive('noIdent', valueFn({ templateUrl: 'test.html', scope: { 'data': '=dirData', @@ -3871,9 +3871,9 @@ describe('$compile', function() { }); inject(function($compile, $rootScope) { expect(function() { - $compile('
')($rootScope); - }).toThrowMinErr('$compile', 'noctrl', - 'Cannot bind to controller without directive \'noCtrl\'s controller.'); + $compile('
')($rootScope); + }).toThrowMinErr('$compile', 'noident', + 'Cannot bind to controller without identifier for directive \'noIdent\'.'); }); }); @@ -3958,7 +3958,7 @@ describe('$compile', function() { }); - it('should put controller in scope when labelled but not using controllerAs', function() { + it('should put controller in scope when controller identifier present but not using controllerAs', function() { var controllerCalled = false; var myCtrl; module(function($compileProvider, $controllerProvider) { @@ -3998,7 +3998,7 @@ describe('$compile', function() { }); expect(this.str).toBe('Hello, world!'); expect(this.fn()).toBe('called!'); - } + }; module(function($compileProvider, $controllerProvider) { $controllerProvider.register('myCtrl', function() { @@ -4052,7 +4052,7 @@ describe('$compile', function() { }); expect(this.str).toBe('Hello, world!'); expect(this.fn()).toBe('called!'); - } + }; module(function($compileProvider, $controllerProvider) { $controllerProvider.register('myCtrl', function() { From fcaca73114226506ac517169d29579d6b612faf6 Mon Sep 17 00:00:00 2001 From: Caitlin Potter Date: Thu, 29 Jan 2015 13:16:31 -0500 Subject: [PATCH 4/4] docs($compile,$route): reword "controller alias" to clarify Clarify that these aliases are identifier names used to reference the controller. --- src/ng/compile.js | 9 +++++---- src/ngRoute/route.js | 4 ++-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/ng/compile.js b/src/ng/compile.js index 26ab57ccf820..ae5b3cf9e2d3 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -64,7 +64,7 @@ * templateNamespace: 'html', * scope: false, * controller: function($scope, $element, $attrs, $transclude, otherInjectables) { ... }, - * controllerAs: 'stringAlias', + * controllerAs: 'stringIdentifier', * require: 'siblingDirectiveName', // or // ['^parentDirectiveName', '?optionalDirectiveName', '?^optionalParent'], * compile: function compile(tElement, tAttrs, transclude) { * return { @@ -224,9 +224,10 @@ * * * #### `controllerAs` - * Controller alias at the directive scope. An alias for the controller so it - * can be referenced at the directive template. The directive needs to define a scope for this - * configuration to be used. Useful in the case when directive is used as component. + * Identifier name for a reference to the controller in the directive's scope. + * This allows the controller to be referenced from the directive template. The directive + * needs to define a scope for this configuration to be used. Useful in the case when + * directive is used as component. * * * #### `restrict` diff --git a/src/ngRoute/route.js b/src/ngRoute/route.js index dfb51684db7c..3a425552abb9 100644 --- a/src/ngRoute/route.js +++ b/src/ngRoute/route.js @@ -73,8 +73,8 @@ function $RouteProvider() { * - `controller` – `{(string|function()=}` – Controller fn that should be associated with * newly created scope or the name of a {@link angular.Module#controller registered * controller} if passed as a string. - * - `controllerAs` – `{string=}` – A controller alias name. If present the controller will be - * published to scope under the `controllerAs` name. + * - `controllerAs` – `{string=}` – An identifier name for a reference to the controller. + * If present, the controller will be published to scope under the `controllerAs` name. * - `template` – `{string=|function()=}` – html template as a string or a function that * returns an html template as a string which should be used by {@link * ngRoute.directive:ngView ngView} or {@link ng.directive:ngInclude ngInclude} directives.