From 3839cfbb18fcc70cae5a6ba4ba7676b1c4acf7a0 Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Mon, 5 Feb 2024 10:30:55 -0800 Subject: [PATCH] fix(router): Routed components never inherit `RouterOutlet` `EnvironmentInjector` (#54265) This commit ensures components in the route config predictably always get their providers from the hierarchy available to routes rather than sometimes being dependent on where they are inserted. fixes #53369 BREAKING CHANGE: Providers available to the routed components always come from the injector heirarchy of the routes and never inherit from the `RouterOutlet`. This means that providers available only to the component that defines the `RouterOutlet` will no longer be available to route components in any circumstances. This was already the case whenever routes defined providers, either through lazy loading an `NgModule` or through explicit `providers` on the route config. PR Close #54265 --- goldens/public-api/router/index.md | 8 +-- .../test/acceptance/injector_profiler_spec.ts | 49 +++++++---------- .../router/src/directives/router_outlet.ts | 10 ++-- .../router/src/operators/activate_routes.ts | 2 +- packages/router/src/router_outlet_context.ts | 9 ++-- .../test/directives/router_outlet.spec.ts | 52 ++++++++++++++++++- packages/router/test/router.spec.ts | 28 +++++++--- 7 files changed, 107 insertions(+), 51 deletions(-) diff --git a/goldens/public-api/router/index.md b/goldens/public-api/router/index.md index 8905f7d4b70..adfeca31fab 100644 --- a/goldens/public-api/router/index.md +++ b/goldens/public-api/router/index.md @@ -183,6 +183,7 @@ export class ChildActivationStart { // @public export class ChildrenOutletContexts { + constructor(parentInjector: EnvironmentInjector); // (undocumented) getContext(childName: string): OutletContext | null; // (undocumented) @@ -539,12 +540,13 @@ export type OnSameUrlNavigation = 'reload' | 'ignore'; // @public export class OutletContext { + constructor(injector: EnvironmentInjector); // (undocumented) attachRef: ComponentRef | null; // (undocumented) children: ChildrenOutletContexts; // (undocumented) - injector: EnvironmentInjector | null; + injector: EnvironmentInjector; // (undocumented) outlet: RouterOutletContract | null; // (undocumented) @@ -882,7 +884,7 @@ export class RouterOutlet implements OnDestroy, OnInit, RouterOutletContract { // (undocumented) activateEvents: EventEmitter; // (undocumented) - activateWith(activatedRoute: ActivatedRoute, environmentInjector?: EnvironmentInjector | null): void; + activateWith(activatedRoute: ActivatedRoute, environmentInjector: EnvironmentInjector): void; attach(ref: ComponentRef, activatedRoute: ActivatedRoute): void; attachEvents: EventEmitter; // (undocumented) @@ -915,7 +917,7 @@ export interface RouterOutletContract { activatedRoute: ActivatedRoute | null; activatedRouteData: Data; activateEvents?: EventEmitter; - activateWith(activatedRoute: ActivatedRoute, environmentInjector: EnvironmentInjector | null): void; + activateWith(activatedRoute: ActivatedRoute, environmentInjector: EnvironmentInjector): void; attach(ref: ComponentRef, activatedRoute: ActivatedRoute): void; attachEvents?: EventEmitter; component: Object | null; diff --git a/packages/core/test/acceptance/injector_profiler_spec.ts b/packages/core/test/acceptance/injector_profiler_spec.ts index 45abc965f74..09395686680 100644 --- a/packages/core/test/acceptance/injector_profiler_spec.ts +++ b/packages/core/test/acceptance/injector_profiler_spec.ts @@ -354,17 +354,14 @@ describe('getInjectorMetadata', () => { expect(injectorMetadata[4]).toBeDefined(); expect(injectorMetadata[5]).toBeDefined(); expect(injectorMetadata[6]).toBeDefined(); - expect(injectorMetadata[7]).toBeDefined(); expect(injectorMetadata[0]!.source).toBe(lazyComponent.elementRef.nativeElement); expect(injectorMetadata[1]!.source) .toBe(myStandaloneComponent.routerOutlet!.nativeElement); expect(injectorMetadata[2]!.source).toBe(myStandaloneComponent.elementRef.nativeElement); expect(injectorMetadata[3]!.source).toBe('Standalone[LazyComponent]'); - expect(injectorMetadata[4]!.source).toBe('Standalone[MyStandaloneComponent]'); - expect(injectorMetadata[5]!.source).toBe('DynamicTestModule'); - expect(injectorMetadata[6]!.source).toBe('Platform: core'); - expect(injectorMetadata[7]!.source).toBeNull(); + expect(injectorMetadata[4]!.source).toBe('DynamicTestModule'); + expect(injectorMetadata[5]!.source).toBe('Platform: core'); expect(injectorMetadata[0]!.type).toBe('element'); expect(injectorMetadata[1]!.type).toBe('element'); @@ -372,8 +369,6 @@ describe('getInjectorMetadata', () => { expect(injectorMetadata[3]!.type).toBe('environment'); expect(injectorMetadata[4]!.type).toBe('environment'); expect(injectorMetadata[5]!.type).toBe('environment'); - expect(injectorMetadata[6]!.type).toBe('environment'); - expect(injectorMetadata[7]!.type).toBe('null'); } })); @@ -932,10 +927,10 @@ describe('getDependenciesFromInjectable', () => { standalone: true }) class MyStandaloneComponentB { - myService = inject(MyService); + myService = inject(MyService, {optional: true}); myServiceB = inject(MyServiceB, {optional: true}); - myServiceC = inject(MyServiceC, {skipSelf: true}); - myInjectionTokenValue = inject(myInjectionToken); + myServiceC = inject(MyServiceC, {skipSelf: true, optional: true}); + myInjectionTokenValue = inject(myInjectionToken, {optional: true}); injector = inject(Injector, {self: true, host: true}); myServiceD = inject(MyServiceD); myServiceG = inject(MyServiceG); @@ -995,7 +990,7 @@ describe('getDependenciesFromInjectable', () => { expect(parentComponentDep.token).toBe(MyStandaloneComponent); expect(dependenciesOfMyStandaloneComponentB[0].flags).toEqual({ - optional: false, + optional: true, skipSelf: false, self: false, host: false, @@ -1007,13 +1002,13 @@ describe('getDependenciesFromInjectable', () => { host: false, }); expect(myServiceCDep.flags).toEqual({ - optional: false, + optional: true, skipSelf: true, self: false, host: false, }); expect(myInjectionTokenValueDep.flags).toEqual({ - optional: false, + optional: true, skipSelf: false, self: false, host: false, @@ -1045,18 +1040,18 @@ describe('getDependenciesFromInjectable', () => { expect(dependenciesOfMyStandaloneComponentB[0].value).toBe(myStandalonecomponentB.myService); - expect(myServiceBDep.value).toBe('hello world'); - expect(myServiceCDep.value).toBe(123); - expect(myInjectionTokenValueDep.value).toBe(myServiceCInstance); + expect(myServiceBDep.value).toBe(null); + expect(myServiceCDep.value).toBe(null); + expect(myInjectionTokenValueDep.value).toBe(null); expect(injectorDep.value).toBe(myStandalonecomponentB.injector); expect(myServiceDDep.value).toBe('123'); expect(myServiceGDep.value).toBe(myStandalonecomponentB.myServiceG); expect(parentComponentDep.value).toBe(myStandalonecomponentB.parentComponent); - expect(dependenciesOfMyStandaloneComponentB[0].providedIn).toBe(standaloneInjector); - expect(myServiceBDep.providedIn).toBe(standaloneInjector); - expect(myServiceCDep.providedIn).toBe(standaloneInjector); - expect(myInjectionTokenValueDep.providedIn).toBe(standaloneInjector); + expect(dependenciesOfMyStandaloneComponentB[0].providedIn).toBe(undefined); + expect(myServiceBDep.providedIn).toBe(undefined); + expect(myServiceCDep.providedIn).toBe(undefined); + expect(myInjectionTokenValueDep.providedIn).toBe(undefined); expect(injectorDep.providedIn).toBe(myStandalonecomponentB.injector); expect(myServiceDDep.providedIn).toBe(standaloneInjector.get(Injector, null, { skipSelf: true @@ -1266,13 +1261,12 @@ describe('getInjectorResolutionPath', () => { * NodeInjector[RouterOutlet], * NodeInjector[MyStandaloneComponent], * R3Injector[LazyComponent], - * R3Injector[MyStandaloneComponent], * R3Injector[Root], * R3Injector[Platform], * NullInjector * ] */ - expect(path.length).toBe(8); + expect(path.length).toBe(7); expect(path[0]).toBe(lazyComponentNodeInjector); @@ -1291,16 +1285,13 @@ describe('getInjectorResolutionPath', () => { expect(path[4]).toBeInstanceOf(R3Injector); expect((path[4] as R3Injector).scopes.has('environment')).toBeTrue(); - expect((path[4] as R3Injector).source).toBe('Standalone[MyStandaloneComponent]'); + expect((path[4] as R3Injector).source).toBe('DynamicTestModule'); + expect((path[4] as R3Injector).scopes.has('root')).toBeTrue(); expect(path[5]).toBeInstanceOf(R3Injector); - expect((path[5] as R3Injector).scopes.has('environment')).toBeTrue(); - expect((path[5] as R3Injector).scopes.has('root')).toBeTrue(); + expect((path[5] as R3Injector).scopes.has('platform')).toBeTrue(); - expect(path[6]).toBeInstanceOf(R3Injector); - expect((path[6] as R3Injector).scopes.has('platform')).toBeTrue(); - - expect(path[7]).toBeInstanceOf(NullInjector); + expect(path[6]).toBeInstanceOf(NullInjector); } })); }); diff --git a/packages/router/src/directives/router_outlet.ts b/packages/router/src/directives/router_outlet.ts index 9e914cca33e..773ddaf7fb9 100644 --- a/packages/router/src/directives/router_outlet.ts +++ b/packages/router/src/directives/router_outlet.ts @@ -71,10 +71,7 @@ export interface RouterOutletContract { /** * Called by the `Router` when the outlet should activate (create a component). */ - activateWith( - activatedRoute: ActivatedRoute, - environmentInjector: EnvironmentInjector | null, - ): void; + activateWith(activatedRoute: ActivatedRoute, environmentInjector: EnvironmentInjector): void; /** * A request to destroy the currently activated component. @@ -216,7 +213,6 @@ export class RouterOutlet implements OnDestroy, OnInit, RouterOutletContract { private parentContexts = inject(ChildrenOutletContexts); private location = inject(ViewContainerRef); private changeDetector = inject(ChangeDetectorRef); - private environmentInjector = inject(EnvironmentInjector); private inputBinder = inject(INPUT_BINDER, {optional: true}); /** @nodoc */ readonly supportsBindingToComponentInputs = true; @@ -350,7 +346,7 @@ export class RouterOutlet implements OnDestroy, OnInit, RouterOutletContract { } } - activateWith(activatedRoute: ActivatedRoute, environmentInjector?: EnvironmentInjector | null) { + activateWith(activatedRoute: ActivatedRoute, environmentInjector: EnvironmentInjector) { if (this.isActivated) { throw new RuntimeError( RuntimeErrorCode.OUTLET_ALREADY_ACTIVATED, @@ -368,7 +364,7 @@ export class RouterOutlet implements OnDestroy, OnInit, RouterOutletContract { this.activated = location.createComponent(component, { index: location.length, injector, - environmentInjector: environmentInjector ?? this.environmentInjector, + environmentInjector: environmentInjector, }); // Calling `markForCheck` to make sure we will run the change detection when the // `RouterOutlet` is inside a `ChangeDetectionStrategy.OnPush` component. diff --git a/packages/router/src/operators/activate_routes.ts b/packages/router/src/operators/activate_routes.ts index e2f872b2e74..f51b86df90c 100644 --- a/packages/router/src/operators/activate_routes.ts +++ b/packages/router/src/operators/activate_routes.ts @@ -224,7 +224,7 @@ export class ActivateRoutes { const injector = getClosestRouteInjector(future.snapshot); context.attachRef = null; context.route = future; - context.injector = injector; + context.injector = injector ?? context.injector; if (context.outlet) { // Activate the outlet when it has already been instantiated // Otherwise it will get activated from its `ngOnInit` when instantiated diff --git a/packages/router/src/router_outlet_context.ts b/packages/router/src/router_outlet_context.ts index 02d050bcdb6..e59c4d82f77 100644 --- a/packages/router/src/router_outlet_context.ts +++ b/packages/router/src/router_outlet_context.ts @@ -19,9 +19,9 @@ import {ActivatedRoute} from './router_state'; export class OutletContext { outlet: RouterOutletContract | null = null; route: ActivatedRoute | null = null; - injector: EnvironmentInjector | null = null; - children = new ChildrenOutletContexts(); + children = new ChildrenOutletContexts(this.injector); attachRef: ComponentRef | null = null; + constructor(public injector: EnvironmentInjector) {} } /** @@ -34,6 +34,9 @@ export class ChildrenOutletContexts { // contexts for child outlets, by name. private contexts = new Map(); + /** @nodoc */ + constructor(private parentInjector: EnvironmentInjector) {} + /** Called when a `RouterOutlet` directive is instantiated */ onChildOutletCreated(childName: string, outlet: RouterOutletContract): void { const context = this.getOrCreateContext(childName); @@ -72,7 +75,7 @@ export class ChildrenOutletContexts { let context = this.getContext(childName); if (!context) { - context = new OutletContext(); + context = new OutletContext(this.parentInjector); this.contexts.set(childName, context); } diff --git a/packages/router/test/directives/router_outlet.spec.ts b/packages/router/test/directives/router_outlet.spec.ts index 4a9c7de8a35..8de1cd15825 100644 --- a/packages/router/test/directives/router_outlet.spec.ts +++ b/packages/router/test/directives/router_outlet.spec.ts @@ -7,7 +7,16 @@ */ import {CommonModule, NgForOf} from '@angular/common'; -import {Component, Input, Type} from '@angular/core'; +import { + Component, + EnvironmentInjector, + Input, + NgModule, + Type, + createEnvironmentInjector, + importProvidersFrom, + inject, +} from '@angular/core'; import {ComponentFixture, fakeAsync, TestBed, tick} from '@angular/core/testing'; import { provideRouter, @@ -15,8 +24,9 @@ import { RouterModule, RouterOutlet, withComponentInputBinding, -} from '@angular/router/src'; +} from '@angular/router'; import {RouterTestingHarness} from '@angular/router/testing'; +import {InjectionToken} from '../../../core/src/di'; describe('router outlet name', () => { it('should support name binding', fakeAsync(() => { @@ -381,6 +391,44 @@ describe('component input binding', () => { }); }); +describe('injectors', () => { + it('should always use environment injector from route hierarchy and not inherit from outlet', async () => { + let childTokenValue: any = null; + const TOKEN = new InjectionToken(''); + + @Component({ + template: '', + standalone: true, + }) + class Child { + constructor() { + childTokenValue = inject(TOKEN as any, {optional: true}); + } + } + + @NgModule({ + providers: [{provide: TOKEN, useValue: 'some value'}], + }) + class ModWithProviders {} + + @Component({ + template: '', + imports: [RouterOutlet, ModWithProviders], + standalone: true, + }) + class App {} + + TestBed.configureTestingModule({ + providers: [provideRouter([{path: 'a', component: Child}])], + }); + const fixture = TestBed.createComponent(App); + fixture.detectChanges(); + await TestBed.inject(Router).navigateByUrl('/a'); + fixture.detectChanges(); + expect(childTokenValue).toEqual(null); + }); +}); + function advance(fixture: ComponentFixture, millis?: number): void { tick(millis); fixture.detectChanges(); diff --git a/packages/router/test/router.spec.ts b/packages/router/test/router.spec.ts index 6de3f219e4c..b177c3f4b09 100644 --- a/packages/router/test/router.spec.ts +++ b/packages/router/test/router.spec.ts @@ -186,7 +186,11 @@ describe('Router', () => { // Since we only test the guards, we don't need to provide a full navigation // transition object with all properties set. const testTransition = { - guards: getAllRouteGuards(futureState, empty, new ChildrenOutletContexts()), + guards: getAllRouteGuards( + futureState, + empty, + new ChildrenOutletContexts(TestBed.inject(EnvironmentInjector)), + ), } as NavigationTransition; of(testTransition) @@ -242,7 +246,11 @@ describe('Router', () => { // Since we only test the guards, we don't need to provide a full navigation // transition object with all properties set. const testTransition = { - guards: getAllRouteGuards(futureState, empty, new ChildrenOutletContexts()), + guards: getAllRouteGuards( + futureState, + empty, + new ChildrenOutletContexts(TestBed.inject(EnvironmentInjector)), + ), } as NavigationTransition; of(testTransition) @@ -296,7 +304,11 @@ describe('Router', () => { // Since we only test the guards, we don't need to provide a full navigation // transition object with all properties set. const testTransition = { - guards: getAllRouteGuards(futureState, currentState, new ChildrenOutletContexts()), + guards: getAllRouteGuards( + futureState, + currentState, + new ChildrenOutletContexts(TestBed.inject(EnvironmentInjector)), + ), } as NavigationTransition; of(testTransition) @@ -368,7 +380,11 @@ describe('Router', () => { // Since we only test the guards, we don't need to provide a full navigation // transition object with all properties set. const testTransition = { - guards: getAllRouteGuards(futureState, currentState, new ChildrenOutletContexts()), + guards: getAllRouteGuards( + futureState, + currentState, + new ChildrenOutletContexts(TestBed.inject(EnvironmentInjector)), + ), } as NavigationTransition; of(testTransition) @@ -841,7 +857,7 @@ function checkResolveData( // Since we only test the guards and their resolve data function, we don't need to provide // a full navigation transition object with all properties set. of({ - guards: getAllRouteGuards(future, curr, new ChildrenOutletContexts()), + guards: getAllRouteGuards(future, curr, new ChildrenOutletContexts(injector)), } as NavigationTransition) .pipe(resolveDataOperator('emptyOnly', injector)) .subscribe(check, (e) => { @@ -858,7 +874,7 @@ function checkGuards( // Since we only test the guards, we don't need to provide a full navigation // transition object with all properties set. of({ - guards: getAllRouteGuards(future, curr, new ChildrenOutletContexts()), + guards: getAllRouteGuards(future, curr, new ChildrenOutletContexts(injector)), } as NavigationTransition) .pipe(checkGuardsOperator(injector)) .subscribe({