feat(compiler): respect preAssignBindingsEnabled state #10726

mgol · 2017-06-08T15:52:14Z

The $mdCompiler is now able to respect the preAssignBindingsEnabled state
when using AngularJS 1.5.10 or higher; to enable, invoke:

$mdCompilerProvider.respectPreAssignBindingsEnabled(true);
$mdCompiler now understands the $onInit lifecycle hooks in controllers.
Note that no other AngularJS 1.5+ lifecycle hooks are supported currently.

Invoking:

$mdCompilerProvider.respectPreAssignBindingsEnabled(true);

will make bindings in Material custom components like $mdDialog or $mdToast
only available in controller constructors if they are available in constructors
of all other AngularJS controllers. By default this will happen in AngularJS
1.6 or newer. This can also be achieved with AngularJS >=1.5.10 <1.6.0 if:

$compilerProvider.preAssignBindingsEnabled(false);

is invoked.

Example:

$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.
}

Huge thanks to Paul Gschwendtner (@devversion) for the initial draft version of
this feature.

Fixes #10016
Fixed #10377
Ref #10469

~~NOTE: I'm marking this PR as WIP as I still want to test it manually with some real code to ensure there's nothing left to do. I also need to think if there's something else I need to test here.~~

@devversion

1. The `$mdCompiler` is now able to respect the `preAssignBindingsEnabled` state when using AngularJS 1.5.10 or higher; to enable, invoke: $mdCompilerProvider.respectPreAssignBindingsEnabled(true); 2. `$mdCompiler` now understands the `$onInit` lifecycle hooks in controllers. Note that no other AngularJS 1.5+ lifecycle hooks are supported currently. Invoking: $mdCompilerProvider.respectPreAssignBindingsEnabled(true); will make bindings in Material custom components like `$mdDialog` or `$mdToast` only available in controller constructors if they are available in constructors of all other AngularJS controllers. By default this will happen in AngularJS 1.6 or newer. This can also be achieved with AngularJS >=1.5.10 <1.6.0 if: $compilerProvider.preAssignBindingsEnabled(false); is invoked. Example: ```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. } ``` Huge thanks to Paul Gschwendtner (@devversion) for the initial draft version of this feature. Fixes angular#10016 Ref angular#10469 Closes angular#10726

mgol · 2017-06-11T15:44:02Z

src/core/services/compiler/compiler.js

+   * This method and the option to assign the bindings before calling the controller's constructor
+   * will be removed in v1.3.0.
+   */
+  var respectPreAssignBindingsEnabled = false;


Add a TODO comment about the default changing to true in Material 1.2.

mgol · 2017-06-11T15:48:51Z

src/core/services/compiler/compiler.js


-  /** @private @const {!angular.$compile} */
-  this.$compile = $compile;
+    // respectPreAssignBindingsEnabled === true


Add a comment that we're ignoring the AngularJS setting in this case.

mgol · 2017-06-11T16:01:17Z

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

