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 from

Conversation

petebacondarwin
Copy link
Member

See FW-1002

Supercedes #28655

@petebacondarwin petebacondarwin added state: WIP area: core Issues related to the framework runtime comp: ivy labels Feb 28, 2019
@ngbot ngbot bot added this to the needsTriage milestone Feb 28, 2019
@petebacondarwin petebacondarwin force-pushed the FW-1002b branch 2 times, most recently from 1d9efaa to 6f6ae43 Compare March 2, 2019 22:51
@petebacondarwin petebacondarwin added action: review The PR is still awaiting reviews from at least one requested reviewer type: bug/fix effort2: days freq1: low severity3: broken target: major This PR is targeted for the next major release risk: low and removed state: WIP labels Mar 3, 2019
@ngbot ngbot bot modified the milestones: needsTriage, Backlog Mar 3, 2019
@petebacondarwin petebacondarwin marked this pull request as ready for review March 3, 2019 08:03
@petebacondarwin petebacondarwin requested review from a team as code owners March 3, 2019 08:03
@petebacondarwin petebacondarwin force-pushed the FW-1002b branch 2 times, most recently from 053174d to 7e23900 Compare March 3, 2019 08:11
@petebacondarwin petebacondarwin force-pushed the FW-1002b branch 4 times, most recently from 14ee11b to 2d159ca Compare March 3, 2019 17:49
Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

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

Approach looks great, just minor comments

packages/compiler/src/core.ts Outdated 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 Show resolved Hide resolved
@kara kara added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Mar 5, 2019
@kara kara added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Mar 7, 2019
@kara kara assigned alxhub and unassigned petebacondarwin Mar 7, 2019
@alxhub
Copy link
Member

alxhub commented Mar 7, 2019

Presubmit

@alxhub alxhub added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Mar 7, 2019
@kara kara added the action: presubmit The PR is in need of a google3 presubmit label Mar 7, 2019
@kara kara unassigned alxhub Mar 7, 2019
@kara kara removed the action: presubmit The PR is in need of a google3 presubmit label Mar 7, 2019
@kara kara closed this in b73e020 Mar 7, 2019
kara pushed a commit that referenced this pull request Mar 7, 2019
…#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 pushed a commit that referenced this pull request Mar 7, 2019
kara pushed a commit that referenced this pull request Mar 7, 2019
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 pushed a commit that referenced this pull request Mar 7, 2019
…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 pushed a commit that referenced this pull request Mar 7, 2019
kara pushed a commit that referenced this pull request Mar 7, 2019
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
@petebacondarwin petebacondarwin deleted the FW-1002b branch April 6, 2019 20:07
wKoza pushed a commit to wKoza/angular that referenced this pull request Apr 17, 2019
…orIndex` (angular#29041)

The previous name was ambiguous as there are different strategies
for matching selectors depending upon the scenario.

PR Close angular#29041
wKoza pushed a commit to wKoza/angular that referenced this pull request Apr 17, 2019
…angular#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 angular#29041
wKoza pushed a commit to wKoza/angular that referenced this pull request Apr 17, 2019
wKoza pushed a commit to wKoza/angular that referenced this pull request Apr 17, 2019
…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 angular#29041
wKoza pushed a commit to wKoza/angular that referenced this pull request Apr 17, 2019
…e-templates (angular#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 angular#29041
wKoza pushed a commit to wKoza/angular that referenced this pull request Apr 17, 2019
wKoza pushed a commit to wKoza/angular that referenced this pull request Apr 17, 2019
…lar#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 angular#29041
@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 Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime cla: yes effort2: days freq1: low risk: low target: major This PR is targeted for the next major release type: bug/fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants