Fix(ng:repeat,ng:controller): Change priority order of ng:repeat and add priority to ng:controller

[asilluron]

Ensure priority is given to ng:controller when using ng:include/ng:repeat in conjuction with ng:controller.

Closes #4431
Closes #4521

[mary-poppins]

Thanks for the PR!

• Contributor signed CLA now or in the past
  • If you just signed, leave a comment here with your real name
• PR's commit messages follow the commit message format

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!

[dgieselaar]

Is this by any chance related to this: https://groups.google.com/forum/#!searchin/angular/ng-Include%7Csort:date/angular/umoMPe1Wl84/rX8X5g-VhoUJ ?

[wizardwerdna]

This appears to be related to #4521

[caitp]

It looks like this patch solves the issue mentioned by wizardwerdna@, and presumably the ngRepeat thing too. So, it might be a good idea to write some tests, just in case it breaks again in the future at some point. Matching the content of interpolated text nodes should be a pretty simple way to do it.

[asilluron]

@caitp, will do.

[damiengenet]

This patch fixes the case where both ng-include and ng-controller are on the same element, but breaks the case where ng-repeat and ng-controller are used together for me. In < 1.2.0-rc2 a new controller would be created for each item in the collection and would be initialized with the item sub-scope. Actually keeping the new 1000 priority for ngRepeatDirective and setting ngControllerDirective to 900 seems to work in both cases for me, but I'm not sure this covers all possible combinations.

[asilluron]

I'll amend with tests to cover as many scenarios as possible.

Keeping existing functionality.

On Monday, November 4, 2013, damiengenet wrote:

This patch fixes the case where both ng-include and ng-controller are on
the same element, but breaks the case where ng-repeat and ng-controller are
used together for me. In < 1.2.0-rc2 a new controller would be created for
each item in the collection and would be initialized with the item
sub-scope. Actually keeping the new 1000 priority for ngRepeatDirective and
setting ngControllerDirective to 900 seems to work in both cases for me,
but I'm not sure this covers all possible combinations.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/4466#issuecomment-27738608
.

[petebacondarwin]

Remove

[caitp]

http://plnkr.co/edit/IaJlAK6evZYF4sj8UQPY?p=preview some tests (not terribly exhaustive), but it seems that if the priority is beneath 400, it does not play nicely with ngInclude. On 400, it only beats ngInclude because of its name, so maybe that's not very good. If it's too high it seems to not play well with ng-switch.

So assuming the actual goal is to re-instantiate a controller every time an item becomes visible, and actually ensure the item is visible, 500 seems like a happy medium. But like I said it's not an exhaustive set of tests, I wouldn't want to miss any obvious use cases

[btford]

When rebased against current master, this doesn't change the behavior of ngController + ngRepeat at all. Your test still passes if you revert the change to ngController's priority.

For ngController + ngInclude, this fix doesn't work. It's not as simple as changing the order of directives, there are some changes you'd have to make in the compiler and how it handles scopes for elements with multiple directives and element transclusion.

[asilluron]

@btford, look at the comments from Peter, Caitlin and the comments on the bugs themselves

[caitp]

There are definitely differences in behaviour based on different priorities, and ngRepeat + ngController does seem to work.

The only thing that we're having problems with (based on my tests) is ngSwitch + ngInclude, and I'm not sure it's worth trying to fix that, since transclusion is crazy enough already.

[btford]

@asilluron @caitp try rebasing this against master and re-running your tests. There have been other changes since this PR was conceived that might affect this.

Looks like I was mistaken about ngController + ngInclude; this fix is still legit for that case.

[asilluron]

Just getting off work, I'll rebase and check this out

On Wednesday, November 20, 2013, Brian Ford wrote:

@asilluron https://github.com/asilluron @caitphttps://github.com/caitptry rebasing this against master and re-running your tests. There have been
other changes since this PR was conceived that affect this.

Changing the priority is no longer sufficient to fixing the ngController +
ngInclude case.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/4466#issuecomment-28949169
.

[btford]

I really want this to land, so I've done some work on top of your commit and pushed to one of my branches: btford@db97799

1. I removed the test for ngRepeat. That might be a useful test, but this change has nothing to do with it anymore, since a previous change seems to have already resolved that. We could still add such a test to track regressions, but that should be in a separate commit, IMO.
2. I cleaned up the test.
3. I fixed the commit message.

If all the tests pass, I think we can merge db977995ede129416104168ec2228012085a3e8b.Thanks for your help and patience!

[btford]

Landed as 6288cf5.

Thanks @asilluron, @caitp, and everyone else that helped report/discuss this issue.