From 3b6f4a21cfc98f8154e0c9e2642c844f58fb84ad Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Sat, 11 Mar 2017 12:04:56 +0100 Subject: [PATCH] fix(compiler): respect preAssignBindingsEnabled state * The `$mdCompiler` now respects the `preAssignBindingsEnabled` state when using AngularJS 1.6 or higher. **BREAKING CHANGE** Developers that are upgrading from AngularJS 1.5 to AngularJS 1.6 may run into issues with their custom controllers. All custom controllers that are are being used inside of Material components, like in the `$mdDialog` or `$mdToast`, can be affected. > Limited to controllers that are used in combination with the `bindToController` and `locals` option. Angular Material starts respecting the [`preAssignBindingsEnabled`](angular/angular.js#15352) state when using AngularJS 1.6+ This will mean that bindings are no longer initialized at `constructor` time. ```js $mdDialog.show({ locals: { myVar: true }, controller: MyController, bindToController: true } function MyController() { // No locals from Angular Material are set yet. e.g myVar is undefined. } MyController.prototype.$onInit = function() { // Bindings are now set in the $onInit lifecycle hook. } ``` To provide consistency, Angular Material components with custom controllers now work with the `$onInit` lifecycle hook (**no** other lifecycle hooks) Fixes #10016 --- package.json | 4 +- src/components/dialog/dialog.js | 50 ++++++----- src/components/toast/toast.js | 46 +++++++---- src/core/services/compiler/compiler.js | 63 +++++++++++--- src/core/services/compiler/compiler.spec.js | 91 +++++++++++++++++---- 5 files changed, 187 insertions(+), 67 deletions(-) diff --git a/package.json b/package.json index 959c1b5da52..6c2e8e15991 100644 --- a/package.json +++ b/package.json @@ -61,7 +61,7 @@ "jquery": "^3.0.0", "jshint": "^2.9.2", "jshint-summary": "^0.4.0", - "karma": "^1.0.0", + "karma": "1.4.1", "karma-chrome-launcher": "^2.0.0", "karma-firefox-launcher": "^1.0.0", "karma-jasmine": "^1.0.2", @@ -84,4 +84,4 @@ "merge-base": "git merge-base $(npm run -s current-branch) origin/master", "squash": "git rebase -i $(npm run -s merge-base)" } -} \ No newline at end of file +} diff --git a/src/components/dialog/dialog.js b/src/components/dialog/dialog.js index e0a15ecfcf3..0f93a4349fd 100644 --- a/src/components/dialog/dialog.js +++ b/src/components/dialog/dialog.js @@ -604,7 +604,7 @@ function MdDialogProvider($$interimElementProvider) { }); /* @ngInject */ - function advancedDialogOptions($mdDialog, $mdConstant) { + function advancedDialogOptions() { return { template: [ '', @@ -631,30 +631,40 @@ function MdDialogProvider($$interimElementProvider) { ' ', '' ].join('').replace(/\s\s+/g, ''), - controller: function mdDialogCtrl() { - var isPrompt = this.$type == 'prompt'; - - if (isPrompt && this.initialValue) { - this.result = this.initialValue; - } - - this.hide = function() { - $mdDialog.hide(isPrompt ? this.result : true); - }; - this.abort = function() { - $mdDialog.cancel(); - }; - this.keypress = function($event) { - if ($event.keyCode === $mdConstant.KEY_CODE.ENTER) { - $mdDialog.hide(this.result); - } - }; - }, + controller: MdDialogController, controllerAs: 'dialog', bindToController: true, }; } + /** + * Controller for the md-dialog interim elements + * @ngInject + */ + function MdDialogController($mdDialog, $mdConstant) { + // For compatibility with AngularJS 1.6+, we should always use the $onInit hook in + // interimElements. The $mdCompiler simulates the $onInit hook for all versions. + this.$onInit = function() { + var isPrompt = this.$type == 'prompt'; + + if (isPrompt && this.initialValue) { + this.result = this.initialValue; + } + + this.hide = function() { + $mdDialog.hide(isPrompt ? this.result : true); + }; + this.abort = function() { + $mdDialog.cancel(); + }; + this.keypress = function($event) { + if ($event.keyCode === $mdConstant.KEY_CODE.ENTER) { + $mdDialog.hide(this.result); + } + }; + }; + } + /* @ngInject */ function dialogDefaultOptions($mdDialog, $mdAria, $mdUtil, $mdConstant, $animate, $document, $window, $rootElement, $log, $injector, $mdTheming, $interpolate, $mdInteraction) { diff --git a/src/components/toast/toast.js b/src/components/toast/toast.js index f346c016ab6..a95f365cba6 100644 --- a/src/components/toast/toast.js +++ b/src/components/toast/toast.js @@ -321,24 +321,7 @@ function MdToastProvider($$interimElementProvider) { ' ' + ' ' + '', - controller: /* @ngInject */ function mdToastCtrl($scope) { - var self = this; - - if (self.highlightAction) { - $scope.highlightClasses = [ - 'md-highlight', - self.highlightClass - ] - } - - $scope.$watch(function() { return activeToastContent; }, function() { - self.content = activeToastContent; - }); - - this.resolve = function() { - $mdToast.hide( ACTION_RESOLVE ); - }; - }, + controller: MdToastController, theme: $mdTheming.defaultTheme(), controllerAs: 'toast', bindToController: true @@ -354,6 +337,33 @@ function MdToastProvider($$interimElementProvider) { return $mdToast; + /** + * Controller for the Toast interim elements. + * @ngInject + */ + function MdToastController($mdToast, $scope) { + // For compatibility with AngularJS 1.6+, we should always use the $onInit hook in + // interimElements. The $mdCompiler simulates the $onInit hook for all versions. + this.$onInit = function() { + var self = this; + + if (self.highlightAction) { + $scope.highlightClasses = [ + 'md-highlight', + self.highlightClass + ] + } + + $scope.$watch(function() { return activeToastContent; }, function() { + self.content = activeToastContent; + }); + + this.resolve = function() { + $mdToast.hide( ACTION_RESOLVE ); + }; + } + } + /* @ngInject */ function toastDefaultOptions($animate, $mdToast, $mdUtil, $mdMedia) { var SWIPE_EVENTS = '$md.swipeleft $md.swiperight $md.swipeup $md.swipedown'; diff --git a/src/core/services/compiler/compiler.js b/src/core/services/compiler/compiler.js index d72da3d2fb3..dfd5100233e 100644 --- a/src/core/services/compiler/compiler.js +++ b/src/core/services/compiler/compiler.js @@ -6,7 +6,22 @@ */ angular .module('material.core') - .service('$mdCompiler', MdCompilerService); + .service('$mdCompiler', MdCompilerService) + .provider('$$mdPreAssignBindings', PreAssignBindingsProvider); + +/** + * Provider that is used to report the preAssignBindingsEnabled state to the $mdCompiler service. + */ +function PreAssignBindingsProvider($compileProvider) { + // To avoid that the preAssignBindings state causes issues when upgrading from AngularJS 1.5 to + // AngularJS 1.6, AngularJS Material will only respect the preAssignBindingsEnabled state in 1.6 + var respectPreAssignState = angular.version.major === 1 && angular.version.minor === 6; + var fallbackValue = !!$compileProvider.preAssignBindingsEnabled; + + this.$get = function() { + return respectPreAssignState ? $compileProvider.preAssignBindingsEnabled() : fallbackValue; + }; +} /** * @ngdoc service @@ -81,7 +96,9 @@ angular * * */ -function MdCompilerService($q, $templateRequest, $injector, $compile, $controller) { +function MdCompilerService($q, $templateRequest, $injector, $compile, $controller, + $$mdPreAssignBindings) { + /** @private @const {!angular.$q} */ this.$q = $q; @@ -96,6 +113,9 @@ function MdCompilerService($q, $templateRequest, $injector, $compile, $controlle /** @private @const {!angular.$controller} */ this.$controller = $controller; + + /** @private @const {boolean} */ + this.preAssignBindingsEnabled = $$mdPreAssignBindings; } /** @@ -244,17 +264,12 @@ MdCompilerService.prototype._compileElement = function(locals, element, options) // Instantiate controller if the developer provided one. if (options.controller) { - var injectLocals = angular.extend(locals, { + var injectLocals = angular.extend({}, locals, { $element: element }); - var invokeCtrl = self.$controller(options.controller, injectLocals, true, options.controllerAs); - - if (options.bindToController) { - angular.extend(invokeCtrl.instance, locals); - } - - var ctrl = invokeCtrl(); + // Create the specified controller instance. + var ctrl = self._createController(options, injectLocals, locals); // Unique identifier for Angular Route ngView controllers. element.data('$ngControllerController', ctrl); @@ -272,6 +287,34 @@ MdCompilerService.prototype._compileElement = function(locals, element, options) }; +/** + * Creates and instantiates a new controller with the specified options. + * @param {!Object} options Options that include the controller + * @param {!Object} injectLocals Locals to to be provided in the controller DI. + * @param {!Object} locals Locals to be injected to the controller. + * @returns {!Object} Created controller instance. + * @private + */ +MdCompilerService.prototype._createController = function(options, injectLocals, locals) { + var invokeCtrl = this.$controller(options.controller, injectLocals, true, options.controllerAs); + + if (this.preAssignBindingsEnabled && options.bindToController) { + angular.extend(invokeCtrl.instance, locals); + } + + // Instantiate and initialize the specified controller. + var ctrl = invokeCtrl(); + + if (!this.preAssignBindingsEnabled && options.bindToController) { + angular.extend(invokeCtrl.instance, locals); + } + + // Call the $onInit hook if it's present on the controller. + angular.isFunction(ctrl.$onInit) && ctrl.$onInit(); + + return ctrl; +}; + /** * Fetches an element removing it from the DOM and using it temporary for the compiler. * Elements which were fetched will be restored after use. diff --git a/src/core/services/compiler/compiler.spec.js b/src/core/services/compiler/compiler.spec.js index f72c6f152da..64c1c4b5079 100644 --- a/src/core/services/compiler/compiler.spec.js +++ b/src/core/services/compiler/compiler.spec.js @@ -12,6 +12,13 @@ describe('$mdCompiler service', function() { return compileData; } + function setPreAssignBindings(preAssignBindingsEnabled) { + module(function($compileProvider) { + $compileProvider.preAssignBindingsEnabled(preAssignBindingsEnabled); + }); + } + + describe('setup', function() { it('element should use templateUrl', inject(function($templateCache) { @@ -159,25 +166,75 @@ describe('$mdCompiler service', function() { expect(scope.myControllerAs).toBe(data.element.controller()); })); - it('should work with bindToController', inject(function($rootScope) { - var called = false; - var data = compile({ - template: 'hello', - controller: function($scope) { - expect(this.name).toBe('Bob'); - expect($scope.$apply).toBeTruthy(); // test DI working properly - called = true; - }, - controllerAs: 'ctrl', - bindToController: true, - locals: { name: 'Bob' } + }); + + }); + + describe('with preAssignBindings', function() { + + function compileAndLink(options, preAssignBindings) { + var compileData; + + inject(function($mdCompiler, $rootScope) { + $mdCompiler.preAssignBindingsEnabled = preAssignBindings; + $mdCompiler.compile(options).then(function(data) { + data.link($rootScope); + compileData = data; }); - var scope = $rootScope.$new(); - data.link(scope); - expect(scope.ctrl.name).toBe('Bob'); - expect(called).toBe(true); - })); + $rootScope.$apply(); + }); + + return compileData; + } + + it('enabled should assign bindings at instantiation', function() { + var isInstantiated = false; + + function TestController($scope) { + isInstantiated = true; + expect($scope.$apply).toBeTruthy(); + expect(this.name).toBe('Bob'); + } + + compileAndLink({ + template: 'hello', + controller: TestController, + controllerAs: 'ctrl', + bindToController: true, + locals: { name: 'Bob' } + }, true); + + expect(isInstantiated).toBe(true); + }); + + it('disabled should assign bindings after constructor', function() { + setPreAssignBindings(false); + + var isInstantiated = false; + + function TestController($scope) { + isInstantiated = true; + expect($scope.$apply).toBeTruthy(); + expect(this.name).toBeUndefined(); + } + + TestController.prototype.$onInit = function() { + expect(this.name).toBe('Bob'); + }; + + spyOn(TestController.prototype, '$onInit').and.callThrough(); + + compileAndLink({ + template: 'hello', + controller: TestController, + controllerAs: 'ctrl', + bindToController: true, + locals: { name: 'Bob' } + }, false); + + expect(TestController.prototype.$onInit).toHaveBeenCalledTimes(1); + expect(isInstantiated).toBe(true); }); });