fix(ngRepeat): ensure expected order of execution for directive lifecycle functions is maintained when they are nested along with ngRepeat

[mihai-tatar]

Request Type: bug

How to reproduce: http://plnkr.co/edit/k09gvqao2oHfIaQfktkd

Component(s): ngRepeat

Impact: small

Complexity: small

This issue is related to:

Detailed Description:

When using directives that are siblings with nested ngRepeat, their lifecycle functions are called out of order ('outer' ones are called before the 'inner' ones) e.g.:

<div foo ng-repeat="...">
  <div bar ng-repeat="..."></div>
<div>

results in the following call order:

foo.compile
bar.compile
foo.controller
foo.preLink
foo.postLink
bar.controller
bar.preLink
bar.postLink

when the expected would be:

foo.compile
bar.compile
foo.controller
foo.preLink
bar.controller
bar.preLink
bar.postLink
foo.postLink

Other Comments:

Ensure that lifecycle functions of directives are called in the expected
order (i.e. compile, controller, preLink, postLink) when using them along
ngRepeat on nested elements.

Previous to this change, lifecycle functions for 'outer' directives were
called before the ones for the 'inner' directives, when ngRepeat was used
on both the 'outer' and the 'inner' elements e.g.:

<div foo ng-repeat="...">
  <div bar ng-repeat="..."></div>
<div>

resulted in the following call order:

foo.compile
bar.compile
foo.controller
foo.preLink
foo.postLink
bar.controller
bar.preLink
bar.postLink

while after applying this change, the order will be:

foo.compile
bar.compile
foo.controller
foo.preLink
bar.controller
bar.preLink
bar.postLink
foo.postLink

Motivation for the change was a means to provide a communication channel
between parent and child directives where both need to be places as a
sibling of ngRepeat.

[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 (#7304)

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!

[caitp]

You should parse rhs and save it to a variable so that it doesn't get parsed twice

[mihai-tatar]

Where is rhs parsed the 2nd time? Do you mean when the directive's postLink function is called again for the same collection?

[mihai-tatar]

@caitp could you please point me in the right direction with the change you proposed?

[caitp]

$scope.$watchCollection(rhs, ngRepeatAction);

It's being parsed there, in $watchCollection.

If you pass the result of $parse(rhs) to $watchCollection, it won't be parsed a second time

[caitp]

we do have a caching strategy which mitigates this issue a bit, but I'd still prefer if we didn't use it for this case, since we don't need to

[mihai-tatar]

@caitp I moved parsing over here and passed the pre-parsed variable to both ngRepeatAction and $watchCollection. Could you please review when you get a chance?

[IgorMinar]

The "out-of-order" behavior exists because the outer repeater creates the inner repeater, which needs to register a $watchCollection watch. The reaction fn for this watcher then links the inner repeater with the inner directive on it. Since all watches in Angular are initialized asynchronously, the inner repeater has to wait until the next iteration of the digest loop before it is fully initialized.

I don't have enough brainpower right now to determine if the fix as proposed is desirable, but if we were to change this, we should change it for all of the structural directives (ngIf, ngSwitch).

cc: @tbosch

[petebacondarwin]

I don't think this is a particularly useful fix. It only solves one specific scenario where compilation "appears" to be out of order due to the asynchronous nature of Angular applications. Others are where you have directives using templateUrl, child elements containing ngInclude.

Trying to rely on synchronous ordering in Angular apps is fraught with problems. What if the data for one of the ngRepeat directives was being loaded asynchronously? Then you would still get out of order calls to linking functions for new child elements that were created in some watch later on.

Instead, we need to educate people to think more asynchronously in their application development in general.

[IgorMinar]

yeah. I agree. there are other situations that would result in linking out of order. in general it is better to let the children know their parents that they are ready if you need parent and children to communicate.

[tbosch]

I agree with @IgorMinar and @petebacondarwin, and we will probably close this PR.
@tatarmihai and @vinayk406 (from the linked issue): could you explain the problem that you trying to solve in your app in more detail? Maybe there is something we are missing here...

[vinayk406]

I faced this issue while working on tabs widget.

Markup:

tab1 content
tab2 content

I expect all the tabs to register themselves with their parent tabset in
the link function. So that, i can work with the child scopes in the link
function of tabset.
When the markup contained ng-repeat, I was not able to access the child
scopes in the link function of tabset.

http://plnkr.co/edit/d1Vzn7qdywVhOgOhGkQc?p=preview

On Tue, Sep 2, 2014 at 9:50 PM, Tobias Bosch notifications@github.com
wrote:

I agree with @IgorMinar https://github.com/IgorMinar and
@petebacondarwin https://github.com/petebacondarwin, and we will
probably close this PR.
@tatarmihai https://github.com/tatarmihai and @vinayk406
https://github.com/vinayk406 (from the linked issue): could you explain
the problem that you trying to solve in your app in more detail? Maybe
there is something we are missing here...

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

[tbosch]

@vinayk406 the tabs can change later in your usecase if the user of the tabs decides to change the collection in ng-repeat (given that ng-repeat does not used a fixed array in the expression by references a property on the scope).

[IgorMinar]

I suggest that you use the require directive option to register the tabs with the tabset, just like we did in the example on our homepage.

I'm going to close this PR as it really it's just a one off solution to a particular case. I understand that the current behavior is non-intuitive but at least it's consistent with the rest of the system. In v2 we are thinking of a different way to deal with the parent-child relationships, but until then the best pattern is for the children to notify the parent that they have been initialized.