Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

Allow directive's bi-directional isolate scope to do deep equality check #3491

Closed
wants to merge 1 commit into from

Conversation

pheuter
Copy link

@pheuter pheuter commented Aug 7, 2013

Background

Consider the following directive:

myApp.directive('listUsers', function() {
  return {
    restrict: 'E',
    template: "<ul><li ng-repeat='user in users'>{{user.name}} - {{user.age}}</li></ul>",
    scope: {
      users: '='
    }
  };
});

It sets up bi-directional binding to the parent scope's users property that it receives via an attribute. In this particular situation, it expects an array of user objects and lists user names and their respective ages.

We could use this directive like so.

<div ng-init="users = [{name: 'Mark', value: 42}, {name: 'Misko', value: 55}]">
  <list-users users="users"></list-users>
</div>

Everything works just fine. At least until we decide to use a filter.

<div ng-init="users = [{name: 'Mark', value: 42}, {name: 'Misko', value: 55}]">
  <input type="text" ng-model="query">
  <list-users users="users | filter:query"></list-users>
</div>

If you look in the browser console, you will see something along the lines of: 10 $digest() iterations reached. Aborting!

The problem is that filter, along with many other functions that mutate arrays, return a copy and not the original array. During the $digest comparison step, the array value is deemed as changed (since a new reference is returned even though the actual elements in the array have not changed) and the watchers are called again and again.

Proposition

Allow optional "deep" equality checks on bi-directional scope values to avoid unnecessary digests.

This pull request introduces an optional argument, *, that can be passed to a directive's scope definition. When passed, a deep equality check will be forced when determining if watcher value has changed.

Now, the example above will work properly with just a minor tweak to the directive's scope definition:

myApp.directive('listUsers', function() {
  return {
    ...
    scope: {
      users: '=*'
    }
  };
});

@ghost ghost assigned IgorMinar Aug 7, 2013
@IgorMinar
Copy link
Contributor

Hi there. I understand the problem and your use case, but deep watching is too aggressive to be used here.

I wonder if using $watchCollection would solve the problem. Can you try to explore if we could just use $watchCollection when =* is used to define the scope mapping?

@pheuter
Copy link
Author

pheuter commented Aug 7, 2013

Yeah, I was afraid the deep check may cause performance issues. Sure, I can try out $watchCollection

@pheuter
Copy link
Author

pheuter commented Aug 8, 2013

@IgorMinar Unlike $watch, $watchCollection does not work if you omit the watch expression argument, so I'm not sure how to implement it in the context of parentValueWatch().

@pheuter
Copy link
Author

pheuter commented Aug 17, 2013

@IgorMinar Any ideas?

@btford
Copy link
Contributor

btford commented Aug 21, 2013

Sorry, we are in the midst of finishing 1.2 and this feature will not make it there. Let's revisit after 1.2 is out.

@timborodin
Copy link

Spent about 3 hours today catching this in my little project.
It does think than [ ] is not equal to [ ].

But, I'd say that introducing =* is not a good solution. Angular already imposes a lot of it's internal ... stuff ... on a developer. Further complicating APIs is a bad idea.

@IgorMinar
Copy link
Contributor

I'm sorry, but I wasn't able to verify your CLA signature. CLA signature is required for any code contributions to AngularJS.

Please sign our CLA and ensure that the CLA signature email address and the email address in this PR's commits match.

If you signed the CLA as a corporation, please let me know the company's name.

Thanks a bunch!

PS: If you signed the CLA in the past then most likely the email addresses don't match. Please sign the CLA again or update the email address in the commit of this PR.
PS2: If you are a Googler, please sign the CLA as well to simplify the CLA verification process.

@IgorMinar
Copy link
Contributor

no CLA. no merge. sorry

@IgorMinar IgorMinar closed this Oct 11, 2014
@pheuter
Copy link
Author

pheuter commented Oct 14, 2014

@IgorMinar oops, sorry. Just signed it!

@gabrielmaldi
Copy link
Contributor

@pheuter I'd use this! Would you be willing to send a new PR using $watchCollection?

@pheuter
Copy link
Author

pheuter commented Oct 20, 2014

@gabrielmaldi I tried it out with $watchCollection earlier, but it didn't work out.

@gabrielmaldi
Copy link
Contributor

@pheuter I gave it a shot in #9725. Seems to be working. I copied most of your code and added the call to $watchCollection and docs.

@pheuter
Copy link
Author

pheuter commented Oct 21, 2014

@gabrielmaldi If it works, I can make the changes in this pull request, as it already contains various comments and background info.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants