feat($compile): add partialDigest property in directive definition

[rodyhaddad]

No description provided.

[mary-poppins]

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

• Uses the issue template (#8055)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

[IgorMinar]

@vojtajina @kseamon @lgalfaso @caitp please take a look. it's not perfect but I think it's a decent try that is useful in some scenarios.

[IgorMinar]

We are still missing a way to trigger global digest from a partial digest if needed.

[caitp]

I think it's cool, but I feel like usually the author of a template would want to have control over this, instead of having to fight with design choices of (likely 3rd party) directives.

I will look at the tests and implementation closer in the morning

[IgorMinar]

The goal of this is to localize digests triggered from a component to only that component. So e.g. ng-model and all event directives would then digest locally. (there is an issue with debounced ng-model because it uses $timeout but we can deal with that later)

The author of the template where this partially digested component lives should not make this decision - the decision should be localized to the component because it's the component that knows about how it changes the state.

[caitp]

The author of the template where this partially digested component lives should not make this decision - the decision should be localized to the component because it's the component that knows about how it changes the state.

The component really has no idea whether the same data is being used outside of the hierarchy. Only the author calling the directive would know that (under the assumption that they're different people). What this means is, under certain cases you end up fighting with a 3rd party package to make sure this setting doesn''t prevent bindings from being updated in your app, or you end up fighting a legacy package to make sure that it can and does work for that.

An author of a component really has no clue how transcluded ng-models (or whatever) need to behave.

[caitp]

An alternative solution which put this in the hands of application template authors (rightfully, I believe), would be a new attribute which could be checked for when deciding to create, or not to create, a new scope. Suppose, if ng-local-apply or whatever is present on one of the compile nodes having directives applied, it should create a scope whose $apply should target itself.

The implementation ought to be simpler, and fewer tests should be broken with a strategy like that.

It's up to you guys, but in my opinion, that would work a lot better.

[IgorMinar]

Only the component knows what it is modifying. So if a component is modifying local state then it can use this option. If it's modifying state that came from outside of the component then it either has to trigger global digest or not opt into the local digest.

The component user has no idea about how the component will use the input (if any) and whether it has internal state changes which would benefit from local digest.

[caitp]

That assumption doesn't hold for transcluded content

[IgorMinar]

Aha! Now that's different. We do need to take that into account.

On Wed, Jul 2, 2014, 8:28 PM Caitlin Potter notifications@github.com
wrote:

That assumption doesn't hold for transcluded content

—
Reply to this email directly or view it on GitHub
#8055 (comment).

[lord2800]

Shouldn't this be describe?

[IgorMinar]

This PR is very much work in progress and intended primarily to explore the
possibilities.
On Jul 2, 2014 9:12 PM, "Jeff Hubbard" notifications@github.com wrote:

In test/ng/compileSpec.js:

@@ -2167,6 +2167,71 @@ describe('$compile', function() {
});
});
});
+

