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(core): ViewContainerRef.createComponent should consult module injector when custom one is provided #44966

Conversation

AndrewKushnir
Copy link
Contributor

Before Ivy, it was only possible to call the ViewContainerRef.createComponent function with the ComponentFactory as the first argument. An instance of a ComponentFactory resolved via ComponentFactoryResolver contained a reference to an NgModule where the component is declared. As a result, the component maintained a DI connection with the module injector tree (by retrieving an instance of NgModuleRef internally), even when the custom injector was provided (we try to find a token in a custom injector first and consult module injector after that).

With Ivy, we expanded the ViewContainerRef.createComponent function API to support direct references to the Component classes without going through the factory resolution step. As a result, there was no connection to the NgModule that declares the component. Thus, if you provide a custom injector, this is the only injector that is taken into account.

This commit updates the logic for the factory-less case to try retrieving an instance of an NgModuleRef using the DI tree which ViewContainerRef belongs to. The NgModuleRef instance is then used to get a hold of a module injector tree. This brings the factory-less and factory-based logic to a consistent state.

Closes #44897.

PR Type

What kind of change does this PR introduce?

  • Bugfix

Does this PR introduce a breaking change?

  • Yes
  • No

@AndrewKushnir AndrewKushnir added type: bug/fix state: WIP area: core Issues related to the framework runtime target: patch This PR is targeted for the next patch release labels Feb 3, 2022
@ngbot ngbot bot added this to the Backlog milestone Feb 3, 2022
@pkozlowski-opensource
Copy link
Member

Drive-by-comment (sorry, just trying to understand things better).

This commit updates the logic for the factory-less case to try retrieving an instance of an NgModuleRef using the DI tree which ViewContainerRef belongs to. The NgModuleRef instance is then used to get a hold of a module injector tree. This brings the factory-less and factory-based logic to a consistent state.

My understanding is that previously a component type would maintain (through a component factory resolver) a connection to a NgModule where the component being instantiated was declared. It mens that it could "see" providers from the declaring module. In other words - a component could see providers from the declaration place.

