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

Issue with assigning lazy loaded standalone components environment injectors #50724

Closed
AleksanderBodurri opened this issue Jun 14, 2023 · 6 comments
Labels
area: router bug core: di cross-cutting: standalone Issues related to the NgModule-less world P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent router: lazy loading state: has PR
Milestone

Comments

@AleksanderBodurri
Copy link
Member

AleksanderBodurri commented Jun 14, 2023

Which @angular/* package(s) are the source of the bug?

core/router

Is this a regression?

No

Description

Noticed some odd DI/Router behaviour,

I have 2 lazily loaded standalone components, each loading from different routes, each importing an NgModule that provides a service.

Source here https://stackblitz.com/edit/angular-n7t8rb, you may have to visit this first to get the links below working.

If I visit https://angular-n7t8rb.stackblitz.io/ and then click the “lazy two” button, I get a DI error.
If i visit https://angular-n7t8rb.stackblitz.io/two it loads successfully. Why would this work when the router-based navigation above did not? When i click the “lazy one” button to load the other lazy component, theres a DI error.

I’ve logged the injectors created by each component as they render, it looks like the first Lazy component that gets loaded is the one that gets a new Environment Injector, the second does not, causing the DI error. We get an R3 injector for App and 1 for either LazyComponent or LazyTwoComponent, based on which loads first. I think this is related to the usage of loadComponent

I made another example with loadChildren for one route and loadComponent for the second, https://stackblitz.com/edit/angular-3ffdoh. In this case we get no errors and an R3 injector for App, LazyComponent and LazyTwoComponent, which seems to me like the expected behavior.

@garrettld
Copy link
Contributor

garrettld commented Jun 15, 2023

I've created a minimal reproduction that doesn't use lazy-loaded components or the router here: https://stackblitz.com/edit/angular-extdt1?file=src%2Fmain.ts

The issue is that your components have the same selector. I'm not sure why this causes this behavior (edit: see my comment below for why), but you can see the NG0912 warning (https://angular.io/errors/NG0912) in the console.

The issue goes away if you fix the duplicate component id issue in any of the ways described by the NG0912 docs.

@garrettld
Copy link
Contributor

garrettld commented Jun 15, 2023

I think the root cause is that standalone component injectors are cached by their component id here:

getOrCreateStandaloneInjector(componentDef: ComponentDef<unknown>): EnvironmentInjector|null {
if (!componentDef.standalone) {
return null;
}
if (!this.cachedInjectors.has(componentDef.id)) {
const providers = internalImportProvidersFrom(false, componentDef.type);
const standaloneInjector = providers.length > 0 ?
createEnvironmentInjector(
[providers], this._injector, `Standalone[${componentDef.type.name}]`) :
null;
this.cachedInjectors.set(componentDef.id, standaloneInjector);
}
return this.cachedInjectors.get(componentDef.id)!;
}

This explains why the order of component activation matters for your reproduction. When the second component is created, it's pulling the injector from the cache instead of creating a new one like you'd expect.

@pkozlowski-opensource
Copy link
Member

Thnx for digging into it @garrettld - this raises an interesting question of using component's id as a key in the cachedInjectors - it seems like it is not reliable enough.

@pkozlowski-opensource
Copy link
Member

pkozlowski-opensource commented Jul 5, 2023

Related: #46093 and #50158

@pkozlowski-opensource pkozlowski-opensource added bug P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent labels Jul 5, 2023
@ngbot ngbot bot modified the milestones: needsTriage, Backlog Jul 5, 2023
@AndrewKushnir
Copy link
Contributor

AndrewKushnir commented Jul 5, 2023

this raises an interesting question of using component's id as a key in the cachedInjectors - it seems like it is not reliable enough.

@pkozlowski-opensource agreed, the component ids that we generate based on the metadata may have collisions. We plan to improve their generation further (by using more fields, including styles), but we should transition to something else (probably a WeakMap) in the DI code to always have proper component->injector connection. // cc @alan-agius4

AndrewKushnir added a commit to AndrewKushnir/angular that referenced this issue Jul 6, 2023
…tances

Prior to this change, we've used `componentDef.id` as a key in a Map that acts as a cache to avoid re-creating injector instances for standalone components. In v16, the logic that generates the id has changed from an auto-incremental to a generation based on metadata. If multiple components have similar metadata, their ids might overlap.

This commit updates the logic to stop using `componentDef.id` as a key and instead, use the `componentDef` itself. This would ensure that we always have a correct instance of an injector associated with a standalone component instance.

Resolves angular#50724.
dylhunn pushed a commit that referenced this issue Jul 10, 2023
…tances (#50954)

Prior to this change, we've used `componentDef.id` as a key in a Map that acts as a cache to avoid re-creating injector instances for standalone components. In v16, the logic that generates the id has changed from an auto-incremental to a generation based on metadata. If multiple components have similar metadata, their ids might overlap.

This commit updates the logic to stop using `componentDef.id` as a key and instead, use the `componentDef` itself. This would ensure that we always have a correct instance of an injector associated with a standalone component instance.

Resolves #50724.

PR Close #50954
sunilbaba pushed a commit to sunilbaba/angular that referenced this issue Jul 26, 2023
…tances (angular#50954)

Prior to this change, we've used `componentDef.id` as a key in a Map that acts as a cache to avoid re-creating injector instances for standalone components. In v16, the logic that generates the id has changed from an auto-incremental to a generation based on metadata. If multiple components have similar metadata, their ids might overlap.

This commit updates the logic to stop using `componentDef.id` as a key and instead, use the `componentDef` itself. This would ensure that we always have a correct instance of an injector associated with a standalone component instance.

Resolves angular#50724.

PR Close angular#50954
@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 Aug 10, 2023
thomasturrell pushed a commit to thomasturrell/angular that referenced this issue Aug 29, 2023
…tances (angular#50954)

Prior to this change, we've used `componentDef.id` as a key in a Map that acts as a cache to avoid re-creating injector instances for standalone components. In v16, the logic that generates the id has changed from an auto-incremental to a generation based on metadata. If multiple components have similar metadata, their ids might overlap.

This commit updates the logic to stop using `componentDef.id` as a key and instead, use the `componentDef` itself. This would ensure that we always have a correct instance of an injector associated with a standalone component instance.

Resolves angular#50724.

PR Close angular#50954
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this issue Jan 23, 2024
…tances (angular#50954)

Prior to this change, we've used `componentDef.id` as a key in a Map that acts as a cache to avoid re-creating injector instances for standalone components. In v16, the logic that generates the id has changed from an auto-incremental to a generation based on metadata. If multiple components have similar metadata, their ids might overlap.

This commit updates the logic to stop using `componentDef.id` as a key and instead, use the `componentDef` itself. This would ensure that we always have a correct instance of an injector associated with a standalone component instance.

Resolves angular#50724.

PR Close angular#50954
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: router bug core: di cross-cutting: standalone Issues related to the NgModule-less world P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent router: lazy loading state: has PR
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants