From 2ee9dc206ec624373423d00d3b1d537ef6b6422e Mon Sep 17 00:00:00 2001 From: Georgii Dolzhykov Date: Sat, 26 May 2018 14:13:00 +0300 Subject: [PATCH] refactor($compile): remove preAssignBindingsEnabled leftovers Now that we don't need to support `preAssignBindingsEnabled` (removed in #15782), complexity introduced in `$controller` by #7645 can be removed. One difference with the previous implementation is that all non-ES2015-class controller instances were available on the element before calling their constructors. Now it depends on the relative order of controllers. Controller constructors shouldn't be used to access other controllers (e.g. via `$element.controller(directiveName)`). The recommended way is to use the `require` property of the directive definition object and the life cycle hooks `$onChanges` or `$onInit`. See https://docs.angularjs.org/api/ng/service/$compile#-require- https://docs.angularjs.org/api/ng/service/$compile#life-cycle-hooks --- src/auto/injector.js | 30 ++------- src/ng/compile.js | 130 ++++++++++++++++-------------------- src/ng/controller.js | 42 +----------- src/ngMock/angular-mocks.js | 13 ++-- test/auto/injectorSpec.js | 19 ------ 5 files changed, 67 insertions(+), 167 deletions(-) diff --git a/src/auto/injector.js b/src/auto/injector.js index b4995af8f6f6..c1cea832acde 100644 --- a/src/auto/injector.js +++ b/src/auto/injector.js @@ -70,12 +70,8 @@ var FN_ARG = /^\s*(_?)(\S+?)\1\s*$/; var STRIP_COMMENTS = /((\/\/.*$)|(\/\*[\s\S]*?\*\/))/mg; var $injectorMinErr = minErr('$injector'); -function stringifyFn(fn) { - return Function.prototype.toString.call(fn); -} - function extractArgs(fn) { - var fnText = stringifyFn(fn).replace(STRIP_COMMENTS, ''), + var fnText = Function.prototype.toString.call(fn).replace(STRIP_COMMENTS, ''), args = fnText.match(ARROW_ARG) || fnText.match(FN_ARGS); return args; } @@ -914,19 +910,6 @@ function createInjector(modulesToLoad, strictDi) { return args; } - function isClass(func) { - // Support: IE 9-11 only - // IE 9-11 do not support classes and IE9 leaks with the code below. - if (msie || typeof func !== 'function') { - return false; - } - var result = func.$$ngIsClass; - if (!isBoolean(result)) { - result = func.$$ngIsClass = /^class\b/.test(stringifyFn(func)); - } - return result; - } - function invoke(fn, self, locals, serviceName) { if (typeof locals === 'string') { serviceName = locals; @@ -938,14 +921,9 @@ function createInjector(modulesToLoad, strictDi) { fn = fn[fn.length - 1]; } - if (!isClass(fn)) { - // http://jsperf.com/angularjs-invoke-apply-vs-switch - // #5388 - return fn.apply(self, args); - } else { - args.unshift(null); - return new (Function.prototype.bind.apply(fn, args))(); - } + // http://jsperf.com/angularjs-invoke-apply-vs-switch + // #5388 + return fn.apply(self, args); } diff --git a/src/ng/compile.js b/src/ng/compile.js index e8bd8ca01668..5dd5e4ff90da 100644 --- a/src/ng/compile.js +++ b/src/ng/compile.js @@ -2788,10 +2788,6 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { }; } - if (controllerDirectives) { - elementControllers = setupControllers($element, attrs, transcludeFn, controllerDirectives, isolateScope, scope, newIsolateScopeDirective); - } - if (newIsolateScopeDirective) { // Initialize isolate scope bindings for new isolate scope directive. compile.$$addScopeInfo($element, isolateScope, true, !(templateDirective && (templateDirective === newIsolateScopeDirective || @@ -2807,53 +2803,69 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { } } - // Initialize bindToController bindings - for (var name in elementControllers) { - var controllerDirective = controllerDirectives[name]; - var controller = elementControllers[name]; - var bindings = controllerDirective.$$bindings.bindToController; + if (controllerDirectives) { + elementControllers = createMap(); + for (var name in controllerDirectives) { + var directive = controllerDirectives[name]; + var locals = { + $scope: directive === newIsolateScopeDirective || directive.$$isolateScope ? isolateScope : scope, + $element: $element, + $attrs: attrs, + $transclude: transcludeFn + }; - controller.instance = controller(); - $element.data('$' + controllerDirective.name + 'Controller', controller.instance); - controller.bindingInfo = - initializeDirectiveBindings(controllerScope, attrs, controller.instance, bindings, controllerDirective); - } + var controllerConstructor = directive.controller; + if (controllerConstructor === '@') { + controllerConstructor = attrs[name]; + } - // Bind the required controllers to the controller, if `require` is an object and `bindToController` is truthy - forEach(controllerDirectives, function(controllerDirective, name) { - var require = controllerDirective.require; - if (controllerDirective.bindToController && !isArray(require) && isObject(require)) { - extend(elementControllers[name].instance, getControllers(name, require, $element, elementControllers)); + var instance = $controller(controllerConstructor, locals, directive.controllerAs); + + $element.data('$' + name + 'Controller', instance); + + // Initialize bindToController bindings + var bindings = directive.$$bindings.bindToController; + var bindingInfo = initializeDirectiveBindings(controllerScope, attrs, instance, bindings, directive); + + elementControllers[name] = { instance: instance, bindingInfo: bindingInfo }; } - }); - // Handle the init and destroy lifecycle hooks on all controllers that have them - forEach(elementControllers, function(controller) { - var controllerInstance = controller.instance; - if (isFunction(controllerInstance.$onChanges)) { - try { - controllerInstance.$onChanges(controller.bindingInfo.initialChanges); - } catch (e) { - $exceptionHandler(e); + // Bind the required controllers to the controller, if `require` is an object and `bindToController` is truthy + forEach(controllerDirectives, function(controllerDirective, name) { + var require = controllerDirective.require; + if (controllerDirective.bindToController && !isArray(require) && isObject(require)) { + extend(elementControllers[name].instance, getControllers(name, require, $element, elementControllers)); } - } - if (isFunction(controllerInstance.$onInit)) { - try { - controllerInstance.$onInit(); - } catch (e) { - $exceptionHandler(e); + }); + + // Handle the init and destroy lifecycle hooks on all controllers that have them + forEach(elementControllers, function(controller) { + var controllerInstance = controller.instance; + if (isFunction(controllerInstance.$onChanges)) { + try { + controllerInstance.$onChanges(controller.bindingInfo.initialChanges); + } catch (e) { + $exceptionHandler(e); + } } - } - if (isFunction(controllerInstance.$doCheck)) { - controllerScope.$watch(function() { controllerInstance.$doCheck(); }); - controllerInstance.$doCheck(); - } - if (isFunction(controllerInstance.$onDestroy)) { - controllerScope.$on('$destroy', function callOnDestroyHook() { - controllerInstance.$onDestroy(); - }); - } - }); + if (isFunction(controllerInstance.$onInit)) { + try { + controllerInstance.$onInit(); + } catch (e) { + $exceptionHandler(e); + } + } + if (isFunction(controllerInstance.$doCheck)) { + controllerScope.$watch(function() { controllerInstance.$doCheck(); }); + controllerInstance.$doCheck(); + } + if (isFunction(controllerInstance.$onDestroy)) { + controllerScope.$on('$destroy', function callOnDestroyHook() { + controllerInstance.$onDestroy(); + }); + } + }); + } // PRELINKING for (i = 0, ii = preLinkFns.length; i < ii; i++) { @@ -2981,34 +2993,6 @@ function $CompileProvider($provide, $$sanitizeUriProvider) { return value || null; } - function setupControllers($element, attrs, transcludeFn, controllerDirectives, isolateScope, scope, newIsolateScopeDirective) { - var elementControllers = createMap(); - for (var controllerKey in controllerDirectives) { - var directive = controllerDirectives[controllerKey]; - var locals = { - $scope: directive === newIsolateScopeDirective || directive.$$isolateScope ? isolateScope : scope, - $element: $element, - $attrs: attrs, - $transclude: transcludeFn - }; - - var controller = directive.controller; - if (controller === '@') { - controller = attrs[directive.name]; - } - - var controllerInstance = $controller(controller, locals, true, directive.controllerAs); - - // For directives with element transclusion the element is a comment. - // In this case .data will not attach any data. - // Instead, we save the controllers for the element in a local hash and attach to .data - // later, once we have the actual element. - elementControllers[directive.name] = controllerInstance; - $element.data('$' + directive.name + 'Controller', controllerInstance.instance); - } - return elementControllers; - } - // Depending upon the context in which a directive finds itself it might need to have a new isolated // or child scope created. For instance: // * if the directive has been pulled into a template because another directive with a higher priority diff --git a/src/ng/controller.js b/src/ng/controller.js index 3b8d6196449b..8148307bd431 100644 --- a/src/ng/controller.js +++ b/src/ng/controller.js @@ -81,16 +81,11 @@ function $ControllerProvider() { * It's just a simple call to {@link auto.$injector $injector}, but extracted into * a service, so that one can override this service with [BC version](https://gist.github.com/1649788). */ - return function $controller(expression, locals, later, ident) { + return function $controller(expression, locals, ident) { // PRIVATE API: - // param `later` --- indicates that the controller's constructor is invoked at a later time. - // If true, $controller will allocate the object with the correct - // prototype chain, but will not invoke the controller until a returned - // callback is invoked. // param `ident` --- An optional label which overrides the label parsed from the controller // expression, if any. var instance, match, constructor, identifier; - later = later === true; if (ident && isString(ident)) { identifier = ident; } @@ -116,41 +111,6 @@ function $ControllerProvider() { assertArgFn(expression, constructor, true); } - if (later) { - // Instantiate controller later: - // This machinery is used to create an instance of the object before calling the - // controller's constructor itself. - // - // This allows properties to be added to the controller before the constructor is - // invoked. Primarily, this is used for isolate scope bindings in $compile. - // - // This feature is not intended for use by applications, and is thus not documented - // publicly. - // Object creation: http://jsperf.com/create-constructor/2 - var controllerPrototype = (isArray(expression) ? - expression[expression.length - 1] : expression).prototype; - instance = Object.create(controllerPrototype || null); - - if (identifier) { - addIdentifier(locals, identifier, instance, constructor || expression.name); - } - - return extend(function $controllerInit() { - 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, - identifier: identifier - }); - } - instance = $injector.instantiate(expression, locals, constructor); if (identifier) { diff --git a/src/ngMock/angular-mocks.js b/src/ngMock/angular-mocks.js index fd3041d3ac8d..fc8f5d543cbf 100644 --- a/src/ngMock/angular-mocks.js +++ b/src/ngMock/angular-mocks.js @@ -2332,14 +2332,11 @@ angular.mock.$RootElementProvider = function() { */ function createControllerDecorator() { angular.mock.$ControllerDecorator = ['$delegate', function($delegate) { - return function(expression, locals, later, ident) { - if (later && typeof later === 'object') { - var instantiate = $delegate(expression, locals, true, ident); - var instance = instantiate(); - angular.extend(instance, later); - return instance; - } - return $delegate(expression, locals, later, ident); + return function(expression, locals, bindings, ident) { + if (angular.isString(bindings)) ident = bindings; + var instance = $delegate(expression, locals, ident); + angular.extend(instance, bindings); + return instance; }; }]; diff --git a/test/auto/injectorSpec.js b/test/auto/injectorSpec.js index 7f5254e201e6..7189c95de5f7 100644 --- a/test/auto/injectorSpec.js +++ b/test/auto/injectorSpec.js @@ -482,25 +482,6 @@ describe('injector', function() { expect(instance).toEqual(new Clazz('a-value')); expect(instance.aVal()).toEqual('a-value'); }); - - they('should detect ES6 classes regardless of whitespace/comments ($prop)', [ - 'class Test {}', - 'class Test{}', - 'class //<--ES6 stuff\nTest {}', - 'class//<--ES6 stuff\nTest {}', - 'class {}', - 'class{}', - 'class //<--ES6 stuff\n {}', - 'class//<--ES6 stuff\n {}', - 'class/* Test */{}', - 'class /* Test */ {}' - ], function(classDefinition) { - // eslint-disable-next-line no-eval - var Clazz = eval('(' + classDefinition + ')'); - var instance = injector.invoke(Clazz); - - expect(instance).toEqual(jasmine.any(Clazz)); - }); } });