From a0551ee761165ec2cf523a53165bcb77d7601c2e Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Wed, 23 Nov 2022 15:07:35 -0800 Subject: [PATCH] refactor(router): Eliminate constructor parameters in Router class (#48215) The Router constructor and `setupRouter` factory mainly exist as a legacy configuration. Since the Router's creation, the style in Angular has evolved quite a bit. This commit eliminates and cleans up some unnecessary comlicated code paths related to the router constructor/factory. Note that there are edits to the `setupTestingRouter` that could be seen as breaking. However, it is not for several reasons: 1. The function is documented as a factory function. If used as documented, the parameters should match what's available in DI 2. The function is totally unused by the Router itself and is not used in g3 either. I believe it was made publicApi by an error when updating documentation annotations long ago. PR Close #48215 --- goldens/public-api/router/index.md | 7 +-- packages/router/src/router.ts | 59 +++++-------------- packages/router/src/router_module.ts | 4 +- .../testing/src/router_testing_module.ts | 34 ++++++++++- 4 files changed, 50 insertions(+), 54 deletions(-) diff --git a/goldens/public-api/router/index.md b/goldens/public-api/router/index.md index 05dee41d1ce..2f97b9dc09c 100644 --- a/goldens/public-api/router/index.md +++ b/goldens/public-api/router/index.md @@ -16,7 +16,6 @@ import { EventEmitter } from '@angular/core'; import * as i0 from '@angular/core'; import { InjectionToken } from '@angular/core'; import { Injector } from '@angular/core'; -import { Location as Location_2 } from '@angular/common'; import { LocationStrategy } from '@angular/common'; import { ModuleWithProviders } from '@angular/core'; import { NgModuleFactory } from '@angular/core'; @@ -655,11 +654,7 @@ export class RouteConfigLoadStart { // @public export class Router { - constructor( - rootComponentType: Type | null, - urlSerializer: UrlSerializer, - rootContexts: ChildrenOutletContexts, - location: Location_2, injector: Injector, compiler: Compiler, config: Routes); + constructor(); // @deprecated canceledNavigationResolution: 'replace' | 'computed'; // (undocumented) diff --git a/packages/router/src/router.ts b/packages/router/src/router.ts index d9e2fb1c56c..1f7a066adf8 100644 --- a/packages/router/src/router.ts +++ b/packages/router/src/router.ts @@ -87,22 +87,6 @@ export function assignExtraOptionsToRouter(opts: ExtraOptions, router: Router): } } -export function setupRouter() { - const urlSerializer = inject(UrlSerializer); - const contexts = inject(ChildrenOutletContexts); - const location = inject(Location); - const injector = inject(Injector); - const compiler = inject(Compiler); - const config = inject(ROUTES, {optional: true}) ?? []; - const opts = inject(ROUTER_CONFIGURATION, {optional: true}) ?? {}; - const router = - new Router(null, urlSerializer, contexts, location, injector, compiler, flatten(config)); - - assignExtraOptionsToRouter(opts, router); - - return router; -} - /** * @description * @@ -115,10 +99,7 @@ export function setupRouter() { * * @publicApi */ -@Injectable({ - providedIn: 'root', - useFactory: setupRouter, -}) +@Injectable({providedIn: 'root'}) export class Router { /** * Represents the activated `UrlTree` that the `Router` is configured to handle (through @@ -201,7 +182,7 @@ export class Router { private get browserPageId(): number|undefined { return (this.location.getState() as RestoredState | null)?.ɵrouterPageId; } - private console: Console; + private console = inject(Console); private isNgZoneEnabled: boolean = false; /** @@ -349,27 +330,22 @@ export class Router { */ canceledNavigationResolution: 'replace'|'computed' = 'replace'; + config: Routes = flatten(inject(ROUTES, {optional: true}) ?? []); + private readonly navigationTransitions = inject(NavigationTransitions); + private readonly urlSerializer = inject(UrlSerializer); + private readonly location = inject(Location); - /** - * Creates the router service. - */ - // TODO: vsavkin make internal after the final is out. - constructor( - /** @internal */ - public rootComponentType: Type|null, - private readonly urlSerializer: UrlSerializer, - private readonly rootContexts: ChildrenOutletContexts, - private readonly location: Location, - injector: Injector, - compiler: Compiler, - public config: Routes, - ) { - this.console = injector.get(Console); - const ngZone = injector.get(NgZone); - this.isNgZoneEnabled = ngZone instanceof NgZone && NgZone.isInAngularZone(); + /** @internal */ + rootComponentType: Type|null = null; - this.resetConfig(config); + constructor() { + const opts = inject(ROUTER_CONFIGURATION, {optional: true}) ?? {}; + assignExtraOptionsToRouter(opts, this); + + this.isNgZoneEnabled = inject(NgZone) instanceof NgZone && NgZone.isInAngularZone(); + + this.resetConfig(this.config); this.currentUrlTree = new UrlTree(); this.rawUrlTree = this.currentUrlTree; this.browserUrlTree = this.currentUrlTree; @@ -386,10 +362,7 @@ export class Router { }); } - /** - * @internal - * TODO: this should be removed once the constructor of the router made internal - */ + /** @internal */ resetRootComponentType(rootComponentType: Type): void { this.rootComponentType = rootComponentType; // TODO: vsavkin router 4.0 should make the root component set to null diff --git a/packages/router/src/router_module.ts b/packages/router/src/router_module.ts index 14dcd7b1fd2..7871d48a100 100644 --- a/packages/router/src/router_module.ts +++ b/packages/router/src/router_module.ts @@ -17,7 +17,7 @@ import {RuntimeErrorCode} from './errors'; import {Routes} from './models'; import {NavigationTransitions} from './navigation_transition'; import {getBootstrapListener, rootRoute, ROUTER_IS_PROVIDED, withDebugTracing, withDisabledInitialNavigation, withEnabledBlockingInitialNavigation, withPreloading} from './provide_router'; -import {Router, setupRouter} from './router'; +import {Router} from './router'; import {ExtraOptions, ROUTER_CONFIGURATION} from './router_config'; import {RouterConfigLoader, ROUTES} from './router_config_loader'; import {ChildrenOutletContexts} from './router_outlet_context'; @@ -45,7 +45,7 @@ export const ROUTER_FORROOT_GUARD = new InjectionToken( export const ROUTER_PROVIDERS: Provider[] = [ Location, {provide: UrlSerializer, useClass: DefaultUrlSerializer}, - {provide: Router, useFactory: setupRouter}, + Router, ChildrenOutletContexts, {provide: ActivatedRoute, useFactory: rootRoute, deps: [Router]}, RouterConfigLoader, diff --git a/packages/router/testing/src/router_testing_module.ts b/packages/router/testing/src/router_testing_module.ts index 3cbf2bfdc85..5291361317e 100644 --- a/packages/router/testing/src/router_testing_module.ts +++ b/packages/router/testing/src/router_testing_module.ts @@ -8,7 +8,7 @@ import {Location} from '@angular/common'; import {provideLocationMocks} from '@angular/common/testing'; -import {Compiler, Injector, ModuleWithProviders, NgModule, Optional} from '@angular/core'; +import {Compiler, inject, Injector, ModuleWithProviders, NgModule} from '@angular/core'; import {ChildrenOutletContexts, ExtraOptions, NoPreloading, Route, Router, ROUTER_CONFIGURATION, RouteReuseStrategy, RouterModule, ROUTES, Routes, TitleStrategy, UrlHandlingStrategy, UrlSerializer, ɵassignExtraOptionsToRouter as assignExtraOptionsToRouter, ɵflatten as flatten, ɵROUTER_PROVIDERS as ROUTER_PROVIDERS, ɵwithPreloading as withPreloading} from '@angular/router'; import {EXTRA_ROUTER_TESTING_PROVIDERS} from './extra_router_testing_providers'; @@ -20,6 +20,12 @@ function isUrlHandlingStrategy(opts: ExtraOptions| return 'shouldProcessUrl' in opts; } +function throwInvalidConfigError(parameter: string): never { + throw new Error( + `Parameter ${parameter} does not match the one available in the injector. ` + + '`setupTestingRouter` is meant to be used as a factory function with dependencies coming from DI.'); +} + /** * Router setup factory function used for testing. * @@ -31,8 +37,30 @@ export function setupTestingRouter( compiler: Compiler, injector: Injector, routes: Route[][], opts?: ExtraOptions|UrlHandlingStrategy|null, urlHandlingStrategy?: UrlHandlingStrategy, routeReuseStrategy?: RouteReuseStrategy, titleStrategy?: TitleStrategy) { - const router = - new Router(null!, urlSerializer, contexts, location, injector, compiler, flatten(routes)); + // Note: The checks below are to detect misconfigured providers and invalid uses of + // `setupTestingRouter`. This function is not used internally (neither in router code or anywhere + // in g3). It appears this function was exposed as publicApi by mistake and should not be used + // externally either. However, if it is, the documented intent is to be used as a factory function + // and parameter values should always match what's available in DI. + const router = new Router(); + if (urlSerializer !== inject(UrlSerializer)) { + throwInvalidConfigError('urlSerializer'); + } + if (contexts !== inject(ChildrenOutletContexts)) { + throwInvalidConfigError('contexts'); + } + if (location !== inject(Location)) { + throwInvalidConfigError('location'); + } + if (compiler !== inject(Compiler)) { + throwInvalidConfigError('compiler'); + } + if (injector !== inject(Injector)) { + throwInvalidConfigError('injector'); + } + if (routes !== inject(ROUTES)) { + throwInvalidConfigError('routes'); + } if (opts) { // Handle deprecated argument ordering. if (isUrlHandlingStrategy(opts)) {