If I understand correctly, based on the comment, we are now trying to figure out NgModuleRef based on the insertion point (this is how I'm interpreting "using the DI tree which ViewContainerRef belongs to").

In many cases the declaration and insertion injector hierarchies will "see" the same providers but it is not always the case, ex.: for lazy-loaded modules. If my reasoning it correct a dynamically instantiated component still wouldn't "see" providers from the lazy module, right? (unless a lazy-loaded NgModuleRef is passed-in explicitly). It is reasonable, I'm just trying to understand if this is the same behaviour as with ComponentFactoryResolver version of things?

@alxhub
Copy link
Member

alxhub commented Feb 4, 2022

@pkozlowski-opensource absolutely accurate summary!

My understanding is that previously a component type would maintain (through a component factory resolver) a connection to a NgModule where the component being instantiated was declared. It mens that it could "see" providers from the declaring module. In other words - a component could see providers from the declaration place.

This (as you probably know) was absolutely true in View Engine, but only conventionally true in Ivy.

In View Engine, you had to use a CFR from the NgModuleRef (injector) from an NgModule which declared/imported the component you were trying to resolve. Doing otherwise would result in the CFR not being able to resolve the component to a factory.

In Ivy, any CFR can give you a factory for any component - with an NgModuleRef tied to the CFR's provenance that may or may not be the "right" one for the component in question. Thus, to use the API correctly, users have to know to get a particular CFR from the right injector (and nowhere in our documentation is this explained).

I think this ship has sailed, to be honest - the framework no longer enforces the component-NgModule relationship, and we're moving towards a world without ComponentFactory at all. Instead, we should make sure that we document the right conventions around components and NgModules.

If I understand correctly, based on the comment, we are now trying to figure out NgModuleRef based on the insertion point (this is how I'm interpreting "using the DI tree which ViewContainerRef belongs to").

I view this approach as using the DI tree which you would've gotten if you naively injected ComponentFactoryResolver and resolved the component to a factory on demand. This does the "right thing" in the standard application case and in lazy-loaded routes. It's insufficient in cases where you loaded the component reference from some "out-of-tree" location - but in that case, Ivy's implementation of CFR wouldn't help either - it's on the user to make sure that they pass in the right NgModuleRef in that case.

@pkozlowski-opensource
Copy link
Member

Thnx @alxhub for the comment, it completely matches my understanding / mental model. Key highlights for me:

Thus, to use the API correctly, users have to know to get a particular CFR from the right injector (and nowhere in our documentation is this explained).

Yep, probably not many people are aware of it. In my head it translates to: it is on users to maintain "component / provider" association "declared" in NgModule. With ivy this association is gone and needs to be re-created "manually".

I think this ship has sailed, to be honest - the framework no longer enforces the component-NgModule relationship, and we're moving towards a world without ComponentFactory at all. Instead, we should make sure that we document the right conventions around components and NgModules.

Agree. And the only long-term strategy, IMO, is to work towards the World without providers defined on a NgModule level....

It's insufficient in cases where you loaded the component reference from some "out-of-tree" location - but in that case, Ivy's implementation of CFR wouldn't help either - it's on the user to make sure that they pass in the right NgModuleRef in that case.

Agreed.

@AndrewKushnir AndrewKushnir force-pushed the vcr-create-component-injector branch 2 times, most recently from 2870489 to 1a78f04 Compare February 5, 2022 01:36
@AndrewKushnir AndrewKushnir added action: review The PR is still awaiting reviews from at least one requested reviewer action: presubmit The PR is in need of a google3 presubmit and removed state: WIP labels Feb 7, 2022
@AndrewKushnir AndrewKushnir marked this pull request as ready for review February 7, 2022 00:36
@AndrewKushnir
Copy link
Contributor Author

Global Presubmit.

…njector when custom one is provided

Before Ivy, it was only possible to call the `ViewContainerRef.createComponent` function with the ComponentFactory as the first argument. An instance of a `ComponentFactory` resolved via `ComponentFactoryResolver` contained a reference to an `NgModule` where the component is declared. As a result, the component maintained a DI connection with the module injector tree (by retrieving an instance of `NgModuleRef` internally), even when the custom injector was provided (we try to find a token in a custom injector first and consult module injector after that).

With Ivy, we expanded the `ViewContainerRef.createComponent` function API to support direct references to the Component classes without going through the factory resolution step. As a result, there was no connection to the NgModule that declares the component. Thus, if you provide a custom injector, this is the only injector that is taken into account.

This commit updates the logic for the factory-less case to try retrieving an instance of an `NgModuleRef` using the DI tree which `ViewContainerRef` belongs to. The `NgModuleRef` instance is then used to get a hold of a module injector tree. This brings the factory-less and factory-based logic to more consistent state.

Closes angular#44897.
@AndrewKushnir AndrewKushnir 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 action: presubmit The PR is in need of a google3 presubmit labels Feb 8, 2022
@dylhunn
Copy link
Contributor

dylhunn commented Feb 8, 2022

This PR was merged into the repository by commit 822439f.

dylhunn pushed a commit that referenced this pull request Feb 8, 2022
…njector when custom one is provided (#44966)

Before Ivy, it was only possible to call the `ViewContainerRef.createComponent` function with the ComponentFactory as the first argument. An instance of a `ComponentFactory` resolved via `ComponentFactoryResolver` contained a reference to an `NgModule` where the component is declared. As a result, the component maintained a DI connection with the module injector tree (by retrieving an instance of `NgModuleRef` internally), even when the custom injector was provided (we try to find a token in a custom injector first and consult module injector after that).

With Ivy, we expanded the `ViewContainerRef.createComponent` function API to support direct references to the Component classes without going through the factory resolution step. As a result, there was no connection to the NgModule that declares the component. Thus, if you provide a custom injector, this is the only injector that is taken into account.

This commit updates the logic for the factory-less case to try retrieving an instance of an `NgModuleRef` using the DI tree which `ViewContainerRef` belongs to. The `NgModuleRef` instance is then used to get a hold of a module injector tree. This brings the factory-less and factory-based logic to more consistent state.

Closes #44897.

PR Close #44966
@dylhunn dylhunn closed this in 822439f Feb 8, 2022
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Feb 19, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [@angular/animations](https://github.com/angular/angular) | dependencies | patch | [`13.2.2` -> `13.2.3`](https://renovatebot.com/diffs/npm/@angular%2fanimations/13.2.2/13.2.3) |
| [@angular/common](https://github.com/angular/angular) | dependencies | patch | [`13.2.2` -> `13.2.3`](https://renovatebot.com/diffs/npm/@angular%2fcommon/13.2.2/13.2.3) |
| [@angular/compiler](https://github.com/angular/angular) | dependencies | patch | [`13.2.2` -> `13.2.3`](https://renovatebot.com/diffs/npm/@angular%2fcompiler/13.2.2/13.2.3) |
| [@angular/compiler-cli](https://github.com/angular/angular) | devDependencies | patch | [`13.2.2` -> `13.2.3`](https://renovatebot.com/diffs/npm/@angular%2fcompiler-cli/13.2.2/13.2.3) |
| [@angular/core](https://github.com/angular/angular) | dependencies | patch | [`13.2.2` -> `13.2.3`](https://renovatebot.com/diffs/npm/@angular%2fcore/13.2.2/13.2.3) |
| [@angular/forms](https://github.com/angular/angular) | dependencies | patch | [`13.2.2` -> `13.2.3`](https://renovatebot.com/diffs/npm/@angular%2fforms/13.2.2/13.2.3) |
| [@angular/platform-browser](https://github.com/angular/angular) | dependencies | patch | [`13.2.2` -> `13.2.3`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser/13.2.2/13.2.3) |
| [@angular/platform-browser-dynamic](https://github.com/angular/angular) | dependencies | patch | [`13.2.2` -> `13.2.3`](https://renovatebot.com/diffs/npm/@angular%2fplatform-browser-dynamic/13.2.2/13.2.3) |
| [@angular/router](https://github.com/angular/angular) | dependencies | patch | [`13.2.2` -> `13.2.3`](https://renovatebot.com/diffs/npm/@angular%2frouter/13.2.2/13.2.3) |

---

### Release Notes

<details>
<summary>angular/angular</summary>

### [`v13.2.3`](https://github.com/angular/angular/blob/HEAD/CHANGELOG.md#&#8203;1323-2022-02-16)

[Compare Source](angular/angular@13.2.2...13.2.3)

##### animations

| Commit | Type | Description |
| -- | -- | -- |
| [0050b01b62](angular/angular@0050b01) | perf | made errors in the animations package tree shakeable ([#&#8203;45079](angular/angular#45079)) |

##### compiler

| Commit | Type | Description |
| -- | -- | -- |
| [09f0254bdd](angular/angular@09f0254) | perf | chain element start/end instructions ([#&#8203;44994](angular/angular#44994)) |

##### core

| Commit | Type | Description |
| -- | -- | -- |
| [92cf9fbebe](angular/angular@92cf9fb) | fix | `ViewContainerRef.createComponent` should consult module injector when custom one is provided ([#&#8203;44966](angular/angular#44966)) |

#### Special Thanks

AlirezaEbrahimkhani, Amer Yousuf, Andrew Kushnir, Aristeidis Bampakos, Dario Piotrowicz, Esteban Gehring, Jessica Janiuk, JiaLiPassion, Kristiyan Kostadinov, Mina Hosseini Moghadam, Patrick Cameron, Srdjan Milic, Yousaf Nawaz, dario-piotrowicz, markostanimirovic, mgechev and zuckjet

<!-- CHANGELOG SPLIT MARKER -->

</details>

---

### Configuration

📅 **Schedule**: 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.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

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/1178
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
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 11, 2022
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 target: patch This PR is targeted for the next patch release type: bug/fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot inject Renderer2 when the application uses Renderer3!
4 participants