feat($compile): isolate scope properties in controller context via controllerAs #7645

caitp · 2014-05-30T18:12:25Z

It is now possible to ask the $compiler's isolate scope property machinery to
reference properties by their isolate scope.

The current syntax is to prefix the scope name with a '@', like so:

    scope: {
     "@myData": "=someData",
     "@myString": "@someInterpolation",
     "@myExpr": "&someExpr"
   },
   controllerAs: "someCtrl"

The putting of properties within the context of the controller will only occur
if controllerAs is used, and the @-prefix is kind of a gross sort of magic.

Benefits:

$scope[controllerAsName] is available to the controllers constructor
isolate scope properties are also available to the controllers constructor
It is now possible to defer calling of an object's constructor via $injector.instantiateLater

Problems:

The '@'-prefix syntax is gross and I don't like it one bit.
Extending the directive API is gross and I don't like it one bit.
Adding new APIs to the injector is gross and I don't like it one bit.
Some of the refactoring will likely have minor performance implications
It's possible that there are bugs not being hit due to lack of tests

Related to #7635

caitp · 2014-05-30T18:19:09Z

src/ng/compile.js

@@ -1175,6 +1175,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
      var terminalPriority = -Number.MAX_VALUE,
          newScopeDirective,
          controllerDirectives = previousCompileContext.controllerDirectives,
+          controllers,


having a separate object for this is suspect... I think elementControllers could probably be used

btford · 2014-06-09T21:27:19Z

From our meeting today, it seems like we want to take the API in this direction:

    scope: {
     "myData": "=someData",
     "myString": "@someInterpolation",
     "myExpr": "&someExpr"
   },
   bindToController: true,
   controllerAs: "someCtrl"

So you can either bind to $scope or to ctrl.

thorn0 · 2014-06-10T12:12:06Z

What if I want bindings but don't need the scope to be isolate? E.g. it's needed to access controllers of the parent directives in the template.

IgorMinar · 2014-07-02T01:39:37Z

@thorn0 that's an anti-pattern. you should never be reaching out into a scope you don't own from components. isolate scopes were created specifically to isolate components from the context in which they are used.

thorn0 · 2014-07-03T19:17:51Z

I understand this. However, consider a situation where directives are in a parent-child relationship so that they're already tightly coupled as children require the parent and the parent's scope is isolate. The children directives aren't full-fledged components or widgets, they're rather additions or plugins for the parent. In this case, especially with the 'controller as' approach applied, it makes sense to make the children's scopes inherit from the parent's, doesn't it?

The Google guide says:

Additionally, using 'controller as' makes it obvious which controller you are accessing when multiple controllers apply to an element.

That's what I mean: one template, multiple controllers; so that both controllers, the parent's and the child's, can be accessed from the child's template. Of course, we can as well bind the parent controller to the children's scopes manually in each child directive definition, but it's not DRY. ~~nor good for performance. Why create an extra watcher when we can get the same scope entry via the prototype chain?~~ OK, I figured out that the binding/watcher isn't needed and something like this might be good enough for the situation I described.

require: "^parentDirective",
link: function(scope, el, attrs, parent) {
    scope.parent = parent;
}

caitp · 2014-07-31T14:38:25Z

I forgot about this a while ago, but @lgalfaso reminded me of it, so I've added the bindToController property and removed the @ prefix requirement. Hows that look to you guys?

caitp · 2014-07-31T14:38:57Z

Still has the changes to $injector though, which could be done without.

lgalfaso · 2014-07-31T15:18:50Z

otherwise, this looks good

caitp · 2014-07-31T15:22:18Z

Right, it's been refactored so that $injector doesn't have a new method.

caitp · 2014-07-31T15:35:06Z

PTAL @btford / @IgorMinar, I think this is probably good to land now.

caitp · 2014-07-31T16:15:14Z

although @lgalfaso thinks there's a problem with it

lgalfaso · 2014-07-31T16:26:18Z

@caitp I was not aware of the restriction that you cannot have two directives with controllers under the same name at the same element. This restriction is already in place so I was not able to find anything wrong with the PR

IgorMinar · 2014-08-01T01:04:49Z

src/ng/controller.js

      }

      return instance;
    };
