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

[ivy] ContentChildren doesn't pick up elements inside ng-container #34768

Closed
henry-alakazhang opened this issue Jan 14, 2020 · 9 comments
Closed
Labels
area: core Issues related to the framework runtime regression Indicates than the issue relates to something that worked in a previous version

Comments

@henry-alakazhang
Copy link

henry-alakazhang commented Jan 14, 2020

🐞 bug report

Affected Package

The issue is caused by package @angular/core@9.0.0-rc.8

Is this a regression?

Yes, the previous version in which this bug was not present was: v8

Description

ContentChildren doesn't pick up things marked with ngProjectAs.

<app-parent name="indirect with projectAs">
  <ng-container *ngFor="let i of [1, 2, 3, 4]" ngProjectAs="app-child">
    <app-child>
      {{ i }}
    </app-child>
  </ng-container>
</app-parent>
...
export class ParentComponent {
  @ContentChildren(ChildComponent, { descendants: false }) children: QueryList<
    ChildComponent
  >;
}

In v8, the ContentChildren in the ParentComponent would pick up the app-child inside the ng-container (technically, it did this regardless of ngProjectAs, but the point is it did work). In Ivy, it no longer picks up the children with or without an ngProjectAs. Given <ng-content select=*> does work with ngProjectAs, it seems to me that ContentChildren should as well.

🔬 Minimal Reproduction

https://github.com/henry-alakazhang/angular-content-children-repro
Master branch is v9, v8 branch is v8.
https://github.com/henry-alakazhang/angular-content-children-repro/tree/v8

🌍 Your Environment

Angular Version:


Package                           Version
-----------------------------------------------------------
@angular-devkit/architect         0.900.0-rc.8
@angular-devkit/build-angular     0.900.0-rc.8
@angular-devkit/build-optimizer   0.900.0-rc.8
@angular-devkit/build-webpack     0.900.0-rc.8
@angular-devkit/core              9.0.0-rc.8
@angular-devkit/schematics        9.0.0-rc.8
@angular/localize                 9.0.0-rc.0
@ngtools/webpack                  9.0.0-rc.8
@schematics/angular               9.0.0-rc.8
@schematics/update                0.900.0-rc.8
rxjs                              6.5.4
typescript                        3.6.4
webpack                           4.41.2

Anything else relevant?
Related to #34289, but the suggested solution (descendants: true) is not always feasible. In my case, I want to be able to nest another parent inside the child and using descendants: true would pick up the nested children as well.

Similarly, the other suggestion in the compatibility guide, to only use direct descendants, is difficult to use if I want to apply multiple structural directives to the child, eg.

<app-parent>
  <ng-container *ngFor="...">
    <app-child *ngIf="...">
  </ng-container>
</app-parent>
@AndrewKushnir AndrewKushnir added area: core Issues related to the framework runtime comp: ivy labels Jan 14, 2020
@ngbot ngbot bot added this to the needsTriage milestone Jan 14, 2020
@crisbeto
Copy link
Member

From what I can tell the issue isn't due to ngProjectAs, but as you mentioned, that you have an extra ng-container between the parent and the child.

@pkozlowski-opensource pkozlowski-opensource changed the title [ivy] ContentChildren doesn't pick up elements with ngProjectAs [ivy] ContentChildren doesn't pick up elements inside ng-container Jan 14, 2020
@pkozlowski-opensource
Copy link
Member

pkozlowski-opensource commented Jan 14, 2020

@henry-alakazhang as commented by @crisbeto I believe that ngProjectAs attribute is not relevant here (and I've changed the title accordingly). What matters here is the presence of <ng-container> that, with ngIvy, acts as any regular element (wrt the descendants option for queries). In this sense this is a known breaking change in Angular + ngIvy.

Having said this, it is very easy to get the correct behaviour without using <ng-container> (which, btw, would be slightly more performant and would result in less generated code => smaller bundles).

Given your first example you could do (shorter and ngProjectAs might not be needed at all!):

<app-parent name="indirect with projectAs">
    <app-child *ngFor="let i of [1, 2, 3, 4]" >
      {{ i }}
    </app-child>
</app-parent>

If you want to have a group of sibling elements then you can use the de-sugared version of ngIf / ngFor:

<app-parent>
  <ng-template ngFor="..." [ngForOf]="...">
    <app-child *ngIf="...">
  </ng-template>
</app-parent>

Queries with descendants: false will work correctly in both VE and ivy using the above forms. @henry-alakazhang could you give it a try and let me know if this works for you? Thnx!

We are still debating what to do with <ng-container> and queries but as of today the solution outlined above is the best approach.

@Airblader
Copy link
Contributor

I'm aware of what has been said above, but just to point something else out: respecting ngProjectAs for the ContentChild(ren) queries would lead to the odd question of what to populate the result with, since it's matched by something that isn't actually that component. So even if it worked, it wouldn't actually give you what you'd expect.

@pkozlowski-opensource
Copy link
Member

@Airblader I'm sorry but I don't understand the use-case that you are describing... Could you put it in a stackblitz ? It looks like you are seeing something I'm not seeing...

@Airblader
Copy link
Contributor

I was just describing that the premise of this issue — that ContentChildren should work with ngProjectAs — is a flawed idea because even if it was supposed to work (which it isn't), the query list couldn't possibly contain instances of that component since what was actually matched was a completely other element.

@henry-alakazhang
Copy link
Author

Hey, thanks for the response. The de-sugared version does work, although it's a bit more complex than I'd like. Large-ish organisation; having to teach people to use the de-sugared version in a single use case; extra training and mental overhead; yada yada yada.

I do feel like the ngProjectAs is somewhat relevant though. It seems like it would make sense if ContentChildren(...) and ng-content select="..." picked up the same children. ng-content doesn't select through ng-container unless you use ngProjectAs, so I feel like ContentChildren should potentially be able to do the same thing. I could just be missing something about ContentChildren and ng-content though.

@Airblader
Copy link
Contributor

Airblader commented Jan 16, 2020

That is exactly my point above, though: conceptually this gets tricky. Consider this:

<app-parent>
    <app-child-a ngProjectAs="app-child-b"></app-child-a>
</app-parent>

@Component({ selector: "app-parent" })
export class AppParentComponent {
    @ContentChildren(AppChildAComponent) childrenA: QueryList<AppChildAComponent>;
    @ContentChildren(AppChildBComponent) childrenB: QueryList<AppChildBComponent>;
}

Which query list would be filled – and more importantly: with what? Logically consistent would be if childrenB was filled but with instances of AppChildAComponent.

No instance of AppChildBComponent exists anywhere, but AppChildAComponent projects itself as one. So actually you'd have to use it like this:

@Component({ selector: "app-parent" })
export class AppParentComponent {
    @ContentChildren(AppChildBComponent) children: QueryList<AppChildAComponent>;
}

which looks really odd, doesn't it?

@henry-alakazhang
Copy link
Author

Hmm, yeah, I see your point now. It's fine for ng-content select="*" since it just projects and doesn't really care about what you're giving it, but it wouldn't really work for ContentChildren.

@kara kara modified the milestones: needsTriage, v10-candidates Jan 27, 2020
@alxhub alxhub added regression Indicates than the issue relates to something that worked in a previous version and removed severity5: ivy-compat labels Feb 10, 2020
pkozlowski-opensource added a commit to pkozlowski-opensource/angular that referenced this issue Feb 12, 2020
pkozlowski-opensource added a commit to pkozlowski-opensource/angular that referenced this issue Feb 13, 2020
pkozlowski-opensource added a commit to pkozlowski-opensource/angular that referenced this issue Feb 13, 2020
pkozlowski-opensource added a commit to pkozlowski-opensource/angular that referenced this issue Feb 13, 2020
pkozlowski-opensource added a commit to pkozlowski-opensource/angular that referenced this issue Feb 13, 2020
pkozlowski-opensource added a commit to pkozlowski-opensource/angular that referenced this issue Feb 13, 2020
pkozlowski-opensource added a commit to pkozlowski-opensource/angular that referenced this issue Feb 18, 2020
pkozlowski-opensource added a commit to pkozlowski-opensource/angular that referenced this issue Feb 18, 2020
…descendants: false option

Before this change content queries with the `descendants: false` option, as implemented in ivy,
would not descendinto `<ng-container>` elements. This behaviour was different from the way the
View Engine worked. This change alligns ngIvy and VE behaviours when it comes to queries and the
`<ng-container>` elements and fixes a common bugs where a query target was placed inside the
`<ng-container>` element with a * directive on it.

Before:

```html
<needs-target>
  <ng-container *ngIf="condition">
    <div #target>...</div>  <!-- this node would NOT match -->
  </ng-container>
</needs-target>
```

After:

```html
<needs-target>
  <ng-container *ngIf="condition">
    <div #target>...</div>  <!-- this node WILL match -->
  </ng-container>
</needs-target>
```

Fixes angular#34768
alxhub pushed a commit that referenced this issue Feb 19, 2020
…descendants: false option (#35384)

Before this change content queries with the `descendants: false` option, as implemented in ivy,
would not descendinto `<ng-container>` elements. This behaviour was different from the way the
View Engine worked. This change alligns ngIvy and VE behaviours when it comes to queries and the
`<ng-container>` elements and fixes a common bugs where a query target was placed inside the
`<ng-container>` element with a * directive on it.

Before:

```html
<needs-target>
  <ng-container *ngIf="condition">
    <div #target>...</div>  <!-- this node would NOT match -->
  </ng-container>
</needs-target>
```

After:

```html
<needs-target>
  <ng-container *ngIf="condition">
    <div #target>...</div>  <!-- this node WILL match -->
  </ng-container>
</needs-target>
```

Fixes #34768

PR Close #35384
@alxhub alxhub closed this as completed in 3f4e02b Feb 19, 2020
danielwiehl added a commit to SchweizerischeBundesbahnen/scion-workbench that referenced this issue Mar 2, 2020
danielwiehl added a commit to SchweizerischeBundesbahnen/scion-workbench that referenced this issue Mar 2, 2020
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Mar 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: core Issues related to the framework runtime regression Indicates than the issue relates to something that worked in a previous version
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants