refactor($compile): remove preAssignBindingsEnabled leftovers #16580

thorn0 · 2018-05-26T11:16:45Z

Now that we don't need to support preAssignBindingsEnabled (removed in #15782), complexity introduced in $controller by #7645 can be removed.

thorn0 · 2018-05-26T11:18:24Z

src/auto/injector.js

-        return fn.apply(self, args);
-      } else {
+      if (isClass(fn)) {
+        // This code path was used by $controller previously and is not really needed any more.


Can we remove these hacks? Or would it be considered a breaking change in the public API? This behavior of invoke isn't documented.

Since fn is documented to be a function (not a class) and it doesn't make sense to call invoke on a class, it is fine to remove this imo.

gkalpak

Generally LGTM.
I was surprized that no tests needed to be changed/removed.

gkalpak · 2018-05-26T20:01:27Z

src/auto/injector.js

-        return fn.apply(self, args);
-      } else {
+      if (isClass(fn)) {
+        // This code path was used by $controller previously and is not really needed any more.


Since fn is documented to be a function (not a class) and it doesn't make sense to call invoke on a class, it is fine to remove this imo.

gkalpak · 2018-05-26T20:07:46Z

src/ng/controller.js

@@ -81,16 +81,12 @@ function $ControllerProvider() {
     * It's just a simple call to {@link auto.$injector $injector}, but extracted into
     * a service, so that one can override this service with [BC version](https://gist.github.com/1649788).
     */
-    return function $controller(expression, locals, later, ident) {
+    return function $controller(expression, locals, _ignored, ident) {


Theoretically, we can do whatever we want with "private", undocumented params 😁

gkalpak · 2018-05-26T20:09:38Z

src/ng/controller.js

-      //                     If true, $controller will allocate the object with the correct
-      //                     prototype chain, but will not invoke the controller until a returned
-      //                     callback is invoked.
+      //   param `_ignored` --- not used, kept for compatibility with the $controller decorator in ngMock


Why keep the unused param and not change the decorated ngMock $controller?

gkalpak · 2018-05-26T20:44:52Z

src/ng/compile.js

-          var require = controllerDirective.require;
-          if (controllerDirective.bindToController && !isArray(require) && isObject(require)) {
-            extend(elementControllers[name].instance, getControllers(name, require, $element, elementControllers));
+            var instance = $controller(controllerConstructor, locals, false, directive.controllerAs);


One difference with the previous implementation is that all controller instances where available on the element before instantiating a controller. (Now it depends on the relative order of controllers.)
But I think this is fine since it is not recommended to access controllers directly off of $element, but require them and access them in ngOnInit().

'Instances available before instantiation` is exactly what we're getting rid of.

Not really 😁

First of all, this is a refactoring commit, so we shouldn't be getting rid of anything 😛
The associated, feature-removing PR got rid of bindings (and instances?) being assigned to the controller instance before instantiation. But if someone tried to grab the instances with $element.controller('xyz'), they would be there (possibly uninstantiated, but that doesn't matter).

So, one could argue that this commit introduces a breaking change. But like I said, "I think this is fine since it is not recommended to access controllers directly off of $element, but one should require them and access them in ngOnInit()". 😃

they would be there (possibly uninstantiated, but that doesn't matter)

And only if the class isn't an ES6 class.

thorn0 · 2018-05-28T08:34:49Z

@gkalpak I've addressed all the comments. Please have a look.

gkalpak

LGTM as soon as Travis is happy 👍

thorn0 · 2018-05-29T08:38:23Z

Travis's unhappiness seems to be unrelated to my changes.

Narretz · 2018-05-30T09:28:45Z

I think the minor concerns in this comment chain: #16580 (comment) should be included in some form in the commit message. In the off chance that this affects someone, it makes it easier to recap what happened.
Would this change affect people that access the controller of another directive in a directive controller? I assume the bindings are not initialized anyway, so it shouldn't make a difference.

thorn0 · 2018-05-30T10:47:26Z

@Narretz Ok, added it.

Would this change affect people that access the controller of another directive in a directive controller

If those people access other controllers via $element.controller(...) (or directly via .data(...)) from a controller constructor and don't use ES2015 classes, then yes. However, why would anyone do it? Nobody promised it would work. It's not a documented use case.

Now that we don't need to support `preAssignBindingsEnabled` (removed in angular#15782), complexity introduced in `$controller` by angular#7645 can be removed. One difference with the previous implementation is that all non-ES2015-class controller instances were available on the element before calling their constructors. Now it depends on the relative order of controllers. Controller constructors shouldn't be used to access other controllers (e.g. via `$element.controller(directiveName)`). The recommended way is to use the `require` property of the directive definition object and the life cycle hooks `$onChanges` or `$onInit`. See https://docs.angularjs.org/api/ng/service/$compile#-require- https://docs.angularjs.org/api/ng/service/$compile#life-cycle-hooks

Narretz · 2018-05-30T16:39:40Z

Well, .controller() is a documented function on the jqlite element. Now if access to the controller instance in the constructor should be possible is probably a grey area. However, the issue #5893 was concluded with the option to bind controllers directly to another controller instance, so I think we are good here.

Narretz

Approve, but we need to be prepared that some people have been requiring the controllers via element in the constructor. ;)

Now that we don't need to support `preAssignBindingsEnabled` (removed in #15782), complexity introduced in `$controller` by #7645 can be removed. One difference with the previous implementation is that all non-ES2015-class controller instances were available on the element before calling their constructors. Now it depends on the relative order of controllers. Controller constructors shouldn't be used to access other controllers (e.g. via `$element.controller(directiveName)`). The recommended way is to use the `require` property of the directive definition object and the life cycle hooks `$onChanges` or `$onInit`. See https://docs.angularjs.org/api/ng/service/$compile#-require- https://docs.angularjs.org/api/ng/service/$compile#life-cycle-hooks Closes #16580

googlebot added the cla: yes label May 26, 2018

thorn0 commented May 26, 2018

View reviewed changes

thorn0 force-pushed the refactor-compile branch 2 times, most recently from 6b9a0fc to 219c0f0 Compare May 26, 2018 13:08

gkalpak reviewed May 26, 2018

View reviewed changes

gkalpak added component: $compile type: refactor component: misc core component: ngMock labels May 26, 2018

gkalpak added this to the 1.7.x milestone May 26, 2018

gkalpak self-assigned this May 26, 2018

thorn0 force-pushed the refactor-compile branch from 95fdaf4 to 342ab1e Compare May 28, 2018 08:41

gkalpak approved these changes May 28, 2018

View reviewed changes

thorn0 force-pushed the refactor-compile branch 2 times, most recently from 162ba2c to a88048f Compare May 29, 2018 06:58

thorn0 force-pushed the refactor-compile branch from a88048f to 510a2b5 Compare May 30, 2018 10:39

thorn0 force-pushed the refactor-compile branch from 510a2b5 to 2ee9dc2 Compare May 30, 2018 12:43

Narretz approved these changes May 30, 2018

View reviewed changes

gkalpak closed this in 8e104ee May 30, 2018

This was referenced May 30, 2018

Update upstream angular-indonesia/angular.js#144

Merged

Update upstream javascript-indonesias/angular.js#89

Merged

Splaktar mentioned this pull request Jun 7, 2018

compiler: improve support for AngularJS 1.7.2+ angular/material#11319

Closed

thorn0 mentioned this pull request Jun 8, 2018

uibModal won't open #16594

Closed

2 tasks

daviferreira mentioned this pull request Jun 11, 2018

Setting a controller doesn't work with angular 1.7.1 likeastore/ngDialog#591

Closed

dependencies bot mentioned this pull request Jun 12, 2018

Update 4 dependencies from npm JetBrains/ring-ui#288

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor($compile): remove preAssignBindingsEnabled leftovers #16580

refactor($compile): remove preAssignBindingsEnabled leftovers #16580

thorn0 commented May 26, 2018

thorn0 May 26, 2018 •

edited

Loading

gkalpak May 26, 2018

gkalpak left a comment

gkalpak May 26, 2018

gkalpak May 26, 2018

gkalpak May 26, 2018

gkalpak May 26, 2018

thorn0 May 27, 2018

gkalpak May 27, 2018

thorn0 May 27, 2018

thorn0 commented May 28, 2018

gkalpak left a comment

thorn0 commented May 29, 2018

Narretz commented May 30, 2018

thorn0 commented May 30, 2018

Narretz commented May 30, 2018 •

edited

Loading

Narretz left a comment

refactor($compile): remove preAssignBindingsEnabled leftovers #16580

refactor($compile): remove preAssignBindingsEnabled leftovers #16580

Conversation

thorn0 commented May 26, 2018

thorn0 May 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thorn0 commented May 28, 2018

gkalpak left a comment

Choose a reason for hiding this comment

thorn0 commented May 29, 2018

Narretz commented May 30, 2018

thorn0 commented May 30, 2018

Narretz commented May 30, 2018 • edited Loading

Narretz left a comment

Choose a reason for hiding this comment

thorn0 May 26, 2018 •

edited

Loading

Narretz commented May 30, 2018 •

edited

Loading