From 79700822fd69b1d76d2c364b713b8663ffe6bd89 Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Mon, 13 Sep 2021 09:25:09 -0700 Subject: [PATCH] refactor(router): remove unnecessary getTransition function (#43436) The getTransition helper function ensures that urlAfterRedirects is assigned for when the router does not process the previous or current URL. In this case, we would set the browserUrlTree to be the urlAfterRedirects, which was initialized to be the browserUrlTree. This is a no-op, so there's no need for the getTransition function at all. PR Close #43436 --- packages/router/src/operators/recognize.ts | 2 +- packages/router/src/router.ts | 92 +++++++++------------- packages/router/test/integration.spec.ts | 33 -------- 3 files changed, 38 insertions(+), 89 deletions(-) diff --git a/packages/router/src/operators/recognize.ts b/packages/router/src/operators/recognize.ts index 1a82f3921f3..b6140bec295 100644 --- a/packages/router/src/operators/recognize.ts +++ b/packages/router/src/operators/recognize.ts @@ -21,7 +21,7 @@ export function recognize( relativeLinkResolution: 'legacy'|'corrected'): MonoTypeOperatorFunction { return mergeMap( t => recognizeFn( - rootComponentType, config, t.urlAfterRedirects, serializer(t.urlAfterRedirects), + rootComponentType, config, t.urlAfterRedirects!, serializer(t.urlAfterRedirects!), paramsInheritanceStrategy, relativeLinkResolution) .pipe(map(targetSnapshot => ({...t, targetSnapshot})))); } diff --git a/packages/router/src/router.ts b/packages/router/src/router.ts index 8a2115c2821..003315f2fca 100644 --- a/packages/router/src/router.ts +++ b/packages/router/src/router.ts @@ -305,27 +305,27 @@ export interface Navigation { previousNavigation: Navigation|null; } -export type NavigationTransition = { - id: number, - targetPageId: number, - currentUrlTree: UrlTree, - currentRawUrl: UrlTree, - extractedUrl: UrlTree, - urlAfterRedirects: UrlTree, - rawUrl: UrlTree, - extras: NavigationExtras, - resolve: any, - reject: any, - promise: Promise, - source: NavigationTrigger, - restoredState: RestoredState|null, - currentSnapshot: RouterStateSnapshot, - targetSnapshot: RouterStateSnapshot|null, - currentRouterState: RouterState, - targetRouterState: RouterState|null, - guards: Checks, - guardsResult: boolean|UrlTree|null, -}; +export interface NavigationTransition { + id: number; + targetPageId: number; + currentUrlTree: UrlTree; + currentRawUrl: UrlTree; + extractedUrl: UrlTree; + urlAfterRedirects?: UrlTree; + rawUrl: UrlTree; + extras: NavigationExtras; + resolve: any; + reject: any; + promise: Promise; + source: NavigationTrigger; + restoredState: RestoredState|null; + currentSnapshot: RouterStateSnapshot; + targetSnapshot: RouterStateSnapshot|null; + currentRouterState: RouterState; + targetRouterState: RouterState|null; + guards: Checks; + guardsResult: boolean|UrlTree|null; +} /** * @internal @@ -695,6 +695,7 @@ export class Router { this.config), // Update the currentNavigation + // `urlAfterRedirects` is guaranteed to be set after this point tap(t => { this.currentNavigation = { ...this.currentNavigation!, @@ -713,16 +714,16 @@ export class Router { if (this.urlUpdateStrategy === 'eager') { if (!t.extras.skipLocationChange) { const rawUrl = this.urlHandlingStrategy.merge( - t.urlAfterRedirects, t.rawUrl); + t.urlAfterRedirects!, t.rawUrl); this.setBrowserUrl(rawUrl, t); } - this.browserUrlTree = t.urlAfterRedirects; + this.browserUrlTree = t.urlAfterRedirects!; } // Fire RoutesRecognized const routesRecognized = new RoutesRecognized( t.id, this.serializeUrl(t.extractedUrl), - this.serializeUrl(t.urlAfterRedirects), t.targetSnapshot!); + this.serializeUrl(t.urlAfterRedirects!), t.targetSnapshot!); eventsSubject.next(routesRecognized); })); } else { @@ -752,7 +753,6 @@ export class Router { * in the browser. */ this.rawUrlTree = t.rawUrl; - this.browserUrlTree = t.urlAfterRedirects; t.resolve(null); return EMPTY; } @@ -781,7 +781,7 @@ export class Router { tap(t => { const guardsStart = new GuardsCheckStart( t.id, this.serializeUrl(t.extractedUrl), - this.serializeUrl(t.urlAfterRedirects), t.targetSnapshot!); + this.serializeUrl(t.urlAfterRedirects!), t.targetSnapshot!); this.triggerEvent(guardsStart); }), @@ -802,7 +802,7 @@ export class Router { const guardsEnd = new GuardsCheckEnd( t.id, this.serializeUrl(t.extractedUrl), - this.serializeUrl(t.urlAfterRedirects), t.targetSnapshot!, + this.serializeUrl(t.urlAfterRedirects!), t.targetSnapshot!, !!t.guardsResult); this.triggerEvent(guardsEnd); }), @@ -823,7 +823,7 @@ export class Router { tap(t => { const resolveStart = new ResolveStart( t.id, this.serializeUrl(t.extractedUrl), - this.serializeUrl(t.urlAfterRedirects), t.targetSnapshot!); + this.serializeUrl(t.urlAfterRedirects!), t.targetSnapshot!); this.triggerEvent(resolveStart); }), switchMap(t => { @@ -847,7 +847,7 @@ export class Router { tap(t => { const resolveEnd = new ResolveEnd( t.id, this.serializeUrl(t.extractedUrl), - this.serializeUrl(t.urlAfterRedirects), t.targetSnapshot!); + this.serializeUrl(t.urlAfterRedirects!), t.targetSnapshot!); this.triggerEvent(resolveEnd); })); } @@ -884,9 +884,9 @@ export class Router { URL and the RouterState, as well as updated the browser URL. All this should happen *before* activating. */ tap((t: NavigationTransition) => { - this.currentUrlTree = t.urlAfterRedirects; + this.currentUrlTree = t.urlAfterRedirects!; this.rawUrlTree = - this.urlHandlingStrategy.merge(t.urlAfterRedirects, t.rawUrl); + this.urlHandlingStrategy.merge(t.urlAfterRedirects!, t.rawUrl); (this as {routerState: RouterState}).routerState = t.targetRouterState!; @@ -894,7 +894,7 @@ export class Router { if (!t.extras.skipLocationChange) { this.setBrowserUrl(this.rawUrlTree, t); } - this.browserUrlTree = t.urlAfterRedirects; + this.browserUrlTree = t.urlAfterRedirects!; } }), @@ -1036,24 +1036,8 @@ export class Router { this.routerState.root.component = this.rootComponentType; } - private getTransition(): NavigationTransition { - const transition = this.transitions.value; - // TODO(atscott): This comment doesn't make it clear why this value needs to be set. In the case - // described below (where we don't handle previous or current url), the `browserUrlTree` is set - // to the `urlAfterRedirects` value. However, these values *are already the same* because of the - // line below. So it seems that we should be able to remove the line below and the line where - // `browserUrlTree` is updated when we aren't handling any part of the navigation url. - // Run TGP to confirm that this can be done. - - // This value needs to be set. Other values such as extractedUrl are set on initial navigation - // but the urlAfterRedirects may not get set if we aren't processing the new URL *and* not - // processing the previous URL. - transition.urlAfterRedirects = this.browserUrlTree; - return transition; - } - private setTransition(t: Partial): void { - this.transitions.next({...this.getTransition(), ...t}); + this.transitions.next({...this.transitions.value, ...t}); } /** @@ -1389,16 +1373,14 @@ export class Router { return Promise.resolve(false); } - // * Imperative navigations (router.navigate) might trigger additional navigations to the same - // URL via a popstate event and the locationChangeListener. We should skip these duplicate - // navs. Duplicates may also be triggered by attempts to sync AngularJS and Angular router - // states. + // * Duplicate navigations may also be triggered by attempts to sync AngularJS and Angular + // router states. // * Imperative navigations can be cancelled by router guards, meaning the URL won't change. If // the user follows that with a navigation using the back/forward button or manual URL change, // the destination may be the same as the previous imperative attempt. We should not skip // these navigations because it's a separate case from the one above -- it's not a duplicate // navigation. - const lastNavigation = this.getTransition(); + const lastNavigation = this.transitions.value; // We don't want to skip duplicate successful navs if they're imperative because // onSameUrlNavigation could be 'reload' (so the duplicate is intended). const browserNavPrecededByRouterNav = isBrowserTriggeredNavigation(source) && lastNavigation && @@ -1408,7 +1390,7 @@ export class Router { // However, if it failed, we should compare to the final result (urlAfterRedirects). const lastNavigationUrl = (lastNavigationSucceeded || this.currentNavigation) ? lastNavigation.rawUrl : - lastNavigation.urlAfterRedirects; + (lastNavigation.urlAfterRedirects ?? this.browserUrlTree); const duplicateNav = lastNavigationUrl.toString() === rawUrl.toString(); if (browserNavPrecededByRouterNav && duplicateNav) { return Promise.resolve(true); // return value is not used diff --git a/packages/router/test/integration.spec.ts b/packages/router/test/integration.spec.ts index 3f2c909b52b..9095d12dab1 100644 --- a/packages/router/test/integration.spec.ts +++ b/packages/router/test/integration.spec.ts @@ -314,39 +314,6 @@ describe('Integration', () => { }))); }); - - /** - * get/setTransition are private APIs. This test is needed though to guarantee the correct - * values are being used. Related to https://github.com/angular/angular/issues/30340 where - * stale transition data was being used when kicking off a new navigation. - */ - describe('get/setTransition', () => { - it('should provide the most recent NavigationTransition', - fakeAsync(inject([Router, Location], (router: Router, location: SpyLocation) => { - router.resetConfig([ - {path: '', component: SimpleCmp}, {path: 'a', component: SimpleCmp}, - {path: 'b', component: SimpleCmp} - ]); - - const fixture = createRoot(router, RootCmp); - - const initialTransition = (router as any).getTransition(); - - // Confirm initial value - expect(initialTransition.urlAfterRedirects.toString()).toBe('/'); - - - router.navigateByUrl('/a', {replaceUrl: true}); - - tick(); - - // After a navigation, we should see the URL after redirect - const nextTransition = (router as any).getTransition(); - // Confirm initial value - expect(nextTransition.urlAfterRedirects.toString()).toBe('/a'); - }))); - }); - describe('navigation warning', () => { const isInAngularZoneFn = NgZone.isInAngularZone; let warnings: string[] = [];