Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ContentChildren query lags in change detection when wrapped by template #38976

Open
waterplea opened this issue Sep 24, 2020 · 7 comments
Open
Assignees
Labels
area: core Issues related to the framework runtime core: change detection core: content projection core: queries P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent state: confirmed
Milestone

Comments

@waterplea
Copy link
Contributor

waterplea commented Sep 24, 2020

🐞 bug report

Affected Package

The issue is caused by package @angular/core probably

Is this a regression?

No, the issue was present at least since version 7

Description

When your component has ng-template in its content for later instantiation and inside that template there's a list of items — you can query those children with ContentChildren once you instantiate the template. However change detection in them lags a cycle.

🔬 Minimal Reproduction

https://stackblitz.com/edit/angular-content-children-query-bug

🌍 Your Environment

Angular Version:

10

Anything else relevant?
In the reproduction Stackblitz, type a short string into input.
This string is then broken into chars and spread into a list of DIVs with a 2 seconds delay.
You can even see that "content checked" is printed to console when the list updates.
However the QueryList of children is not refreshed.
Also note that I had to wrap setInterval into runOutsideAngular.
Otherwise it would create a tick that refreshes the query, even though both components are OnPush.

@AndrewKushnir AndrewKushnir added the area: core Issues related to the framework runtime label Sep 24, 2020
@ngbot ngbot bot added this to the needsTriage milestone Sep 24, 2020
@waterplea
Copy link
Contributor Author

Cannot think of any workarounds :( Maybe there's a way to manually kick it to refresh? At least with some private API, it's blocking me at the moment :(

@Achilles1515
Copy link

Achilles1515 commented Sep 28, 2020

If you have access to the component the template is defined in (AppComponent in this case) and set the change detection to Default, instead of OnPush, then it appears to work as expected. So I think some part of this issue has to do with the template defined view being OnPush/CheckOnce.

Also note that I had to wrap setInterval into runOutsideAngular.
Otherwise it would create a tick that refreshes the query, even though both components are OnPush.

I don't think this is the case.

If I comment out the runOutsideAngular like this:

export class HelloComponent {
  //...

  constructor(ngZone: NgZone) {
    // ngZone.runOutsideAngular(() => {
      setInterval(() => {
        console.log(this.children.length);
      }, 1000);
    // });
  }
  //...
}

...and type something in quickly, two seconds later after the observable emits I am still seeing 0 being reported as the content children length (0 until I type something else in).

Something is definitely up here.

@waterplea
Copy link
Contributor Author

You are right about the zone. For some reason it did trigger it in my app where I originally stumbled upon it. Switching to Default is not an option for me. And since you see in console content is being checked, change detection should notice this in OnPush as well.

@waterplea
Copy link
Contributor Author

waterplea commented Sep 29, 2020

Even changeDetectorRef.detectChanges() on the wrapping view doesn't seem to help. I suppose because content is technically a part of parent view.

EDIT: Ok, I've managed to find a workaround that unblocked me. I've added a directive to template and injected ChangeDetectorRef of the parent view. And when I need to recalculate ContentChildren I can trigger its detectChanges. Still the issues exists, would be great to have it working as is.

@Achilles1515
Copy link

Achilles1515 commented Sep 29, 2020

Adding:

ngDoCheck() {
    this._cref.detectChanges();
}

to HelloComponent works. Very ugly though.
If you wanted to drop into private APIs (Ivy) you could add a check to only detect changes if the component view is marked dirty, which should work for this specific example because the async pipe is marking the view dirty.

ngDoCheck() {
  const isDirty = (this as any).__ngContext__[2] & 0b000001000000;
  if (isDirty) {
    this._cref.detectChanges();
  }    
}

But if we're talking about private APIs...then there may be a more performant solution.
Queries are in dire need of work.

Here is a modified StackBlitz that adds the same query for "test" elements to the parent view (but, ViewChildren now), which more explicitly illustrates the problem.

The ViewChildren and ContentChildren queries are matching the same elements, but type something in the input and two seconds later they will have different queryList.length values as reported in the console.

@jelbourn
Copy link
Member

jelbourn commented Oct 19, 2020

@atscott is this potentially related transplanted view change detection?

@atscott
Copy link
Contributor

atscott commented Oct 20, 2020

@jelbourn Yea, this is related to transplanted views. Ironically, there was a short period of time that this actually worked (change the version to 9.1.0 in the demo), but it worked in a way that broke other scenario (Transplanted views would get refreshed twice if both insertion and declaration are dirty. This could be an error if the changes in the insertion component result in data not being available to the transplanted view because it is slated to be removed.).

Issue summary:

  • The ng-template is what we call a "transplanted view" - it's defined in one component and inserted into another
  • We refresh transplanted views at their insertion points only. In this example, the content of the ng-template is refreshed as part of CD for HelloComponent.
  • Content queries are refreshed as part of the declaration component CD. Here, that means the content query gets refreshed when AppComponent gets refreshed. This happens before child components are refreshed. That means we're refreshing the content query before the new items are present.

My guess is that the implementation of content queries didn't consider that the ng-content might have transplanted views. I would also guess that fixing this would end up being a breaking change. The content query results should be available before executing the template of the insertion component or we could otherwise run into ExpressionChangedAfterItWasChecked issues.

Just a small note that the behavior is quite a departure from what's expected. The act of executing the HelloComponent template changes the ng-content template. Generally, we would expect this not to happen -- the ng-content is intended to be updated as part of the declaration component (this is always the case unless there's a transplanted view in it like this example).

Simpler repro without the intervals: https://stackblitz.com/edit/angular-content-children-query-bug-vjaxd3?file=src%2Fapp%2Fapp.component.html

@atscott atscott added core: change detection core: content projection P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent labels Oct 20, 2020
@ngbot ngbot bot modified the milestones: needsTriage, Backlog Oct 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: core Issues related to the framework runtime core: change detection core: content projection core: queries P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent state: confirmed
Projects
None yet
Development

No branches or pull requests

6 participants