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

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

Closed
wants to merge 1 commit into from

Conversation

asilluron
Copy link
Contributor

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

Closes #4431
Closes #4521

@mary-poppins
Copy link

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
Copy link

@wizardwerdna
Copy link
Contributor

This appears to be related to #4521

@caitp
Copy link
Contributor

caitp commented Oct 19, 2013

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
Copy link
Contributor Author

@caitp, will do.

@damiengenet
Copy link

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
Copy link
Contributor Author

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
.

@@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove

@caitp
Copy link
Contributor

caitp commented Nov 16, 2013

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

@ghost ghost assigned btford Nov 20, 2013
@btford
Copy link
Contributor

btford commented Nov 21, 2013

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
Copy link
Contributor Author

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

@caitp
Copy link
Contributor

caitp commented Nov 21, 2013

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
Copy link
Contributor

btford commented Nov 21, 2013

@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
Copy link
Contributor Author

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
Copy link
Contributor

btford commented Nov 21, 2013

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
Copy link
Contributor

btford commented Nov 21, 2013

Landed as 6288cf5.

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

@btford btford closed this Nov 21, 2013
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
8 participants