From 4d2e0063728286866e83cf9cea1510c2a319d005 Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Fri, 13 May 2022 09:53:55 -0700 Subject: [PATCH] refactor(router): Only create the `Route` injector when the path matches (#45990) Currently, the `Router` will create the `EnvironmentInjector` for a `Route` with `providers` as soon as it _attempts_ to match segments to the `Route`. Instead, this change updates the logic to only create the injector once we have confirmed that the `Route` matches. Note that this will include partial matches where the final navigation still fails with a "cannot match any routes to segment" error. We need to maintain the injector hierarchy so we still need to create parent injectors before descending into the child routes and determining if there is a full match or just a partial one. resolves #45988 PR Close #45990 --- .../router/bundle.golden_symbols.json | 3 ++ packages/router/src/apply_redirects.ts | 21 ++++------ packages/router/src/utils/config.ts | 19 ++++++++- packages/router/test/apply_redirects.spec.ts | 41 ++++++++++++++++++- 4 files changed, 69 insertions(+), 15 deletions(-) diff --git a/packages/core/test/bundling/router/bundle.golden_symbols.json b/packages/core/test/bundling/router/bundle.golden_symbols.json index c9fcbcff5fc..c426a897615 100644 --- a/packages/core/test/bundling/router/bundle.golden_symbols.json +++ b/packages/core/test/bundling/router/bundle.golden_symbols.json @@ -1247,6 +1247,9 @@ { "name": "getOrCreateNodeInjectorForNode" }, + { + "name": "getOrCreateRouteInjectorIfNeeded" + }, { "name": "getOrCreateTComponentView" }, diff --git a/packages/router/src/apply_redirects.ts b/packages/router/src/apply_redirects.ts index a0537aef78f..359bea0d55d 100644 --- a/packages/router/src/apply_redirects.ts +++ b/packages/router/src/apply_redirects.ts @@ -6,8 +6,8 @@ * found in the LICENSE file at https://angular.io/license */ -import {createEnvironmentInjector, EnvironmentInjector} from '@angular/core'; -import {EmptyError, from, Observable, Observer, of, throwError} from 'rxjs'; +import {EnvironmentInjector} from '@angular/core'; +import {EmptyError, from, Observable, of, throwError} from 'rxjs'; import {catchError, concatMap, first, last, map, mergeMap, scan, tap} from 'rxjs/operators'; import {CanLoadFn, LoadedRouterConfig, Route, Routes} from './models'; @@ -16,7 +16,7 @@ import {RouterConfigLoader} from './router_config_loader'; import {navigationCancelingError, Params, PRIMARY_OUTLET} from './shared'; import {UrlSegment, UrlSegmentGroup, UrlSerializer, UrlTree} from './url_tree'; import {forEach, wrapIntoObservable} from './utils/collection'; -import {getOutlet, sortByMatchingOutlets} from './utils/config'; +import {getOrCreateRouteInjectorIfNeeded, getOutlet, sortByMatchingOutlets} from './utils/config'; import {isImmediateMatch, match, noLeftoversInUrl, split} from './utils/config_matching'; import {isCanLoad, isFunction, isUrlTree} from './utils/type_guards'; @@ -183,17 +183,8 @@ class ApplyRedirects { allowRedirects: boolean): Observable { return from(routes).pipe( concatMap(r => { - if (r.providers && !r._injector) { - r._injector = createEnvironmentInjector(r.providers, injector, `Route: ${r.path}`); - } - // We specifically _do not_ want to include the _loadedInjector here. The loaded injector - // only applies to the route's children, not the route itself. Note that this distinction - // only applies here to any tokens we try to retrieve during this phase. At the moment, - // that only includes `canLoad`, which won't run again once the child module is loaded. As - // a result, this makes no difference right now, but could in the future if there are more - // actions here that need DI (for example, a canMatch guard). const expanded$ = this.expandSegmentAgainstRoute( - r._injector ?? injector, segmentGroup, routes, r, segments, outlet, allowRedirects); + injector, segmentGroup, routes, r, segments, outlet, allowRedirects); return expanded$.pipe(catchError((e: any) => { if (e instanceof NoMatch) { return of(null); @@ -280,6 +271,8 @@ class ApplyRedirects { injector: EnvironmentInjector, rawSegmentGroup: UrlSegmentGroup, route: Route, segments: UrlSegment[], outlet: string): Observable { if (route.path === '**') { + // Only create the Route's `EnvironmentInjector` if it matches the attempted navigation + injector = getOrCreateRouteInjectorIfNeeded(route, injector); if (route.loadChildren) { const loaded$ = route._loadedRoutes ? of({routes: route._loadedRoutes, injector: route._loadedInjector}) : @@ -297,6 +290,8 @@ class ApplyRedirects { const {matched, consumedSegments, remainingSegments} = match(rawSegmentGroup, route, segments); if (!matched) return noMatch(rawSegmentGroup); + // Only create the Route's `EnvironmentInjector` if it matches the attempted navigation + injector = getOrCreateRouteInjectorIfNeeded(route, injector); const childConfig$ = this.getChildConfig(injector, route, segments); return childConfig$.pipe(mergeMap((routerConfig: LoadedRouterConfig) => { diff --git a/packages/router/src/utils/config.ts b/packages/router/src/utils/config.ts index 9ecbcc78c2c..0d19e80f452 100644 --- a/packages/router/src/utils/config.ts +++ b/packages/router/src/utils/config.ts @@ -6,13 +6,30 @@ * found in the LICENSE file at https://angular.io/license */ -import {EnvironmentInjector, Type, ɵisStandalone as isStandalone} from '@angular/core'; +import {createEnvironmentInjector, EnvironmentInjector, Type, ɵisStandalone as isStandalone} from '@angular/core'; import {EmptyOutletComponent} from '../components/empty_outlet'; import {Route, Routes} from '../models'; import {ActivatedRouteSnapshot} from '../router_state'; import {PRIMARY_OUTLET} from '../shared'; +/** + * Creates an `EnvironmentInjector` if the `Route` has providers and one does not already exist + * and returns the injector. Otherwise, if the `Route` does not have `providers`, returns the + * `currentInjector`. + * + * @param route The route that might have providers + * @param currentInjector The parent injector of the `Route` + */ +export function getOrCreateRouteInjectorIfNeeded( + route: Route, currentInjector: EnvironmentInjector) { + if (route.providers && !route._injector) { + route._injector = + createEnvironmentInjector(route.providers, currentInjector, `Route: ${route.path}`); + } + return route._injector ?? currentInjector; +} + export function getLoadedRoutes(route: Route): Route[]|undefined { return route._loadedRoutes; } diff --git a/packages/router/test/apply_redirects.spec.ts b/packages/router/test/apply_redirects.spec.ts index b1e9ac8de3d..a6d7d16eb9f 100644 --- a/packages/router/test/apply_redirects.spec.ts +++ b/packages/router/test/apply_redirects.spec.ts @@ -15,7 +15,7 @@ import {applyRedirects} from '../src/apply_redirects'; import {Route, Routes} from '../src/models'; import {RouterConfigLoader} from '../src/router_config_loader'; import {DefaultUrlSerializer, equalSegments, UrlSegment, UrlSegmentGroup, UrlTree} from '../src/url_tree'; -import {getLoadedRoutes} from '../src/utils/config'; +import {getLoadedRoutes, getProvidersInjector} from '../src/utils/config'; describe('applyRedirects', () => { const serializer = new DefaultUrlSerializer(); @@ -190,6 +190,45 @@ describe('applyRedirects', () => { }); }); + it('should not create injector for Route if the route does not match', () => { + const routes = [ + {path: '', pathMatch: 'full' as const, providers: []}, + { + path: 'a', + component: ComponentA, + children: [ + {path: 'b', component: ComponentB}, + ], + }, + ]; + checkRedirect(routes, '/a/b', (t: UrlTree) => { + expectTreeToBe(t, '/a/b'); + expect(getProvidersInjector(routes[0])).not.toBeDefined(); + }); + }); + + it('should create injectors for partial Route route matches', () => { + const routes = [ + { + path: 'a', + component: ComponentA, + providers: [], + }, + {path: 'doesNotMatch', providers: []}, + ]; + applyRedirects(testModule.injector, null!, serializer, tree('a/b/c'), routes).subscribe({ + next: () => { + throw 'Should not be reached'; + }, + error: () => { + // The 'a' segment matched, so we needed to create the injector for the `Route` + expect(getProvidersInjector(routes[0])).toBeDefined(); + // The second `Route` did not match at all so we should not create an injector for it + expect(getProvidersInjector(routes[1])).not.toBeDefined(); + } + }); + }); + describe('lazy loading', () => { it('should load config on demand', () => { const loadedConfig = {