• ddescribe('partialDigest', function () {

Shouldn't this be describe?

—
Reply to this email directly or view it on GitHub
https://github.com/angular/angular.js/pull/8055/files#r14496326.

[lgalfaso]

There are some weirdnesses in trying to block a $digest on the $rootScope. Say there is a directive with isolated scope and a two way binding. Say that the child scope has the flag on for partialDigest (in whatever mechanism this is defined). Now, if the value changes on the child scope and we trigger a digest on the child scope, then the parent value will be updated (this is because the $watch is created on the child scope), but there will be no digest there. To somehow add confusion, if we merge #7700, this changes once again as now the $watch is at the parent scope

[IgorMinar]

@lgalfaso if a component is propagating changes outwards it should not opt into the partial digest.

[IgorMinar]

I was thinking more about the transclusion issue and I came to conclusion that if instead of a simple flag, we'd actually point to the nearest scope on which a partial digest should be applied then transclusion would not be a problem and triggering digest from within a transcluded view would result in a digest targeting the digest target scope of the scope from which the transclusion inherits.

So given this scope tree:

 Root
 |- child 1
   |- isolate 1
     |- transclude 1-1
 |- child 2

where isolate 1 should be partially digested then the digest targets for each scope would look like this:

 Root (target = root)
 |- child 1 (target = root)
   |- isolate 1 (target = isolate 1)
     |- transclude 1-1 (target = root)
 |- child 2 (target = root)

So if a digest is triggered from the transclusion, a full digest would happen, but if the digest is triggered from isolate 1 then only partial digest would occur (and we should actually look into possibility to stop digest from descending into transclude 1-1 scope)

[lgalfaso]

if a component is propagating changes outwards it should not opt into the partial digest.

You are right, but it is possible to write a case where this is happens indirectly.

Say there are the following directives

• A directive named directiveWithPartialDigestNoIsolatedScope, that has partialDigest: true and is not isolated
• A directive named directiveWithTwoWayBinding with a two way binding at man
• A directive clickThatDoesPartialDigest that executes the expression using partial digest

<div ng-init="foo = {bar: 'baz'}">
  <directive-with-partial-digest-no-isolated-scope>
    <directive-with-two-way-binding man="foo.bar">
      <button click-that-does-partial-digest="man='go'" />
    </directive-with-two-way-binding>
  </directive-with-partial-digest-no-isolated-scope>
  {{foo.bar}}
</div>

[petebacondarwin]

What about the situation where you map an object (rather than a string or number) to the isolated scope?

scope: {
  myObj: '='
}

$isolatedScope.myObj.val = 'new thing';
$isolatedScope.$digest();

In this case if you change a property on the object, from within the isolated scope it will change in the parent scope, by the fact that both the isolated and parent scopes reference the same object.

expect($rootScope.myObj.val).toEqual('new thing');

[petebacondarwin]

Regarding transclusion: the DOM hierarchy does not map 1-1 to the scope hierarchy.

DOM:

 Root
 |- child 1
   |- isolate 1
     |- transclude 1-1
 |- child 2

SCOPE:

 |- child 1
   |- transclude 1-1
   |- isolate 1
 |- child 2

The transclude 1-1 scope and isolate 1 scope are siblings. So if a $apply is triggered inside transclude 1-1 then in this solution the digest would be triggered on the root scope. No?

So we do have a situation where the DOM and the Scope hierarchy don't directly match but we do know that for some scope and DOM association (S, E), the scopes (S', S'') of all ancestor DOM elements (E', E'', ...), can never be descendent scopes of the original scope (S).

[IgorMinar]

btw @petebacondarwin is right about the tree structure.

[tbosch]

I would prefer one argument with named flags (e.g. scope.$new({isolate: true, partialDigest: false})), as calling scope.$new(true, false) is hard to read...

[petebacondarwin]

+1, if we are going to make a (fairly major) breaking change

[tbosch]

I am thinking about the following use case:
A component that edits the properties of a person object in a modal. It has input fields for the properties of the person as well as an OK and Cancel button. While the modal is open, it only does partial digest the modal. However, when the OK button is pressed, it updates the person object and needs to do a global digest.

One solution that came to my mind: Make the directive.partialDigest an expression (or a function that returns an expression). And in rootScope.$apply: If a child scope has a partialDigest expression, we digest that scope. Afterwards $eval the partialDigest expression. When it returns true, stop, otherwise go the the next parent scope.

For the use case above we would have:

module.directive('userDetails', function() {
  return {
    scope: {
      person: '='
    },
    partialDigest: 'opened'
  };
});

A controller like this:

function PersonDetailController($scope) {
  $scope.name = $scope.person.name;
  $scope.ok = function() {
    $scope.person.name = $scope.name;
    $scope.opened = false;
  }
}

And a template with:

<div ng-show="opened">
  <input ng-model="name">
  <button ng-click="ok()">Close</button>
</div>

[tbosch]

For my proposal: This is the place where we should evaluate the $$digestTarget expression:

```if (scope.$$digestTarget && !scope.$eval(scope.$$digestTarget)) scope = scope.$parent; ...`

[tbosch]

Here is a plunker for the example above (a little bit nicer): http://plnkr.co/edit/5wlwHtkyxjRvsiz8VyXK?p=preview

Just talked to @IgorMinar and concluded that we should also add a check that shows a warning when an isolate directive uses partialDigest but modifies one of the variables that it binds to (declared in the scope property of the directive definition): After a partial digest do a global digest and check whether something else changed. If so, show a warning.

This check should only be executed when the app is run in batarang or protractor. Allowing protractor to check this as well is actually better than just checking in bataran, as this check really needs to run the app to detect the error (@btford I think this is also a good idea for the angular-hint-* projects).

[tbosch]

After sleeping over it, the expression in directive.partialDigest might not be such a good idea.

1. a function would be better than an expression as this is easier to work with
2. the example shows that this approach adds additional flags to the scope that might need to be reseted (not in the example above as it recreates the scope after every open/close of the detail).

So I would vote for a scope.preventPartialDigest() or something similar to force a global digest when needed. In the example, the personDetail directive would call this method in its ok() method.

[IgorMinar]

we should also add a test for triggering full digest from local digest - I think we'll uncover that that doesn't work because of digest in progress error. this is something that we must address and tobias's comments above tried to tackle that.

[IgorMinar]

This PR doesn't look quite right to me though.

Has @btford tried to use this on material components? I don't think that he did.

[IgorMinar]

btw while working on the slides for this feature I realized that "local digest" might be a better name. what do you think?

[kseamon]

Local apply probably - $digest is already local to the scope in question.

On Jul 31, 2014, at 2:17 AM, Igor Minar notifications@github.com wrote:

btw while working on the slides for this feature I realized that "local digest" might be a better name. what do you think?

—
Reply to this email directly or view it on GitHub.

[petebacondarwin]

+1 Karl

[rodyhaddad]

So the remaining unanswered questions are:

1. How do we go about triggering a full digest without running into a digest in progress error.
2. Currently partialDigest on a directive definition can be a function, allowing a directive to decide if its scope is partially digested at runtime. Igor points out that in the wild "each directive that uses it will have a different attr name" for turning on partial digest, which isn't something we'd want.
   Initially we thought this could be useful for modals, where the writer of the template knows more about the manipulated data than the author of the modal directive.**
3. Do we change the name to localApply?

** I'm not sure how true this still is, considering partialDigest can only be turned on for directives with isolated scopes now.

[btford]

I think localApply is a much better name.

[petebacondarwin]

1. I believe the simplest way would be $scope.$root.$evalAsync(); But we could perhaps provide a new API, such as $scope.$rootApply()
2. How about allowing the directive to specify a flag that states that localApply is optional, then we provide a new directive ngLocalApply, which takes an expression and can modify the localApply state. This ngLocalApply directive could only appear on elements that contain an isolated directive, which has specified that localApply is optional.

app.js

mod.directive('modal', function() {
  return {
    scope: {
      myObj: '='
    },
    localApply: 'optional',
    link: function(scope) {
      scope.update = function() {
        scope.$root.$evalAsync();
      };
    },
    template: 
    '<div>' +
      '<button ng-click="update()">Update</button>' +
      '<input ng-model="myObj.value">' +
    '</div>'
  };
});

index.html

<div modal ng-local-apply="true"></div>

1. +1 for localApply

[tbosch]

We are going to close this issue.
Reasoning:

• localApply would not work together with promises
• localApply would not work with directives that have an isolate scope and change the bindings (e.g. a custom checkbox)
• if a user enabled this, its hard to detect whether it really worked correctly. We would need a debug mode which does a second digest to check that nothing outside of the localApply scope was modified

To summarize, this would be doable and some use cases would benefit, but its not such a huge win as we hoped it could be.