-
Notifications
You must be signed in to change notification settings - Fork 27.4k
Fix(ng:repeat,ng:controller): Change priority order of ng:repeat and add priority to ng:controller #4466
Conversation
Thanks for the PR!
If you need to make changes to your pull request, you can update the commit with Thanks again for your help! |
Is this by any chance related to this: https://groups.google.com/forum/#!searchin/angular/ng-Include%7Csort:date/angular/umoMPe1Wl84/rX8X5g-VhoUJ ? |
This appears to be related to #4521 |
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. |
@caitp, will do. |
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. |
I'll amend with tests to cover as many scenarios as possible. Keeping existing functionality. On Monday, November 4, 2013, damiengenet wrote:
|
@@ -192,7 +192,6 @@ | |||
if (IGNORE[prop] || prop.match(/^moz[A-Z]/)) { //skip special variables which keep on changing | |||
continue; | |||
} else if (!globalVars.hasOwnProperty(varKey)) { | |||
//console.log('new global variable found: ', prop); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove
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 |
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. |
@btford, look at the comments from Peter, Caitlin and the comments on the bugs themselves |
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. |
@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. |
Just getting off work, I'll rebase and check this out On Wednesday, November 20, 2013, Brian Ford wrote:
|
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
If all the tests pass, I think we can merge db977995ede129416104168ec2228012085a3e8b.Thanks for your help and patience! |
Landed as 6288cf5. Thanks @asilluron, @caitp, and everyone else that helped report/discuss this issue. |
Ensure priority is given to ng:controller when using ng:include/ng:repeat in conjuction with ng:controller.
Closes #4431
Closes #4521