fix(core): fix `Self` flag inside embedded views with custom injectors #50270

garrettld · 2023-05-12T19:30:26Z

When an embedded view injector is present anywhere above a node in the tree, the Self flag was effectively ignored. With this change, embedded view injectors are not checked at all when the Self flag is present, because resolution should stop at the current node before reaching any embedded view injector(s).

Fixes #49959

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/main/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

@Self is ignored when using createEmbeddedView with a custom injector.

Issue Number: #49959

What is the new behavior?

@Self is no longer ignored when using createEmbeddedView with a custom injector.

Does this PR introduce a breaking change?

Yes
No

Other information

I initially made a change to lookupTokenUsingEmbeddedInjector so that it would bail out after checking the current node if the Self flag was set, but I think skipping the embedded view injector lookup entirely makes more sense, and it avoids a case where the current node injector would be checked twice when both the Self and Optional flags are set.

I called this a "fix", but I could see how someone could've become reliant on this behavior in the last year or so since this feature was released. Is that enough to call this a breaking change?

I'm not certain about this, but I think there might be a similar issue with the Host flag and embedded view injectors. I don't see any special handling of the Host flag in the lookupTokenUsingEmbeddedInjector function, so I assume that if you created a hierarchy like hostNode -> embeddedView -> hostNode -> embeddedView -> node and used @Host on that last node, it would reach all the way up to the top node (past its "host") to find the token it was looking for. I ran out of time to look into this today, but I can take a look next week.

When an embedded view injector is present anywhere above a node in the tree, the `Self` flag was effectively ignored. With this change, embedded view injectors are not checked at all when the `Self` flag is present, because resolution should stop at the current node before reaching any embedded view injector(s). Fixes angular#49959

garrettld · 2023-05-12T20:25:22Z

I've created a reproduction of the bug that this fixes here: https://stackblitz.com/edit/angular-1fsxct?file=src/main.ts

garrettld · 2023-05-20T00:44:57Z

Following up on this, there does also appear to be an issue with the Host flag as I suspected. I've thrown together a number of reproductions here: https://stackblitz.com/edit/angular-dr2c75?file=src%2Fmain.ts

crisbeto · 2023-05-22T07:35:18Z

packages/core/src/render3/di.ts

