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

fix(ivy): match attribute selectors for content projection with inline-templates #29041

Closed
wants to merge 7 commits into
base: master
from

Conversation

@petebacondarwin
Copy link
Member

petebacondarwin commented Feb 28, 2019

See FW-1002

Supercedes #28655

@ngbot ngbot bot added this to the needsTriage milestone Feb 28, 2019

@googlebot googlebot added the cla: yes label Feb 28, 2019

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:FW-1002b branch 2 times, most recently from 1d9efaa to 6f6ae43 Mar 1, 2019

@ngbot ngbot bot modified the milestones: needsTriage, Backlog Mar 3, 2019

@petebacondarwin petebacondarwin marked this pull request as ready for review Mar 3, 2019

@petebacondarwin petebacondarwin requested review from angular/fw-compiler as code owners Mar 3, 2019

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:FW-1002b branch 2 times, most recently from 053174d to 7e23900 Mar 3, 2019

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:FW-1002b branch 4 times, most recently from 14ee11b to 2d159ca Mar 3, 2019

@kara
Copy link
Contributor

kara left a comment

Approach looks great, just minor comments

Show resolved Hide resolved packages/compiler/src/core.ts Outdated
Show resolved Hide resolved packages/core/src/render3/node_selector_matcher.ts
Show resolved Hide resolved packages/core/src/render3/interfaces/node.ts Outdated
Show resolved Hide resolved packages/core/src/render3/interfaces/node.ts
Show resolved Hide resolved packages/core/src/render3/interfaces/node.ts Outdated
Show resolved Hide resolved packages/core/src/render3/di.ts Outdated
Show resolved Hide resolved packages/core/src/render3/node_selector_matcher.ts Outdated
Show resolved Hide resolved packages/core/src/render3/node_selector_matcher.ts Outdated
Show resolved Hide resolved packages/core/src/render3/node_selector_matcher.ts Outdated

petebacondarwin added some commits Mar 7, 2019

fix(compiler): ensure template is updated if an output is transformed
No idea where the tests for this code are, but it looks like this was an
oversight in the original commit 6a0f78.
refactor: rename `matchingSelectorIndex` to `matchingProjectionSelect…
…orIndex`

The previous name was ambiguous as there are different strategies
for matching selectors depending upon the scenario.
refactor(ivy): define new `AttributeMarker.Template` marker
This commit adds a new `AttributeMarker` type that will be used, in a
future commit, to mark attributes as coming from an inline-template
expansion, rather than the element that is being contained in the template.
fix(ivy): teach template type checker about template attributes
For the template type checking to work correctly, it needs to know
what attributes are bound to expressions or directives, which may
require expressions in the template to be evaluated in a different
scope.

In inline templates, there are attributes that are now marked as
"Template" attributes. We need to ensure that the template
type checking code looks at these "bound" attributes as well as the
"input" attributes.
fix(ivy): match attribute selectors for content projection with inlin…
…e-templates

The content projection mechanism is static, in that it only looks at the static
template nodes before directives are matched and change detection is run.
When you have a selector-based content projection the selection is based
on nodes that are available in the template.

For example:

```
<ng-content selector="[some-attr]"></ng-content>
```

would match

```
<div some-attr="..."></div>
```

If you have an inline-template in your projected nodes. For example:

```
<div *ngIf="..." some-attr="..."></div>
```

This gets pre-parsed and converted to a canonical form.

For example:

```
<ng-template [ngIf]="...">
  <div some-attr=".."></div>
</ng-template>
```

Note that only structural attributes (e.g. `*ngIf`) stay with the `<ng-template>`
node. The other attributes move to the contained element inside the template.

When this happens in ivy, the ng-template content is removed
from the component template function and is compiled into its own
template function. But this means that the information about the
attributes that were on the content are lost and the projection
selection mechanism is unable to match the original
`<div *ngIf="..." some-attr>`.

This commit adds support for this in ivy. Attributes are separated into three
groups (Bindings, Templates and "other"). For inline-templates the Bindings
and "other" types are hoisted back from the contained node to the `template()`
instruction, so that they can be used in content projection matching.

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:FW-1002b branch from de06e79 to bc63228 Mar 7, 2019

@petebacondarwin petebacondarwin requested a review from alxhub Mar 7, 2019

@kara kara assigned alxhub and unassigned petebacondarwin Mar 7, 2019

@alxhub

alxhub approved these changes Mar 7, 2019

@alxhub

This comment has been minimized.

Copy link
Contributor

alxhub commented Mar 7, 2019

@kara kara unassigned alxhub Mar 7, 2019

@kara kara closed this in b73e020 Mar 7, 2019

kara added a commit that referenced this pull request Mar 7, 2019

fix(compiler): ensure template is updated if an output is transformed (
…#29041)

No idea where the tests for this code are, but it looks like this was an
oversight in the original commit 6a0f78.

PR Close #29041

kara added a commit that referenced this pull request Mar 7, 2019

kara added a commit that referenced this pull request Mar 7, 2019

refactor(ivy): define new `AttributeMarker.Template` marker (#29041)
This commit adds a new `AttributeMarker` type that will be used, in a
future commit, to mark attributes as coming from an inline-template
expansion, rather than the element that is being contained in the template.

PR Close #29041

kara added a commit that referenced this pull request Mar 7, 2019

fix(ivy): match attribute selectors for content projection with inlin…
…e-templates (#29041)

The content projection mechanism is static, in that it only looks at the static
template nodes before directives are matched and change detection is run.
When you have a selector-based content projection the selection is based
on nodes that are available in the template.

For example:

```
<ng-content selector="[some-attr]"></ng-content>
```

would match

```
<div some-attr="..."></div>
```

If you have an inline-template in your projected nodes. For example:

```
<div *ngIf="..." some-attr="..."></div>
```

This gets pre-parsed and converted to a canonical form.

For example:

```
<ng-template [ngIf]="...">
  <div some-attr=".."></div>
</ng-template>
```

Note that only structural attributes (e.g. `*ngIf`) stay with the `<ng-template>`
node. The other attributes move to the contained element inside the template.

When this happens in ivy, the ng-template content is removed
from the component template function and is compiled into its own
template function. But this means that the information about the
attributes that were on the content are lost and the projection
selection mechanism is unable to match the original
`<div *ngIf="..." some-attr>`.

This commit adds support for this in ivy. Attributes are separated into three
groups (Bindings, Templates and "other"). For inline-templates the Bindings
and "other" types are hoisted back from the contained node to the `template()`
instruction, so that they can be used in content projection matching.

PR Close #29041

kara added a commit that referenced this pull request Mar 7, 2019

kara added a commit that referenced this pull request Mar 7, 2019

fix(ivy): teach template type checker about template attributes (#29041)
For the template type checking to work correctly, it needs to know
what attributes are bound to expressions or directives, which may
require expressions in the template to be evaluated in a different
scope.

In inline templates, there are attributes that are now marked as
"Template" attributes. We need to ensure that the template
type checking code looks at these "bound" attributes as well as the
"input" attributes.

PR Close #29041
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.