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

Commit

Permalink
fix(compiler): remove dependency on AngularJS private API (#11320)
Browse files Browse the repository at this point in the history
remove one of two uses of private/undocumented arguments to $controller
mark setting `respectPreAssignBindingsEnabled(false)` as deprecated
update documentation
add AngularJS 1.5.x back to TravisCI
update AngularJS devDependencies to `^1.7.2`

Closes #11319
  • Loading branch information
Splaktar authored and jelbourn committed Jun 22, 2018
1 parent 9cc165f commit f6534d6
Show file tree
Hide file tree
Showing 5 changed files with 108 additions and 75 deletions.
1 change: 1 addition & 0 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ branches:

jobs:
include:
- env: "NG_VERSION=1.5"
- env: "NG_VERSION=1.6"
- env: "NG_VERSION=1.7"
- env: "NG_VERSION=snapshot"
Expand Down
48 changes: 24 additions & 24 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

16 changes: 8 additions & 8 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,14 @@
"scss": "./dist/angular-material.scss",
"devDependencies": {
"add-stream": "^1.0.0",
"angular": "^1.5.0",
"angular-animate": "^1.5.0",
"angular-aria": "^1.5.0",
"angular-messages": "^1.5.0",
"angular-mocks": "^1.5.0",
"angular-route": "^1.5.0",
"angular-sanitize": "^1.5.0",
"angular-touch": "^1.5.0",
"angular": "^1.7.2",

This comment has been minimized.

Copy link
@fsmeier

fsmeier Jun 29, 2018

Contributor

This comment has been minimized.

Copy link
@Splaktar

Splaktar Jun 29, 2018

Author Member

Not sure what you mean. These dependencies are only used for contributors to AngularJS Material.

For consumers of AngularJS Material, the dependencies to look at are in https://github.com/angular/bower-material/blob/master/package.json#L7-L11.

"angular-animate": "^1.7.2",
"angular-aria": "^1.7.2",
"angular-messages": "^1.7.2",
"angular-mocks": "^1.7.2",
"angular-route": "^1.7.2",
"angular-sanitize": "^1.7.2",
"angular-touch": "^1.7.2",
"angularytics": "^0.4.0",
"canonical-path": "0.0.2",
"cli-color": "^1.0.0",
Expand Down
87 changes: 55 additions & 32 deletions src/core/services/compiler/compiler.js
Original file line number Diff line number Diff line change
Expand Up @@ -102,9 +102,9 @@ function MdCompilerProvider($compileProvider) {
* @name $mdCompilerProvider#respectPreAssignBindingsEnabled
*
* @param {boolean=} respected update the `respectPreAssignBindingsEnabled` state if provided,
* otherwise just return the current Material `respectPreAssignBindingsEnabled` state.
* @returns {boolean|MdCompilerProvider} current value if used as getter or itself (chaining)
* if used as setter
* otherwise just return the current Material `respectPreAssignBindingsEnabled` state.
* @returns {boolean|MdCompilerProvider} current value, if used as a getter, or itself (chaining)
* if used as a setter.
*
* @description
* Call this method to enable/disable whether Material-specific (dialog/panel/toast/bottomsheet)
Expand All @@ -114,26 +114,36 @@ function MdCompilerProvider($compileProvider) {
*
* If disabled (`false`), the compiler assigns the value of each of the bindings to the
* properties of the controller object before the constructor of this object is called.
* The ability to disable this settings is **deprecated** and will be removed in
* AngularJS Material 1.2.0.
*
* If enabled (`true`) the behavior depends on the AngularJS version used:
*
* - `<1.5.10` - bindings are pre-assigned.
* - `>=1.5.10 <1.7` - behaves like set to whatever `$compileProvider.preAssignBindingsEnabled()` reports.
* If the `preAssignBindingsEnabled` flag wasn't set manually, it defaults to pre-assigning bindings
* with AngularJS `1.5.x` and to calling the constructor first with AngularJS `1.6.x`.
* - `>=1.7` - the compiler calls the constructor first before assigning bindings and
* - `<1.5.10`
* - Bindings are pre-assigned.
* - `>=1.5.10 <1.7`
* - Respects whatever `$compileProvider.preAssignBindingsEnabled()` reports. If the
* `preAssignBindingsEnabled` flag wasn't set manually, it defaults to pre-assigning bindings
* with AngularJS `1.5` and to calling the constructor first with AngularJS `1.6`.
* - `>=1.7`
* - The compiler calls the constructor first before assigning bindings and
* `$compileProvider.preAssignBindingsEnabled()` no longer exists.
*
* The default value is `false` but will change to `true` in AngularJS Material 1.2.
* Defaults
* - The default value is `false` in AngularJS 1.6 and earlier.
* - It is planned to fix this value to `true` and not allow the `false` value in
* AngularJS Material 1.2.0.
*
* It is recommended to set this flag to `true` in AngularJS Material 1.1.x. The only reason
* it's not set that way by default is backwards compatibility. Not setting the flag to `true`
* when AngularJS' `$compileProvider.preAssignBindingsEnabled()` is set to `false`
* (i.e. default behavior in AngularJS 1.6 or newer) makes it hard to unit test
* It is recommended to set this flag to `true` when using AngularJS Material 1.1.x with
* AngularJS versions >= 1.5.10. The only reason it's not set that way by default is backwards
* compatibility.
*
* By not setting the flag to `true` when AngularJS' `$compileProvider.preAssignBindingsEnabled()`
* is set to `false` (i.e. default behavior in AngularJS 1.6 or newer), unit testing of
* Material Dialog/Panel/Toast/BottomSheet controllers using the `$controller` helper
* as it always follows the `$compileProvider.preAssignBindingsEnabled()` value.
* is problematic as it always follows AngularJS' `$compileProvider.preAssignBindingsEnabled()`
* value.
*/
// TODO change it to `true` in Material 1.2.
var respectPreAssignBindingsEnabled = false;
this.respectPreAssignBindingsEnabled = function(respected) {
if (angular.isDefined(respected)) {
Expand All @@ -147,11 +157,11 @@ function MdCompilerProvider($compileProvider) {
/**
* @private
* @description
* This function returns `true` if AngularJS Material-specific (dialog/panel/toast/bottomsheet) controllers have
* bindings pre-assigned in controller constructors and `false` otherwise.
* This function returns `true` if AngularJS Material-specific (dialog/panel/toast/bottomsheet)
* controllers have bindings pre-assigned in controller constructors and `false` otherwise.
*
* Note that this doesn't affect directives/components created via regular AngularJS methods which constitute most
* Material and user-created components; their behavior can be checked via
* Note that this doesn't affect directives/components created via regular AngularJS methods
* which constitute most Material and user-created components; their behavior can be checked via
* `$compileProvider.preAssignBindingsEnabled()` in AngularJS `>=1.5.10 <1.7.0`.
*
* @returns {*} current preAssignBindingsEnabled state
Expand Down Expand Up @@ -444,30 +454,43 @@ function MdCompilerProvider($compileProvider) {

/**
* Creates and instantiates a new controller with the specified options.
* @param {!Object} options Options that include the controller
* @param {!Object} options Options that include the controller function or string.
* @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.
*/
MdCompilerService.prototype._createController = function(options, injectLocals, locals) {
// The third and fourth arguments to $controller are considered private and are undocumented:
// https://github.com/angular/angular.js/blob/master/src/ng/controller.js#L86
var ctrl;
var preAssignBindingsEnabled = getPreAssignBindingsEnabled();
// The third argument to $controller is considered private and undocumented:
// https://github.com/angular/angular.js/blob/v1.6.10/src/ng/controller.js#L102-L109.
// TODO remove the use of this third argument in AngularJS Material 1.2.0.
// Passing `true` as the third argument causes `$controller` to return a function that
// gets the controller instance instead returning of the instance directly. When the
// gets the controller instance instead of returning the instance directly. When the
// controller is defined as a function, `invokeCtrl.instance` is the *same instance* as
// `invokeCtrl()`. However, then the controller is an ES6 class, `invokeCtrl.instance` is a
// `invokeCtrl()`. However, when the controller is an ES6 class, `invokeCtrl.instance` is a
// *different instance* from `invokeCtrl()`.
var invokeCtrl = this.$controller(options.controller, injectLocals, true, options.controllerAs);
if (preAssignBindingsEnabled) {
var invokeCtrl = this.$controller(options.controller, injectLocals, true);

if (getPreAssignBindingsEnabled() && options.bindToController) {
angular.extend(invokeCtrl.instance, locals);
}
if (options.bindToController) {
angular.extend(invokeCtrl.instance, locals);
}

// Instantiate and initialize the specified controller.
var ctrl = invokeCtrl();
// Use the private API callback to instantiate and initialize the specified controller.
ctrl = invokeCtrl();
} else {
// If we don't need to pre-assign bindings, avoid using the private API third argument and
// related callback.
ctrl = this.$controller(options.controller, injectLocals);

if (options.bindToController) {
angular.extend(ctrl, locals);
}
}

if (!getPreAssignBindingsEnabled() && options.bindToController) {
angular.extend(ctrl, locals);
if (options.controllerAs) {
injectLocals.$scope[options.controllerAs] = ctrl;
}

// Call the $onInit hook if it's present on the controller.
Expand Down
31 changes: 20 additions & 11 deletions src/core/services/compiler/compiler.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ describe('$mdCompiler service', function() {
var data = compile({
template: '<span>hello</span>'
});
var scope = $rootScope.$new();
var scope = $rootScope.$new(false);
data.link(scope);
expect(data.element.scope()).toBe(scope);
}));
Expand All @@ -127,7 +127,7 @@ describe('$mdCompiler service', function() {
this.injectedOne = one;
}
});
var scope = $rootScope.$new();
var scope = $rootScope.$new(false);
data.link(scope);
expect(data.element.controller()).toBeTruthy();
expect(data.element.controller().injectedOne).toBe(1);
Expand All @@ -143,7 +143,7 @@ describe('$mdCompiler service', function() {
}
});

var scope = $rootScope.$new();
var scope = $rootScope.$new(false);
data.link(scope);

expect(ctrlElement).toBe(data.element);
Expand All @@ -155,7 +155,7 @@ describe('$mdCompiler service', function() {
controller: function Ctrl() {},
controllerAs: 'myControllerAs'
});
var scope = $rootScope.$new();
var scope = $rootScope.$new(false);
data.link(scope);
expect(scope.myControllerAs).toBe(data.element.controller());
}));
Expand All @@ -164,12 +164,21 @@ describe('$mdCompiler service', function() {

});

[
var bindingStatesToTest;
if (angular.version.major === 1 && angular.version.minor >= 6) {
bindingStatesToTest = [
{respectPreAssignBindingsEnabled: true},
{respectPreAssignBindingsEnabled: false},
// TODO change `equivalentTo` to `true` in Material 1.2.
// TODO change `equivalentTo` to `true` in Material 1.2.0.
{respectPreAssignBindingsEnabled: '"default"', equivalentTo: false}
].forEach(function(options) {
];
} else if (angular.version.major === 1 && angular.version.minor < 6) {
bindingStatesToTest = [
{respectPreAssignBindingsEnabled: false}
];
}

bindingStatesToTest.forEach(function(options) {
var realRespectPreAssignBindingsEnabled = options.respectPreAssignBindingsEnabled;
var respectPreAssignBindingsEnabled = angular.isDefined(options.equivalentTo) ?
options.equivalentTo :
Expand Down Expand Up @@ -224,8 +233,8 @@ describe('$mdCompiler service', function() {
expect(isInstantiated).toBe(true);
});

// Bindings are not preassigned only if we respect the AngularJS config and they're
// disabled there. This logic will change in Material 1.2.0.
// Bindings are not pre-assigned if we respect the AngularJS config and pre-assigning
// them is disabled there. This logic will change in AngularJS Material 1.2.0.
if (respectPreAssignBindingsEnabled && !preAssignBindingsEnabledInAngularJS) {
it('disabled should assign bindings after constructor', function() {
var isInstantiated = false;
Expand Down Expand Up @@ -400,7 +409,7 @@ describe('$mdCompiler service', function() {

it('should preserve a previous linked scope', function() {

var scope = $rootScope.$new();
var scope = $rootScope.$new(false);

var data = compile({
contentElement: $compile('<div>With Scope</div>')(scope)
Expand Down Expand Up @@ -445,7 +454,7 @@ describe('$mdCompiler service', function() {
beforeEach(inject(function($injector) {
$mdCompiler = $injector.get('$mdCompiler');
$rootScope = $injector.get('$rootScope');
pageScope = $rootScope.$new();
pageScope = $rootScope.$new(false);
}));

it('should assign bindings by $onInit for ES6 classes', function(done) {
Expand Down

0 comments on commit f6534d6

Please sign in to comment.