+    if (lView[FLAGS] & LViewFlags.HasEmbeddedViewInjector &&
+        // The token must be present on the current node injector when the `Self`
+        // flag is set, so the lookup on embedded view injector(s) can be skipped.
+        !(flags & InjectFlags.Self)) {


Wouldn't we still have to check the embedded inject if it's on the current view? The HasEmbeddedViewInjector flag indicates that an embedded injector exists either on the current view or one of its ancestors.

It's very possible that I'm missing something here, but my understanding is that the embedded view injector must be "above" the current element injector in the injector hierarchy, and therefore cannot be the current element injector.

Since Self means we only check the current element injector, we can skip embedded view injector checks altogether when it's set.

crisbeto

LGTM

jessicajaniuk · 2023-05-24T13:59:25Z

This PR was merged into the repository by commit 75fdbcb.

#50270) When an embedded view injector is present anywhere above a node in the tree, the `Self` flag was effectively ignored. With this change, embedded view injectors are not checked at all when the `Self` flag is present, because resolution should stop at the current node before reaching any embedded view injector(s). Fixes #49959 PR Close #50270

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@angular/animations](https://github.com/angular/angular) | dependencies | patch | [`16.0.2` -> `16.0.4`](https://renovatebot.com/diffs/npm/@angular%2fanimations/16.0.2/16.0.4) | | [@angular/common](https://github.com/angular/angular) | dependencies | patch | [`16.0.2` -> `16.0.4`](https://renovatebot.com/diffs/npm/@angular%2fcommon/16.0.2/16.0.4) | | [@angular/compiler](https://github.com/angular/angular) | dependencies | patch | [`16.0.2` -> `16.0.4`](https://renovatebot.com/diffs/npm/@angular%2fcompiler/16.0.2/16.0.4) | | [@angular/compiler-cli](https://github.com/angular/angular/tree/main/packages/compiler-cli) ([source](https://github.com/angular/angular)) | devDependencies | patch | [`16.0.2` -> `16.0.4`](https://renovatebot.com/diffs/npm/@angular%2fcompiler-cli/16.0.2/16.0.4) | | [@angular/core](https://github.com/angular/angular) | dependencies | patch | [`16.0.2` -> `16.0.4`](https://renovatebot.com/diffs/npm/@angular%2fcore/16.0.2/16.0.4) | | [@angular/forms](https://github.com/angular/angular) | dependencies | patch | [`16.0.2` -> `16.0.4`](https://renovatebot.com/diffs/npm/@angular%2fforms/16.0.2/16.0.4) | | [@angular/platform-browser](https://github.com/angular/angular) | dependencies | patch | [`16.0.2` -> `16.0.4`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser/16.0.2/16.0.4) | | [@angular/platform-browser-dynamic](https://github.com/angular/angular) | dependencies | patch | [`16.0.2` -> `16.0.4`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser-dynamic/16.0.2/16.0.4) | --- ### Release Notes <details> <summary>angular/angular</summary> ### [`v16.0.4`](https://github.com/angular/angular/blob/HEAD/CHANGELOG.md#1604-2023-06-01) [Compare Source](angular/angular@16.0.3...16.0.4) ##### animations | Commit | Type | Description | | -- | -- | -- | | [df65c4fc8f](angular/angular@df65c4f) | fix | Trigger leave animation when ViewContainerRef is injected ([#48705](angular/angular#48705)) | ##### common | Commit | Type | Description | | -- | -- | -- | | [7e1bc513de](angular/angular@7e1bc51) | fix | untrack subscription and unsubscription in async pipe ([#50522](angular/angular#50522)) | ##### core | Commit | Type | Description | | -- | -- | -- | | [9970b29ace](angular/angular@9970b29) | fix | update `ApplicationRef.isStable` to account for rendering pending tasks ([#50425](angular/angular#50425)) |  ### [`v16.0.3`](https://github.com/angular/angular/blob/HEAD/CHANGELOG.md#1603-2023-05-24) [Compare Source](angular/angular@16.0.2...16.0.3) ##### core | Commit | Type | Description | | -- | -- | -- | | [c11041e372](angular/angular@c11041e) | fix | adds missing symbols for animation standalone bundling test ([#50434](angular/angular#50434)) | | [98e8fdf40e](angular/angular@98e8fdf) | fix | fix `Self` flag inside embedded views with custom injectors ([#50270](angular/angular#50270)) | | [199ff4fe7f](angular/angular@199ff4f) | fix | host directives incorrectly validating aliased bindings ([#50364](angular/angular#50364)) | ##### http | Commit | Type | Description | | -- | -- | -- | | [080bbd2137](angular/angular@080bbd2) | fix | create macrotask during request handling instead of load start ([#50406](angular/angular#50406)) |  </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1915 Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>

angular-automatic-lock-bot · 2023-07-09T00:11:29Z

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#50270) When an embedded view injector is present anywhere above a node in the tree, the `Self` flag was effectively ignored. With this change, embedded view injectors are not checked at all when the `Self` flag is present, because resolution should stop at the current node before reaching any embedded view injector(s). Fixes angular#49959 PR Close angular#50270

pullapprove bot requested a review from dylhunn May 12, 2023 19:30

garrettld mentioned this pull request May 12, 2023

bug(mat-bottom-sheet): custom form validation is executed multiple times #49959

Closed

1 task

AndrewKushnir requested a review from crisbeto May 12, 2023 20:05

AndrewKushnir added the area: core Issues related to the framework runtime label May 17, 2023

ngbot bot added this to the Backlog milestone May 17, 2023

garrettld mentioned this pull request May 17, 2023

The "Hierarchical injectors" page describes a white flower emoji as yellow on Windows #50347

Closed

crisbeto reviewed May 22, 2023

View reviewed changes

crisbeto approved these changes May 24, 2023

View reviewed changes

crisbeto added the target: patch This PR is targeted for the next patch release label May 24, 2023

crisbeto removed the request for review from dylhunn May 24, 2023 07:36

crisbeto added the action: merge The PR is ready for merge by the caretaker label May 24, 2023

jessicajaniuk closed this in 75fdbcb May 24, 2023

garrettld deleted the embedded-view-injectors branch June 8, 2023 16:52

mishahrokhola mentioned this pull request Jun 9, 2023

bug(dialog): custom reactive form control in a dialog created with ng-template throw error angular/components#25515

Open

1 task

angular-automatic-lock-bot bot locked and limited conversation to collaborators Jul 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(core): fix `Self` flag inside embedded views with custom injectors #50270

fix(core): fix `Self` flag inside embedded views with custom injectors #50270

Uh oh!

garrettld commented May 12, 2023 •

edited

Loading

Uh oh!

garrettld commented May 12, 2023 •

edited

Loading

Uh oh!

garrettld commented May 20, 2023

Uh oh!

crisbeto May 22, 2023

Uh oh!

garrettld May 22, 2023 •

edited

Loading

Uh oh!

crisbeto left a comment

Uh oh!

jessicajaniuk commented May 24, 2023

Uh oh!

angular-automatic-lock-bot bot commented Jul 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fix(core): fix Self flag inside embedded views with custom injectors #50270

fix(core): fix Self flag inside embedded views with custom injectors #50270

Uh oh!

Conversation

garrettld commented May 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

garrettld commented May 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

garrettld commented May 20, 2023

Uh oh!

crisbeto May 22, 2023

Choose a reason for hiding this comment

Uh oh!

garrettld May 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crisbeto left a comment

Choose a reason for hiding this comment

Uh oh!

jessicajaniuk commented May 24, 2023

Uh oh!

angular-automatic-lock-bot bot commented Jul 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fix(core): fix `Self` flag inside embedded views with custom injectors #50270

fix(core): fix `Self` flag inside embedded views with custom injectors #50270

garrettld commented May 12, 2023 •

edited

Loading

garrettld commented May 12, 2023 •

edited

Loading

garrettld May 22, 2023 •

edited

Loading