feat(core): allow for injector to be specified when creating an embedded view #45156

crisbeto · 2022-02-21T14:15:28Z

Adds support for passing in an optional injector when creating an embedded view through ViewContainerRef.createEmbeddedView and TemplateRef.createEmbeddedView. The injector allows for the DI behavior to be customized within the specific template.

This is a second stab at the changes in #44666. The difference this time is that the new injector acts as a node injector, rather than a module injector.

Fixes #14935.

crisbeto · 2022-02-21T14:17:02Z

packages/core/src/render3/di.ts

+ * @param notFoundValue The value to return when the injection flags is `InjectFlags.Optional`
+ * @returns the value from the injector, `null` when not found, or `notFoundValue` if provided
+ */
+function lookupTokenUsingNodeInjector<T>(


This function's code is the same as getOrCreateInjectable currently, feel free to ignore when reviewing.

crisbeto · 2022-02-21T14:18:55Z

packages/core/test/render3/perf/embedded_view_injector/index.ts

+
+const noInjectorHarness = setupTestHarness(template, 1, 0, 1, {}, null, [noInjectorApp.ɵcmp]);
+const withInjectorHarness = setupTestHarness(template, 1, 0, 1, {}, null, [withInjectorApp.ɵcmp]);
+


There's something weird going on in this benchmark, because whichever case runs first ends up being quicker, even if it I give it identical components by not passing in an injector above. The difference can be observed by swapping the order of the while loops below.

cc @pkozlowski-opensource

That's super interesting. One hypothesis is that it could be a branch misprediction issue, where we train the branch predictor to expect one case (no embedded injector) and then violate that assumption in the next case (embedded injector present).

It's worth noting that the same happens if I create two identical components that don't hit the new code path.

dylhunn

reviewed-for: fw-core, fw-common, public-api

jessicajaniuk

LGTM 🍪

reviewed-for: public-api, fw-core, fw-common, size-tracking

alxhub

Reviewed-for: public-api

dylhunn

reviewed-for: public-api, fw-core, fw-common, size-tracking

jessicajaniuk · 2022-02-28T19:42:28Z

This PR was merged into the repository by commit 94c949a.

dylhunn · 2022-03-01T00:38:06Z

We are going to roll this back, we think it caused a failure in sync cl/431510120.

…an embedded view (angular#45156)" This reverts commit 94c949a.

…an embedded view (#45156)" (#45220) This reverts commit 94c949a. PR Close #45220

…ded view Adds support for passing in an optional injector when creating an embedded view through `ViewContainerRef.createEmbeddedView` and `TemplateRef.createEmbeddedView`. The injector allows for the DI behavior to be customized within the specific template. This is a second stab at the changes in angular#44666. The difference this time is that the new injector acts as a node injector, rather than a module injector. Fixes angular#14935.

crisbeto · 2022-03-02T08:47:15Z

Passing TGP after a fix yesterday where one of the bitmaps was missing a bit. Unblocking.

jessicajaniuk · 2022-03-02T16:38:02Z

This PR was merged into the repository by commit 69018c9.

angular-automatic-lock-bot · 2022-04-02T16:06:20Z

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.}

…ded view (angular#45156) Adds support for passing in an optional injector when creating an embedded view through `ViewContainerRef.createEmbeddedView` and `TemplateRef.createEmbeddedView`. The injector allows for the DI behavior to be customized within the specific template. This is a second stab at the changes in angular#44666. The difference this time is that the new injector acts as a node injector, rather than a module injector. Fixes angular#14935. PR Close angular#45156

crisbeto commented Feb 21, 2022

View reviewed changes

crisbeto force-pushed the embedded-view-injector-again branch 3 times, most recently from 6c25558 to e000854 Compare February 21, 2022 15:18

crisbeto marked this pull request as ready for review February 21, 2022 15:34

pullapprove bot requested a review from dylhunn February 21, 2022 15:34

crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer area: common Issues related to APIs in the @angular/common package area: core Issues related to the framework runtime target: minor This PR is targeted for the next minor release labels Feb 21, 2022

ngbot bot modified the milestone: Backlog Feb 21, 2022

dylhunn approved these changes Feb 23, 2022 •

edited

Loading

View reviewed changes

pullapprove bot requested review from atscott, dylhunn, jelbourn and jessicajaniuk February 23, 2022 17:30

dylhunn approved these changes Feb 23, 2022

View reviewed changes

pullapprove bot requested a review from alxhub February 23, 2022 17:30

jessicajaniuk approved these changes Feb 24, 2022

View reviewed changes

pullapprove bot requested a review from AndrewKushnir February 24, 2022 18:18

crisbeto force-pushed the embedded-view-injector-again branch 3 times, most recently from b4a4ee1 to f179023 Compare February 28, 2022 10:37

alxhub approved these changes Feb 28, 2022

View reviewed changes

pullapprove bot requested a review from dylhunn February 28, 2022 17:06

dylhunn approved these changes Feb 28, 2022

View reviewed changes

crisbeto force-pushed the embedded-view-injector-again branch from f179023 to 7f83ce0 Compare February 28, 2022 18:16

crisbeto 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 Feb 28, 2022

jessicajaniuk removed request for atscott, jelbourn and AndrewKushnir February 28, 2022 19:05

jessicajaniuk closed this in 94c949a Feb 28, 2022

dylhunn reopened this Mar 1, 2022

dylhunn added a commit to dylhunn/angular that referenced this pull request Mar 1, 2022

Revert "feat(core): allow for injector to be specified when creating …

4995a07

…an embedded view (angular#45156)" This reverts commit 94c949a.

dylhunn mentioned this pull request Mar 1, 2022

Revert "feat(core): allow for injector to be specified when creating … #45220

Closed

dylhunn added a commit to dylhunn/angular that referenced this pull request Mar 1, 2022

Revert "feat(core): allow for injector to be specified when creating …

62fc23d

…an embedded view (angular#45156)" This reverts commit 94c949a.

jessicajaniuk pushed a commit that referenced this pull request Mar 1, 2022

Revert "feat(core): allow for injector to be specified when creating …

4b936a1

…an embedded view (#45156)" (#45220) This reverts commit 94c949a. PR Close #45220

jessicajaniuk added state: blocked action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: merge The PR is ready for merge by the caretaker labels Mar 1, 2022

crisbeto force-pushed the embedded-view-injector-again branch 2 times, most recently from a8cac4d to 29e2c76 Compare March 2, 2022 06:20

crisbeto force-pushed the embedded-view-injector-again branch from 29e2c76 to 2db9321 Compare March 2, 2022 06:45

crisbeto added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews state: blocked labels Mar 2, 2022

jessicajaniuk closed this in 69018c9 Mar 2, 2022

angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 2, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): allow for injector to be specified when creating an embedded view #45156

feat(core): allow for injector to be specified when creating an embedded view #45156

crisbeto commented Feb 21, 2022

crisbeto Feb 21, 2022

crisbeto Feb 21, 2022

alxhub Feb 23, 2022

crisbeto Feb 23, 2022

dylhunn left a comment

jessicajaniuk left a comment •

edited

Loading

alxhub left a comment

dylhunn left a comment

jessicajaniuk commented Feb 28, 2022

dylhunn commented Mar 1, 2022

crisbeto commented Mar 2, 2022

jessicajaniuk commented Mar 2, 2022

angular-automatic-lock-bot bot commented Apr 2, 2022


		const noInjectorHarness = setupTestHarness(template, 1, 0, 1, {}, null, [noInjectorApp.ɵcmp]);
		const withInjectorHarness = setupTestHarness(template, 1, 0, 1, {}, null, [withInjectorApp.ɵcmp]);

feat(core): allow for injector to be specified when creating an embedded view #45156

feat(core): allow for injector to be specified when creating an embedded view #45156

Conversation

crisbeto commented Feb 21, 2022

crisbeto Feb 21, 2022

Choose a reason for hiding this comment

crisbeto Feb 21, 2022

Choose a reason for hiding this comment

alxhub Feb 23, 2022

Choose a reason for hiding this comment

crisbeto Feb 23, 2022

Choose a reason for hiding this comment

dylhunn left a comment

Choose a reason for hiding this comment

jessicajaniuk left a comment • edited Loading

Choose a reason for hiding this comment

alxhub left a comment

Choose a reason for hiding this comment

dylhunn left a comment

Choose a reason for hiding this comment

jessicajaniuk commented Feb 28, 2022

dylhunn commented Mar 1, 2022

crisbeto commented Mar 2, 2022

jessicajaniuk commented Mar 2, 2022

angular-automatic-lock-bot bot commented Apr 2, 2022

jessicajaniuk left a comment •

edited

Loading