Skip to content

Commit

Permalink
fix(router): Routed components never inherit RouterOutlet `Environm…
Browse files Browse the repository at this point in the history
…entInjector` (#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
  • Loading branch information
atscott committed Apr 1, 2024
1 parent 87f3f27 commit 3839cfb
Show file tree
Hide file tree
Showing 7 changed files with 107 additions and 51 deletions.
8 changes: 5 additions & 3 deletions goldens/public-api/router/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -183,6 +183,7 @@ export class ChildActivationStart {

// @public
export class ChildrenOutletContexts {
constructor(parentInjector: EnvironmentInjector);
// (undocumented)
getContext(childName: string): OutletContext | null;
// (undocumented)
Expand Down Expand Up @@ -539,12 +540,13 @@ export type OnSameUrlNavigation = 'reload' | 'ignore';

// @public
export class OutletContext {
constructor(injector: EnvironmentInjector);
// (undocumented)
attachRef: ComponentRef<any> | null;
// (undocumented)
children: ChildrenOutletContexts;
// (undocumented)
injector: EnvironmentInjector | null;
injector: EnvironmentInjector;
// (undocumented)
outlet: RouterOutletContract | null;
// (undocumented)
Expand Down Expand Up @@ -882,7 +884,7 @@ export class RouterOutlet implements OnDestroy, OnInit, RouterOutletContract {
// (undocumented)
activateEvents: EventEmitter<any>;
// (undocumented)
activateWith(activatedRoute: ActivatedRoute, environmentInjector?: EnvironmentInjector | null): void;
activateWith(activatedRoute: ActivatedRoute, environmentInjector: EnvironmentInjector): void;
attach(ref: ComponentRef<any>, activatedRoute: ActivatedRoute): void;
attachEvents: EventEmitter<unknown>;
// (undocumented)
Expand Down Expand Up @@ -915,7 +917,7 @@ export interface RouterOutletContract {
activatedRoute: ActivatedRoute | null;
activatedRouteData: Data;
activateEvents?: EventEmitter<unknown>;
activateWith(activatedRoute: ActivatedRoute, environmentInjector: EnvironmentInjector | null): void;
activateWith(activatedRoute: ActivatedRoute, environmentInjector: EnvironmentInjector): void;
attach(ref: ComponentRef<unknown>, activatedRoute: ActivatedRoute): void;
attachEvents?: EventEmitter<unknown>;
component: Object | null;
Expand Down
49 changes: 20 additions & 29 deletions packages/core/test/acceptance/injector_profiler_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -354,26 +354,21 @@ 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');
expect(injectorMetadata[2]!.type).toBe('element');
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');
}
}));

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);

Expand All @@ -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[6]).toBeInstanceOf(R3Injector);
expect((path[6] as R3Injector).scopes.has('platform')).toBeTrue();
expect((path[5] as R3Injector).scopes.has('platform')).toBeTrue();

expect(path[7]).toBeInstanceOf(NullInjector);
expect(path[6]).toBeInstanceOf(NullInjector);
}
}));
});
10 changes: 3 additions & 7 deletions packages/router/src/directives/router_outlet.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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,
Expand All @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion packages/router/src/operators/activate_routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
9 changes: 6 additions & 3 deletions packages/router/src/router_outlet_context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<any> | null = null;
constructor(public injector: EnvironmentInjector) {}
}

/**
Expand All @@ -34,6 +34,9 @@ export class ChildrenOutletContexts {
// contexts for child outlets, by name.
private contexts = new Map<string, OutletContext>();

/** @nodoc */
constructor(private parentInjector: EnvironmentInjector) {}

/** Called when a `RouterOutlet` directive is instantiated */
onChildOutletCreated(childName: string, outlet: RouterOutletContract): void {
const context = this.getOrCreateContext(childName);
Expand Down Expand Up @@ -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);
}

Expand Down
52 changes: 50 additions & 2 deletions packages/router/test/directives/router_outlet.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,26 @@
*/

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,
Router,
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(() => {
Expand Down Expand Up @@ -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<any>('');

@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: '<router-outlet/>',
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<unknown>, millis?: number): void {
tick(millis);
fixture.detectChanges();
Expand Down
28 changes: 22 additions & 6 deletions packages/router/test/router.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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) => {
Expand All @@ -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({
Expand Down

0 comments on commit 3839cfb

Please sign in to comment.