+  [
+      {respectPreAssignBindingsEnabled: true},
+      {respectPreAssignBindingsEnabled: false},
+      {respectPreAssignBindingsEnabled: '"default"', equivalentTo: false}


Add a comment about the Material 1.2 change.

mgol · 2017-06-11T16:04:00Z

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

+
+      beforeEach(function() {
+        module(function($mdCompilerProvider) {
+          console.log(angular.version, preAssignBindingsEnabledInAngularJS);


mgol · 2017-06-11T16:09:01Z

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

+      if (respectPreAssignBindingsEnabled && !preAssignBindingsEnabledInAngularJS) {
+        it('$mdCompileProvider.preAssignBindingsEnabled should report bindings are not pre-assigned', function() {
+          module(function($mdCompilerProvider) {
+            expect($mdCompilerProvider.preAssignBindingsEnabled()).toBe(false);


~~It should be true; why did the tests pass? I need to check that.~~

EDIT: I commented in a wrong place.

Why should it be true?

It shouldn't; I commented on a wrong line; fixed.

Nice catch!

mgol · 2017-06-11T16:12:23Z

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

+
+  });
+
+  [


Add a test asserting that if bindToController is set to false the $onInit method is called anyway.

mgol · 2017-06-11T16:30:30Z

src/core/services/compiler/compiler.js

+   * makes it hard to unit test Material Dialog/Toast controllers using the `$controller` helper as it always follows
+   * the `$compileProvider.preAssignBindingsEnabled()` value.
+   *
+   * @deprecated


Remove the deprecated note (here & in other places if present).

mgol · 2017-06-12T19:12:42Z

src/core/services/compiler/compiler.js

+   *
+   * This method will be removed in v1.3.0.
+   */
+  this.preAssignBindingsEnabled = function() {


Maybe we don't need this method to be public until someone needs it?

Also, maybe change the name to indicate it's a getter? Like getPreAssignBindingsEnabled.

gkalpak

Nice 💯

gkalpak · 2017-06-12T20:47:25Z

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

+      if (respectPreAssignBindingsEnabled && !preAssignBindingsEnabledInAngularJS) {
+        it('$mdCompileProvider.preAssignBindingsEnabled should report bindings are not pre-assigned', function() {
+          module(function($mdCompilerProvider) {
+            expect($mdCompilerProvider.preAssignBindingsEnabled()).toBe(false);


Why should it be true?

gkalpak · 2017-06-12T20:50:51Z

src/core/services/compiler/compiler.js

-  /** @private @const {!angular.$q} */
-  this.$q = $q;
+MdCompilerProvider.$inject = ['$compileProvider'];
+/** @this */


Is this necessary?

gkalpak · 2017-06-12T20:51:30Z

src/core/services/compiler/compiler.js

+   * @name  $mdCompilerProvider#respectPreAssignBindingsEnabled
+   *
+   * @param {boolean=} enabled update the respectPreAssignBindingsEnabled state if provided, otherwise just return
+   * the current mdPreAssignBindingsEnabled state


mdPreAssignBindingsEnabled --> respectPreAssignBindingsEnabled

gkalpak · 2017-06-12T20:52:42Z

src/core/services/compiler/compiler.js

+   * ](https://code.angularjs.org/1.6.4/docs/api/ng/provider/$compileProvider#preAssignBindingsEnabled)
+   * for more information.
+   *
+   * If disabled (false), the compiler assigns the value of each of the bindings to the


disabled (false) --> enabled (true)

@gkalpak - @mgol and I reviewed this PR yesterday... he is making slight improvements within the next 2 days. Standy by...

😞 OK 😞
But I'll be back 😈

Of course. I expect nothing less.

@gkalpak What's wrong here? I think current text is OK and your proposed change is wrong...

Yes, it seems correct now. Not sure what happened 😁

gkalpak · 2017-06-12T20:53:17Z

src/core/services/compiler/compiler.js

+   * @ngdoc method
+   * @name  $mdCompilerProvider#respectPreAssignBindingsEnabled
+   *
+   * @param {boolean=} enabled update the respectPreAssignBindingsEnabled state if provided, otherwise just return


enabled --> respect (?)
_{(Or was enabled intntional?)}

EladBezalel · 2017-06-13T15:02:22Z

src/components/dialog/dialog.js

+    // 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';


Please use ===

This is exactly the same code that was here before. I'd rather not change unrelated things in this PR.

The same applies to most of your other comments.

If you ignore whitespace-only changes (via appending ?w=1 to the URL: https://github.com/angular/material/pull/10726/files?w=1) you will only see what really changed in the logic.

Yeah i understand.. thanks!

EladBezalel · 2017-06-13T15:02:57Z

src/components/dialog/dialog.js

+      }
+
+      this.hide = function() {
+        $mdDialog.hide(isPrompt ? this.result : true);


i think hide returns a promise so let's return it so it can be chainable

this.hide = function() { return $mdDialog.hide(isPrompt ? this.result : true); }

EladBezalel · 2017-06-13T15:03:27Z

src/components/dialog/dialog.js

+        $mdDialog.hide(isPrompt ? this.result : true);
+      };
+      this.abort = function() {
+        $mdDialog.cancel();


return here as well

EladBezalel · 2017-06-13T15:03:48Z

src/components/dialog/dialog.js

+      };
+      this.keypress = function($event) {
+        if ($event.keyCode === $mdConstant.KEY_CODE.ENTER) {
+          $mdDialog.hide(this.result);


return here as well

and outside the if

return $q.resolve();

EladBezalel · 2017-06-13T15:04:44Z

src/components/toast/toast.js

+      });
+
+      this.resolve = function() {
+        $mdToast.hide( ACTION_RESOLVE );


return here as well

this.resolve = function() { return $mdToast.hide( ACTION_RESOLVE ); }

EladBezalel · 2017-06-13T15:05:51Z

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

+        module(function($mdCompilerProvider) {
+          console.log(angular.version, preAssignBindingsEnabledInAngularJS);
+          // Don't set the value so that the default state can be tested.
+          if (realRespectPreAssignBindingsEnabled === true || realRespectPreAssignBindingsEnabled === false) {


cant we do angular.isDefined(realRespectPreAssignBindingsEnabled) instead?

I wonder why I didn't think about that having used exactly this method in source. :)

This is why it hit me

@mgol: Because you want to differentiate true/false from "default". angular.isDefined() won't work here.

Ah, that's why I didn't think about that. 😄

What about

typeof(realRespectPreAssignBindingsEnabled) === 'boolean'

That would work. As would realRespectPreAssignBindingsEnabled !== 'default'.

mgol · 2017-06-13T20:50:39Z

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

+      } else {
+        it('$mdCompileProvider.preAssignBindingsEnabled should report bindings are pre-assigned', function() {
+          module(function($mdCompilerProvider) {
+            expect($mdCompilerProvider.preAssignBindingsEnabled()).toBe(false);


It should be true; why did the tests pass? I need to check that.

@devversion

1. The `$mdCompiler` is now able to respect the `preAssignBindingsEnabled` state when using AngularJS 1.5.10 or higher; to enable, invoke: $mdCompilerProvider.respectPreAssignBindingsEnabled(true); 2. `$mdCompiler` now understands the `$onInit` lifecycle hooks in controllers. Note that no other AngularJS 1.5+ lifecycle hooks are supported currently. Invoking: $mdCompilerProvider.respectPreAssignBindingsEnabled(true); will make bindings in Material custom components like `$mdDialog` or `$mdToast` only available in controller constructors if they are available in constructors of all other AngularJS controllers. By default this will happen in AngularJS 1.6 or newer. This can also be achieved with AngularJS >=1.5.10 <1.6.0 if: $compilerProvider.preAssignBindingsEnabled(false); is invoked. Example: ```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. } ``` Huge thanks to Paul Gschwendtner (@devversion) for the initial draft version of this feature. Fixes angular#10016 Ref angular#10469 Closes angular#10726

mgol · 2017-06-21T13:08:24Z

@ThomasBurleson @EladBezalel @gkalpak PR updated, comments addressed.

mgol · 2017-06-21T14:11:45Z

I've tested this with one of my company's internal apps and at first sight everything seemed to work with $mdCompilerProvider#respectPreAssignBindingsEnabled(true) (including tests with some minor modifications).

I've also run tests on default settings and everything seemed to work without any modifications.

There's one last thing to resolve - @jbedard & @gkalpak mentioned that exposing $mdCompilerProvider.preAssignBindingsEnabled() to the public is not necessary, it may be an internal function. Let me know what you think, @ThomasBurleson; I can apply such a change. The rest should be ready.

@devversion

1. The `$mdCompiler` is now able to respect the `preAssignBindingsEnabled` state when using AngularJS 1.5.10 or higher; to enable, invoke: $mdCompilerProvider.respectPreAssignBindingsEnabled(true); 2. `$mdCompiler` now understands the `$onInit` lifecycle hooks in controllers. Note that no other AngularJS 1.5+ lifecycle hooks are supported currently. Invoking: $mdCompilerProvider.respectPreAssignBindingsEnabled(true); will make bindings in Material custom components like `$mdDialog` or `$mdToast` only available in controller constructors if they are available in constructors of all other AngularJS controllers. By default this will happen in AngularJS 1.6 or newer. This can also be achieved with AngularJS >=1.5.10 <1.6.0 if: $compilerProvider.preAssignBindingsEnabled(false); is invoked. Example: ```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. } ``` Huge thanks to Paul Gschwendtner (@devversion) for the initial draft version of this feature. Fixes angular#10016 Ref angular#10469 Closes angular#10726

ThomasBurleson

lgtm

jelbourn · 2017-08-02T23:56:34Z

Looks like this succeeds on google presubmit

mgol requested review from ThomasBurleson and EladBezalel June 8, 2017 15:52

mgol changed the title ~~feat(compiler): respect preAssignBindingsEnabled state~~ [WIP] feat(compiler): respect preAssignBindingsEnabled state Jun 8, 2017

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

mgol mentioned this pull request Jun 8, 2017

fix(compiler): respect preAssignBindingsEnabled state #10469

Closed

mgol force-pushed the no-pre-assigning branch from 1cb80f2 to acb9c98 Compare June 9, 2017 10:58

ThomasBurleson added the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Jun 10, 2017

ThomasBurleson added this to the 1.1.5 milestone Jun 10, 2017

mgol commented Jun 11, 2017

View reviewed changes

mgol commented Jun 12, 2017

View reviewed changes

gkalpak reviewed Jun 12, 2017

View reviewed changes

ThomasBurleson mentioned this pull request Jun 13, 2017

Incorrect Peer Dep #10703

Closed

EladBezalel reviewed Jun 13, 2017

View reviewed changes

ThomasBurleson mentioned this pull request Jun 13, 2017

Controller with prerendered $mdDialog never called. #10377

Closed

mgol commented Jun 13, 2017

View reviewed changes

mgol force-pushed the no-pre-assigning branch from acb9c98 to cb80f28 Compare June 21, 2017 13:08

mgol changed the title ~~[WIP] feat(compiler): respect preAssignBindingsEnabled state~~ feat(compiler): respect preAssignBindingsEnabled state Jun 21, 2017

mgol force-pushed the no-pre-assigning branch from c8d2e49 to d4487f9 Compare June 21, 2017 16:07

ThomasBurleson approved these changes Jun 21, 2017

View reviewed changes

ThomasBurleson added P0 - Presubmit First and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs labels Jun 21, 2017

tinayuangao merged commit fa997b9 into angular:master Aug 3, 2017

mgol deleted the no-pre-assigning branch September 18, 2017 18:39

This was referenced Apr 24, 2018

docs(mdCompilerProvider): missing from docs site #11252

Closed

docs(mdCompilerProvider): add respectPreAssignBindingsEnabled docs #11255

Merged

feat(compiler): respect preAssignBindingsEnabled state #10726

feat(compiler): respect preAssignBindingsEnabled state #10726

Uh oh!

Conversation

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

Uh oh!

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!

mgol Jun 11, 2017 • 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!

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 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!

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!

mgol Jun 13, 2017 • 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!

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!

mgol Jun 13, 2017 • 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!

mgol commented Jun 8, 2017 •

edited

Loading

mgol Jun 11, 2017 •

edited

Loading

mgol Jun 13, 2017 •

edited

Loading

mgol Jun 13, 2017 •

edited

Loading

EladBezalel Jun 13, 2017 •

edited by ThomasBurleson

Loading

gkalpak Jun 13, 2017 •

edited

Loading

mgol Jun 13, 2017 •

edited

Loading