compiler: improve support for AngularJS 1.7.2+

[Splaktar]

Bug, enhancement request, or proposal:

Proposal

CodePen and steps to reproduce the issue:

Demo which demonstrates the issue:

https://travis-ci.org/angular/material/jobs/388688438 is just one of many CI unit test jobs which have failed for the AngularJS SNAPSHOT view since angular/angular.js#16580 was merged.

What is the expected behavior?

AngularJS Material unit tests in master pass against the AngularJS SNAPSHOT version.

What is the current behavior?

54 failing tests can be seen in all recent jobs like https://travis-ci.org/angular/material/jobs/388688438.

What is the use-case or motivation for changing an existing behavior?

We were fairly well prepared for the changes in angular/angular.js#15782 that went into AngularJS 1.7.0, but the follow on breaking changes in angular/angular.js#16580 that are part of the unreleased AngularJS 1.7.1 remove some private and undocumented APIs that AngularJS Material depended upon.

One example is the use of the third and fourth arguments in $controller which were added to fix an issue with ES6 that we resolved in 1.1.5. You can see some of the comments that explain this here:

material/src/core/services/compiler/compiler.js

Lines 452 to 477 in 0046467

	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
	// 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
	// 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
	// *different instance* from `invokeCtrl()`.
	var invokeCtrl = this.$controller(options.controller, injectLocals, true, options.controllerAs);
	if (getPreAssignBindingsEnabled() && options.bindToController) {
	angular.extend(invokeCtrl.instance, locals);
	}
	// Instantiate and initialize the specified controller.
	var ctrl = invokeCtrl();
	if (!getPreAssignBindingsEnabled() && options.bindToController) {
	angular.extend(ctrl, locals);
	}
	// Call the $onInit hook if it's present on the controller.
	angular.isFunction(ctrl.$onInit) && ctrl.$onInit();
	return ctrl;
	};

.

Which versions of AngularJS, Material, OS, and browsers are affected?

• AngularJS: 1.7.1
• AngularJS Material: All
• OS: All
• Browsers: All

Is there anything else we should know? Stack Traces, Screenshots, etc.

We cover some of the details of $compileProvider.preAssignBindingsEnabled() in our updated docs for 1.1.9 here: https://material.angularjs.org/latest/api/service/$mdCompilerProvider#mdcompilerprovider-respectpreassignbindingsenabled-respected

[Splaktar]

@gkalpak provided this guidance:

So, this method needs to be refactored so that it doesn't pass the 3rd argument to $controller() if AngularJS is at v1.7.1 or higher 😁

material/src/core/services/compiler/compiler.js

Lines 452 to 477 in 0046467

	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
	// 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
	// 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
	// *different instance* from `invokeCtrl()`.
	var invokeCtrl = this.$controller(options.controller, injectLocals, true, options.controllerAs);
	if (getPreAssignBindingsEnabled() && options.bindToController) {
	angular.extend(invokeCtrl.instance, locals);
	}
	// Instantiate and initialize the specified controller.
	var ctrl = invokeCtrl();
	if (!getPreAssignBindingsEnabled() && options.bindToController) {
	angular.extend(ctrl, locals);
	}
	// Call the $onInit hook if it's present on the controller.
	angular.isFunction(ctrl.$onInit) && ctrl.$onInit();
	return ctrl;
	};

This appears to just be one step of many towards fixing this. Making this change alone still results in 54 failing tests.

If I make some changes to respectPreAssignBindingsEnabled in compiler.js and compiler.spec.js, I can get the failures down to 48. Most of them are from InterimElements like Dialog, Toast, Panel, but I'm still seeing 6 mdCompiler errors that all look like this:

	TypeError: invokeCtrl is not a function
	    at MdCompilerService.MdCompilerProvider.MdCompilerService._createController (src/core/services/compiler/compiler.js:477:16)
	    at Object.linkFn [as link] (src/core/services/compiler/compiler.js:432:25)
	    at UserContext.<anonymous> (src/core/services/compiler/compiler.spec.js:131:14)
	    at Object.invoke (node_modules/angular/angular.js:5090:17)
	    at UserContext.WorkFn (node_modules/angular-mocks/angular-mocks.js:3261:20)
	Error: Declaration Location
	    at window.inject.angular.mock.inject (node_modules/angular-mocks/angular-mocks.js:3224:25)
	    at Suite.<anonymous> (src/core/services/compiler/compiler.spec.js:120:53)
	    at Suite.<anonymous> (src/core/services/compiler/compiler.spec.js:109:5)
	    at Suite.<anonymous> (src/core/services/compiler/compiler.spec.js:16:3)

[Splaktar]

PR #11320 is open and appears to fix all of the tests for AngularJS SNAPSHOT while not breaking any of the tests against AngularJS 1.5 and 1.6.

[gkalpak]

this method needs to be refactored so that it doesn't pass the 3rd argument to $controller() if AngularJS is at v1.7.1 or higher

To be clear, by "refactored" I meant making it so that it doesn't rely on the removed AngularJS feature (3rd argument) in addition to not passing the argument 😁

[sgilroy]

It looks like the removal of preAssignBindingsEnabled was reverted in AngularJS 1.7.2 https://github.com/angular/angular.js/blob/master/CHANGELOG.md#172-extreme-compatiplication-2018-06-12

Does this resolve the compatibility problem, at least for AngularJS 1.7.2? Are you still planning to move forward with attempting to eliminate use of preAssignBindingsEnabled?

[Splaktar]

@sgilroy yes it was reverted in 1.7.2. Yes it solves the compatibility problem with 1.7.2 and AngularJS Material. We're re-evaluating our approach due to that change. We do still plan to eliminate respectPreAssignBindingsEnabled in AngularJS Material 1.2.0.