fix(compiler): remove dependency on AngularJS private API #11320

Splaktar · 2018-06-07T08:12:12Z

PR Checklist

Please check that your PR fulfills the following requirements:

The commit message follows our guidelines
Tests for the changes have been added or this is not a bug fix / enhancement
Docs have been added, updated, or were not required

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Enhancement
[x] Documentation content changes
[ ] Code style update (formatting, local variables)
[x] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[x] CI related changes
[ ] Infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

MdCompilerService._createController uses two private and undocumented AngularJS APIs (on $controller).
Some of the docs for respectPreAssignBindingsEnabled is confusing or incomplete.

Issue Number:
Closes #11319

What is the new behavior?

remove one of two uses of private/undocumented arguments to $controller
mark setting respectPreAssignBindingsEnabled(false) as deprecated
updated documentation
add AngularJS 1.5.x back to TravisCI
update AngularJS devDependencies to ^1.7.2

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

mgol · 2018-06-07T09:36:29Z

src/core/services/compiler/compiler.js

@@ -124,7 +124,7 @@ function MdCompilerProvider($compileProvider) {
   * - `>=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.
+   * The default value is `false` in to AngularJS 1.6 and earlier but `true` in AngularJS 1.7.


in to AngularJS 1.6 -> in AngularJS 1.6

Also, the info that the default will change to true (for all AngularJS versions) in Material 1.2 was useful as well, I think it should stay here.

mgol · 2018-06-07T09:37:13Z

src/core/services/compiler/compiler.js

-  // TODO change it to `true` in Material 1.2.
-  var respectPreAssignBindingsEnabled = false;
+  var respectPreAssignBindingsEnabled;
+  if (angular.version.major === 1 && angular.version.minor === 7) {


=== 7 -> >= 7.

mgol · 2018-06-07T09:37:24Z

src/core/services/compiler/compiler.js

  this.respectPreAssignBindingsEnabled = function(respected) {
+    if (!respected && angular.version.major === 1 && angular.version.minor === 7) {


=== 7 -> >= 7

The !respected check means you'll catch a getter as well so this API won't work at all in 1.7. The error message suggests you meant to add this error for the setter, not a getter. The getter is probably fine.

IOW, this should be moved to inside the if (angular.isDefined(respected)) block.

mgol · 2018-06-07T09:37:46Z

src/core/services/compiler/compiler.js

  this.respectPreAssignBindingsEnabled = function(respected) {
+    if (!respected && angular.version.major === 1 && angular.version.minor === 7) {
+      throw new Error(
+        'Disabling respectPreAssignBindingsEnabled is not supported in AngularJS 1.7.');


AngularJS 1.7 -> AngularJS >= 1.7.

mgol · 2018-06-07T09:39:16Z

src/core/services/compiler/compiler.js

-      angular.extend(invokeCtrl.instance, locals);
-    }
+    var ctrl;
+    if (angular.version.major === 1 && angular.version.minor === 7) {


=== 7 -> >= 7.

Unfortunately, you need to check for 1.7.1 here. In 1.7.0 the options.controllerAs should be the 4th argument 😒
(It is still OK if you don't support pre-assigning in 1.7.0 so that material bahaves consistently with all 1.7+ versions, but you need to call $controller differently on 1.7.0 vs 1.7.1.)

controllerAs is an internal parameter, isn't it? Is there a way to stop relying on it?

It should be possible. It is only used for assigning the controller instance onto locals.$scope. It could be done manually with injectLocals.$scope[options.controllerAs] = <instance>.

That seems like a better way to do it.

mgol · 2018-06-07T09:39:45Z

src/core/services/compiler/compiler.spec.js

@@ -164,12 +164,24 @@ describe('$mdCompiler service', function() {

  });

-  [
+  var bindingStatesToTest;
+  if (angular.version.major === 1 && angular.version.minor === 7) {


=== 7 -> >= 7.

mgol · 2018-06-07T09:42:03Z

src/core/services/compiler/compiler.js

+  if (angular.version.major === 1 && angular.version.minor === 7) {
+    respectPreAssignBindingsEnabled = true;
+  } else {
+    respectPreAssignBindingsEnabled = false;


You can shorten that if you want:

var respectPreAssignBindingsEnabled = angular.version.major === 1 && angular.version.minor >= 7;

Splaktar · 2018-06-07T19:18:38Z

I believe that all of the feedback has been addressed. Tests still look good against 1.7.0 and 1.7.1. PTAL.

gkalpak

The respectPreAssignBindingsEnabled() docs mention:

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.

This isn't true for 1.7. Maybe this should be mentioned in the docs.

Other than that LGTM 👍

mgol

I left some more comments (it's almost there for me, though!).

mgol · 2018-06-07T21:04:23Z

src/core/services/compiler/compiler.js

-    if (getPreAssignBindingsEnabled() && options.bindToController) {
-      angular.extend(invokeCtrl.instance, locals);
+    var ctrl;
+    if ((angular.version.major === 1 && angular.version.minor === 7 && angular.version.dot >= 1) ||


I don't think you have to check for >= 1.7.1 now that internal API usage is completely removed (i.e. the controllerAs parameter is no longer passed).

This can be changed to just angular.version.major === 1 && angular.version.minor >= 7 as in other places.

Am I right, @gkalpak?

mgol · 2018-06-07T21:08:21Z

src/core/services/compiler/compiler.js

@@ -124,7 +124,8 @@ function MdCompilerProvider($compileProvider) {
   * - `>=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.
+   * The default value is `false` in AngularJS 1.6 and earlier but `true` in AngularJS 1.7.
+   * It is planned to change this to always default to `true` in AngularJS Material 1.2.


Saying that the default is true in AngularJS 1.7 suggests it can be changed, especially that the docs nowhere say that you can't change it.

Also, AngularJS 1.7 -> AngularJS 1.7 or later.

How about something like:

In AngularJS 1.7 or later the value is hardcoded to `true`. In AngularJS 1.6 and earlier it's `false` by default. It is planned to change this to always default to `true` in AngularJS Material 1.2.

mgol · 2018-06-07T21:10:07Z

src/core/services/compiler/compiler.spec.js

@@ -164,12 +164,24 @@ describe('$mdCompiler service', function() {

  });

-  [
+  var bindingStatesToTest;
+  if (angular.version.major === 1 && angular.version.minor >= 7) {


That's not 100% necessary but a test checking the respectPreAssignBindingsEnabled setter throws in 1.7 would be awesome.

I gave it a shot, but I'm not sure how to test an exception in a module config function and calling it outside of that isn't allowed (not a function).

This may be doable, the main problem is it's not possible to call module (which is used in tests to access the config phase) after inject was called at least once. You'd have to move inject from beforeEach of it blocks so that at the beginning of a new it you can still call:

module(function ($mdCompilerProvider) { expect(function () { $compileProvider.preAssignBindingsEnabled(false); }).toThrow(); });

I'm not 100% sure if an error thrown during the config phase doesn't break the test infrastructure but maybe it doesn't.

You can test that it throws with something like:

it('should throw ...', () => { module('material.core', $mdCompilerProvider => $mdCompilerProvider.respectPreAssignBindingsEnabled(false)); expect(inject).toThrowError(regexThatMatchesTheErrorMessage); });

OR

it('should throw ...', () => { let provider; module('material.core', $mdCompilerProvider => { provider = $mdCompilerProvider; }); inject(); expect(() => provider.respectPreAssignBindingsEnabled(false)). toThrowError(regexThatMatchesTheErrorMessage); });

(Note: If you don't call inject(), config blocks will not be run.)

gkalpak · 2018-06-11T19:29:07Z

>=1.7 - the compiler calls the constructor first before assigning bindings and $compileProvider.preAssignBindingsEnabled() no longer exists.

This can still break an app, though.

Splaktar · 2018-06-12T01:30:25Z

Right, but it's a breaking change in AngularJS 1.7, so they can keep AngularJS on 1.6.x or they can refactor their app.

If they only upgrade AngularJS Material (and not AngularJS), then they shouldn't see anything break.

Am I missing something here?

gkalpak · 2018-06-12T08:16:49Z

Right, but it's a breaking change in AngularJS 1.7, so they can keep AngularJS on 1.6.x or they can refactor their app.

This is debatable 😁 AngularJS removed preAssignBindings and that is a breaking change in AngularJS. ngMaterial (through its own $mdCompiler) also supported preAssignBindings for specific components (that were instantiated via $mdCompiler).

There are two ways to look at things:

The fact that ngMaterial used some private AngularJS APIs to achieve this is an implementation detail (as far the user is concerned). Seen that way, it could be considered a breaking change in ngMaterial to stop pre-assigning bindings in dialogs, etc.
On the other hand, the fact that ngMaterial has a separate preAssignBindings mechanism could also be seen as an implementation detail. In that regard, one could say ngMaterial should always follow the AngularJS behavior (and respect $compileProvider.preAssingBindingsEnabled() - if present).

I think (2) would be better (aligning ngMaterial with AngularJS), but this hasn't been the case 😞
ngMaterial used to always pre-assign bindings and ignore preAssignBindingsEnabled() and then supported respectPreAssignBindingsEnabled(), which imply that pre-assigning bindings in ngMaterial is decoupled from AngularJS' pre-assigning.

It is a grey area 😃

Possible scenario:

I am using ngMaterial with AngularJS 1.6.x with preAssignBindingsEnabled(false).
I have fixed all my component that go through $compile to work without pre-assigning bindings.
My ngMaterial components (e.g. dialogs), still depend on pre-assigning bindings.
I upgrade to AngularJS 1.7.x - everything works.
I upgrade ngMaterial to a version that removes pre-assigning bindings - app breaks.

(Again, this is very open to interpretation. E.g. based on peerDependency ranges, one shouldn't be using 1.7.x yet, etc.)

mgol · 2018-06-12T08:25:04Z

I upgrade ngMaterial to a version that removes pre-assigning bindings - app breaks.

To add to that: if one used a version of Material that doesn't officially support AngularJS 1.7 and then performs a patch-level update of Material and the app breaks because pre-assigning is gone then IMO it's not technically a breaking change as they were using an unsupported configuration.

That is, unless there is a version of Material 1.1.x that officially supports AngularJS 1.7.0 - then this PR would definitely be a breaking change in Material so shouldn't land in 1.1.x.

EDIT: I see @gkalpak has already said something like that, I just expanded on it a little.

jpike88 · 2018-06-19T03:17:11Z

It was said in a different PR, but preAssignBindings has been reverted and is now back in AngularJS 1.7.2

gkalpak · 2018-06-19T08:14:26Z

@jpike88, to be clear, preAssignBindings have not been reverted in AngularJS. I.e. AngularJS 1.7.x does not support pre-assigning bindings on components and directives that are compiled through the built-in $compile service.

What was reverted, were some internal, private APIs (no longer used by AngularJS itself), that are used by AngularJS Material to implement pre-assigning bindings in their $mdCompiler service. This means, that pre-assigning is only possible for components/directive that go through ngMaterial's $mdCompiler (such as mdDialogs, etc.).

Splaktar · 2018-06-19T09:32:01Z

As @mgol mentioned, there is no current version of AngularJS Material that officially supports AngularJS 1.7.x. AngularJS Material 1.1.10 will be the first version that supports it.

Our current documentation (1.1.9) for $mdCompilerProvider.respectPreAssignBindingsEnabled(true) says the following:

>=1.7 - the compiler calls the constructor first before assigning bindings and $compileProvider.preAssignBindingsEnabled() no longer exists.

That is the recommended configuration, but it only covers half of the options. If $mdCompilerProvider.respectPreAssignBindingsEnabled(false) then

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.

So it seems like we need to support this $mdCompilerProvider.respectPreAssignBindingsEnabled(false), at least until 1.2.0. We should probably mark it deprecated in this update though.

mgol · 2018-06-19T09:47:23Z

So it seems like we need to support this $mdCompilerProvider.respectPreAssignBindingsEnabled(false), at least until 1.2.0. We should probably mark it deprecated in this update though.

Deprecating it is tricky as this means you're discouraging people from using it - and not using it means having the default of false which is not desirable...

I guess you can deprecate setting it to false but since that's the default I don't imagine a lot of people specifying it...

Splaktar · 2018-06-19T09:58:20Z

@mgol good point, we can't use @deprecated on the API. I'll just add to the existing description:

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.

With the following:

The ability to disable this settings is deprecated and will be removed in AngularJS Material 1.2.0.

Splaktar · 2018-06-19T10:13:09Z

PR updated, description in OP updated, commit msg updated. PTAL.

mgol

One remark.

mgol · 2018-06-19T11:00:44Z

src/core/services/compiler/compiler.js

    // *different instance* from `invokeCtrl()`.
-    var invokeCtrl = this.$controller(options.controller, injectLocals, true, options.controllerAs);
+    var invokeCtrl = this.$controller(options.controller, injectLocals, true);


Could you switch the logic to not pass the third parameter if getPreAssignBindingsEnabled() === false? This would be safer as in the recommended usage no private API would be used.

Good call. I think it makes the 1.2.0 update more straightforward as well.

PTAL and let me know if I missed anything.

mgol

LGTM, good job!

gkalpak · 2018-06-20T11:53:49Z

src/core/services/compiler/compiler.js


-    if (!getPreAssignBindingsEnabled() && options.bindToController) {
+    if (!preAssignBindingsEnabled && options.bindToController) {


Nit: this could be moved inside the the else block above.

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

Splaktar self-assigned this Jun 7, 2018

Splaktar requested a review from mgol June 7, 2018 08:12

googlebot added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Jun 7, 2018

Splaktar force-pushed the angularJS-1.7.1-Support branch from 531bf90 to 0f7df0a Compare June 7, 2018 09:15

Splaktar changed the title ~~WIP: fix(compiler): resolve breaking changes in AngularJS 1.7.1~~ fix(compiler): resolve breaking changes in AngularJS 1.7.1 Jun 7, 2018

Splaktar added needs: review This PR is waiting on review from the team and removed needs: investigation The cause of this issue is not well understood and needs to be investigated by the team or community labels Jun 7, 2018

Splaktar mentioned this pull request Jun 7, 2018

compiler: improve support for AngularJS 1.7.2+ #11319

Closed

Splaktar added this to the 1.1.10 milestone Jun 7, 2018

Splaktar removed the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Jun 7, 2018

mgol suggested changes Jun 7, 2018

View reviewed changes

Splaktar added in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs and removed needs: review This PR is waiting on review from the team labels Jun 7, 2018

Splaktar force-pushed the angularJS-1.7.1-Support branch 3 times, most recently from 2918cc9 to a96abb5 Compare June 7, 2018 19:17

Splaktar added needs: review This PR is waiting on review from the team and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs labels Jun 7, 2018

gkalpak approved these changes Jun 7, 2018

View reviewed changes

mgol suggested changes Jun 7, 2018

View reviewed changes

Splaktar force-pushed the angularJS-1.7.1-Support branch from a96abb5 to 50a8820 Compare June 8, 2018 00:01

Splaktar removed needs: review This PR is waiting on review from the team needs: team discussion This issue requires a decision from the team before moving forward. labels Jun 8, 2018

Splaktar force-pushed the angularJS-1.7.1-Support branch from d7beae2 to bae289e Compare June 19, 2018 09:12

Splaktar force-pushed the angularJS-1.7.1-Support branch from bae289e to bc06844 Compare June 19, 2018 10:01

Splaktar changed the title ~~fix(compiler): resolve breaking changes in AngularJS 1.7.1~~ fix(compiler): remove dependency on AngularJS private API Jun 19, 2018

Splaktar added P3: important Important issues that really should be fixed when possible. type: docs and removed P1: urgent Urgent issues that should be addressed in the next minor or patch release. labels Jun 19, 2018

Splaktar added the pr: merge ready This PR is ready for a caretaker to review label Jun 19, 2018

mgol suggested changes Jun 19, 2018

View reviewed changes

Splaktar added in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs and removed pr: merge ready This PR is ready for a caretaker to review labels Jun 20, 2018

Splaktar force-pushed the angularJS-1.7.1-Support branch from bc06844 to 401599c Compare June 20, 2018 08:19

mgol approved these changes Jun 20, 2018

View reviewed changes

Splaktar added pr: merge ready This PR is ready for a caretaker to review and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs labels Jun 20, 2018

gkalpak approved these changes Jun 20, 2018

View reviewed changes

Splaktar force-pushed the angularJS-1.7.1-Support branch from 401599c to 3ebf6cb Compare June 20, 2018 19:56

jelbourn merged commit f6534d6 into master Jun 22, 2018

Splaktar deleted the angularJS-1.7.1-Support branch June 22, 2018 21:08

		this.respectPreAssignBindingsEnabled = function(respected) {
		if (!respected && angular.version.major === 1 && angular.version.minor === 7) {


		if (!getPreAssignBindingsEnabled() && options.bindToController) {
		if (!preAssignBindingsEnabled && options.bindToController) {

fix(compiler): remove dependency on AngularJS private API #11320

fix(compiler): remove dependency on AngularJS private API #11320

Uh oh!

Conversation

Splaktar commented Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Splaktar commented Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gkalpak left a comment

Choose a reason for hiding this comment

Uh oh!

mgol left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gkalpak Jun 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gkalpak commented Jun 11, 2018

Uh oh!

Splaktar commented Jun 12, 2018

Uh oh!

gkalpak commented Jun 12, 2018 • edited by Splaktar Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mgol commented Jun 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jpike88 commented Jun 19, 2018 • edited by Splaktar Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gkalpak commented Jun 19, 2018

Uh oh!

Splaktar commented Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Splaktar commented Jun 7, 2018 •

edited

Loading

Splaktar commented Jun 7, 2018 •

edited

Loading

gkalpak Jun 8, 2018 •

edited

Loading

gkalpak commented Jun 12, 2018 •

edited by Splaktar

Loading

mgol commented Jun 12, 2018 •

edited

Loading

jpike88 commented Jun 19, 2018 •

edited by Splaktar

Loading

Splaktar commented Jun 19, 2018 •

edited

Loading