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

fix(compiler): respect preAssignBindingsEnabled state #10469

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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)"
}
}
}
50 changes: 30 additions & 20 deletions src/components/dialog/dialog.js
Original file line number Diff line number Diff line change
Expand Up @@ -604,7 +604,7 @@ function MdDialogProvider($$interimElementProvider) {
});

/* @ngInject */
function advancedDialogOptions($mdDialog, $mdConstant) {
function advancedDialogOptions() {
return {
template: [
'<md-dialog md-theme="{{ dialog.theme || dialog.defaultTheme }}" aria-label="{{ dialog.ariaLabel }}" ng-class="dialog.css">',
Expand All @@ -631,30 +631,40 @@ function MdDialogProvider($$interimElementProvider) {
' </md-dialog-actions>',
'</md-dialog>'
].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) {
Expand Down
46 changes: 28 additions & 18 deletions src/components/toast/toast.js
Original file line number Diff line number Diff line change
Expand Up @@ -321,24 +321,7 @@ function MdToastProvider($$interimElementProvider) {
' </md-button>' +
' </div>' +
'</md-toast>',
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
Expand All @@ -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';
Expand Down
63 changes: 53 additions & 10 deletions src/core/services/compiler/compiler.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

About not respecting the flag in 1.5: it's about how MdCompilerService#_createController behaves so it will not break 1.5, it will just still preassign bindings in 1.5.10 even if the developer sets preAssignBindingsEnabled to false, I see it now.

We could have respected the flag in 1.5.10 if your PR from a while ago accounting for this flag wasn't merged before this one as it wouldn't be breaking then - no one would be using 1.5.10 with Material & this flag set to false as this configuration was broken.

I assume Material needs to ignore this flag because Material 1.1.3 works fine in 1.5.10 with this flag set to false? (Does it?)

Copy link
Member

Choose a reason for hiding this comment

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

BTW, just in case we'd have to revert the removal and defer it to 1.8, it would be safer to check for angular.version.minor > 5 instead of angular.version.minor === 6

Copy link
Member

@mgol mgol Mar 11, 2017

Choose a reason for hiding this comment

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

BTW, regarding the argument for not respecting the flag in ~1.5.10 but respecting in ~1.6 - if someone was using Material with AngularJS 1.6 and it worked for them as the components they used worked fine, this change may break them.

Maybe it would be safer to release this as 1.2.0, not 1.1.5 and then not ignoring the flag if it exists?

What do you think? @devversion @gkalpak @ThomasBurleson

Copy link
Member

Choose a reason for hiding this comment

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

if someone was using Material with AngularJS 1.6 and it worked for them as the components they used worked fine, this change may break them.

🤢 True.
It sounds like there is indeed no (reaconable) way to make this non-breaking (without making it opt-in - e.g. with an mdCompiler flag) 😞

var fallbackValue = !!$compileProvider.preAssignBindingsEnabled;
Copy link
Member

Choose a reason for hiding this comment

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

As @mgol has pointed out, this will break in the future (most probably 1.7+). The plan is to remove the preAssingBindingsEnabled method and not support pre-assignement neither by default, nor as an opt-in (this is the PR btw: angular/angular.js#15782).

Base on what we have discussed in the last meeting, you need something like this:

var defaultPreAssignValue = angular.version.major === 1 && angular.version.minor < 6;
var respectPreAssignState = angular.version.major === 1 && angular.version.minor > 5 &&
                            $compileProvider.preAssignBindingsEnabled;

this.$get = function() {
  return respectPreAssingState ? $compileProvider.preAssignBindingsEnabled() : defaultPreAssignState;
};

Copy link
Member

Choose a reason for hiding this comment

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

BTW, the current implementation will also break in 1.5.0-1.5.9 (where there is no $compileProvider.preAssignBindingsEnabled method).

Copy link
Member

Choose a reason for hiding this comment

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

@gkalpak

BTW, the current implementation will also break in 1.5.0-1.5.9 (where there is no $compileProvider.preAssignBindingsEnabled method).

How so? fallbackValue will then be false but it won't be used anyway.

Copy link
Member

Choose a reason for hiding this comment

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

fallbackValue will be false and it will be used. respectPreAssignState would be false, so the (incorrect) fallback value will be used.

(To be clear, by "current implementation" I mean the implementation currently used in this PR.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, I thought that preAssignBindingsEnabled has been added in 1.5.X and changed in 1.6 to false by default and will be removed in 1.7.

Copy link
Member

Choose a reason for hiding this comment

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

@gkalpak Ah, you're right.

@devversion The flag has been added in 1.5.9 which was released after a long wait and even after 1.6.0 was released. It's highly likely most of 1.5.x users will not have this flag available.

Copy link
Member

Choose a reason for hiding this comment

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

The fallback value should be defined based on AngularJS version, not the presence of the $compileProvider.preAssignBindingsEnabled flag.

Besides, as far as I understand, Material still supports AngularJS 1.3+, doesn't it?


this.$get = function() {
return respectPreAssignState ? $compileProvider.preAssignBindingsEnabled() : fallbackValue;
};
}

/**
* @ngdoc service
Expand Down Expand Up @@ -81,7 +96,9 @@ angular
* </hljs>
*
*/
function MdCompilerService($q, $templateRequest, $injector, $compile, $controller) {
function MdCompilerService($q, $templateRequest, $injector, $compile, $controller,
$$mdPreAssignBindings) {

/** @private @const {!angular.$q} */
this.$q = $q;

Expand All @@ -96,6 +113,9 @@ function MdCompilerService($q, $templateRequest, $injector, $compile, $controlle

/** @private @const {!angular.$controller} */
this.$controller = $controller;

/** @private @const {boolean} */
this.preAssignBindingsEnabled = $$mdPreAssignBindings;
}

/**
Expand Down Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

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

Currently, injectLocals and locals are the same object. I assume this a bug, but not directly related to (or introduced by) this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

In what way are those similar? injectLocals contains the $element DI token.

Copy link
Member

Choose a reason for hiding this comment

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

In the way that angular.extend(locals, {$element: ...}) extends locals with $element and then returns locals. So, injectLocals === locals. I assume the intended behavior was injectLocals = angular.extend({$element: ...}, locals) or injectLocals = angular.extend({}, locals, {$element: ...}).


// Unique identifier for Angular Route ngView controllers.
element.data('$ngControllerController', ctrl);
Expand All @@ -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.
Expand Down
91 changes: 74 additions & 17 deletions src/core/services/compiler/compiler.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
});

});
Expand Down