+
+    function addIdentifier(locals, identifier, instance, name) {
+      if (!(locals && typeof locals.$scope == 'object')) {


you can use isObject here

caitp · 2014-08-01T02:09:22Z

new comments addressed

caitp · 2014-08-11T15:25:52Z

PTAL --- Are we good to check this in for beta 18?

btford · 2014-08-11T15:47:51Z

src/ng/compile.js

@@ -878,7 +882,7 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
          : $compileNodes;

        forEach(transcludeControllers, function(instance, name) {
-          $linkNode.data('$' + name + 'Controller', instance);
+          $linkNode.data('$' + name + 'Controller', instance.instance);


perhaps worth rewriting to eliminate the instance.instance; controllerInstance.instance maybe?

osama-lionheart · 2014-08-18T16:01:30Z

Thanks @caitp for submitting this 👍 would really love to see it released ASAP as it will allow us to use the great controller-as feature and help us to have a clean codebase.

Please don't forget to merge it ;)

IgorMinar · 2014-08-21T21:16:17Z

lgtm

…ntrollerAs It is now possible to ask the $compiler's isolate scope property machinery to reference properties by their isolate scope. The current syntax is to prefix the scope name with a '@', like so: scope: { "myData": "=someData", "myString": "@someInterpolation", "myExpr": "&someExpr" }, controllerAs: "someCtrl", bindtoController: true The putting of properties within the context of the controller will only occur if controllerAs is used for an isolate scope with the `bindToController` property of the directive definition object set to `true`. Closes angular#7635 Closes angular#7645

This reverts commit 787c5a7. This change causes a regression at Google. We'll take a better look at it next week. Reopens #7645

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

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

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

caitp added component: $compile labels May 30, 2014

caitp reviewed May 30, 2014
View reviewed changes

caitp added this to the 1.3.0 milestone May 30, 2014

caitp added cla: yes and removed cla: no labels Jun 5, 2014

btford added severity: broken expected use labels Jun 9, 2014

IgorMinar reviewed Aug 1, 2014
View reviewed changes

btford reviewed Aug 11, 2014
View reviewed changes

btford removed the gh: PR label Aug 20, 2014

IgorMinar modified the milestones: 1.3.0, 1.3.0-beta.19 Aug 21, 2014

caitp force-pushed the issue-7635 branch from 6e6fbe1 to 3c3b6ad Compare August 21, 2014 22:35

caitp force-pushed the issue-7635 branch from 3c3b6ad to bd3849c Compare August 21, 2014 22:40

caitp closed this in 787c5a7 Aug 21, 2014

IgorMinar reopened this Aug 22, 2014

IgorMinar added a commit that referenced this pull request Aug 22, 2014

revert: feat($compile): bind isolate scope properties to controller

252e8b5

This reverts commit 787c5a7. This change causes a regression at Google. We'll take a better look at it next week. Reopens #7645

tbosch modified the milestones: 1.3.0-beta.19, 1.3.0-rc.0 Aug 27, 2014

caitp closed this in 5f3f25a Aug 29, 2014

thorn0 mentioned this pull request May 26, 2018

refactor($compile): remove preAssignBindingsEnabled leftovers #16580

Closed

This was referenced May 30, 2018

Update upstream angular-indonesia/angular.js#144

Merged

Update upstream javascript-indonesias/angular.js#89

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat($compile): isolate scope properties in controller context via controllerAs #7645

feat($compile): isolate scope properties in controller context via controllerAs #7645

caitp commented May 30, 2014

caitp May 30, 2014

btford commented Jun 9, 2014

thorn0 commented Jun 10, 2014

IgorMinar commented Jul 2, 2014

thorn0 commented Jul 3, 2014

caitp commented Jul 31, 2014

caitp commented Jul 31, 2014

lgalfaso commented Jul 31, 2014

caitp commented Jul 31, 2014

caitp commented Jul 31, 2014

caitp commented Jul 31, 2014

lgalfaso commented Jul 31, 2014

IgorMinar Aug 1, 2014

caitp commented Aug 1, 2014

caitp commented Aug 11, 2014

btford Aug 11, 2014

osama-lionheart commented Aug 18, 2014

IgorMinar commented Aug 21, 2014

feat($compile): isolate scope properties in controller context via controllerAs #7645

feat($compile): isolate scope properties in controller context via controllerAs #7645

Conversation

caitp commented May 30, 2014

caitp May 30, 2014

Choose a reason for hiding this comment

btford commented Jun 9, 2014

thorn0 commented Jun 10, 2014

IgorMinar commented Jul 2, 2014

thorn0 commented Jul 3, 2014

caitp commented Jul 31, 2014

caitp commented Jul 31, 2014

lgalfaso commented Jul 31, 2014

caitp commented Jul 31, 2014

caitp commented Jul 31, 2014

caitp commented Jul 31, 2014

lgalfaso commented Jul 31, 2014

IgorMinar Aug 1, 2014

Choose a reason for hiding this comment

caitp commented Aug 1, 2014

caitp commented Aug 11, 2014

btford Aug 11, 2014

Choose a reason for hiding this comment

osama-lionheart commented Aug 18, 2014

IgorMinar commented Aug 21, 2014