From 98223c11caab3f4fefa8530b72dcd680280afe71 Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Tue, 11 Apr 2023 13:27:10 -0700 Subject: [PATCH] fix(router): canceledNavigationResolution: 'computed' with redirects to the current URL (#49793) The `canceledNavigationResolution: 'computed'` option does not correctly assign page IDs or restore them when redirects result in navigating to the current URL. This change ensures that the page IDs are still incremented and restored correctly in this scenario. PR Close #49793 --- packages/router/src/router.ts | 40 +++++++-------- .../test/computed_state_restoration.spec.ts | 49 +++++++++++++++++++ 2 files changed, 67 insertions(+), 22 deletions(-) diff --git a/packages/router/src/router.ts b/packages/router/src/router.ts index 5f68622214e..3e1dc26a5f6 100644 --- a/packages/router/src/router.ts +++ b/packages/router/src/router.ts @@ -329,7 +329,7 @@ export class Router { this.navigationTransitions.setupNavigations(this).subscribe( t => { this.lastSuccessfulId = t.id; - this.currentPageId = t.targetPageId; + this.currentPageId = this.browserPageId ?? 0; }, e => { this.console.warn(`Unhandled Navigation Error: ${e}`); @@ -733,13 +733,9 @@ export class Router { if (restoredState && restoredState.ɵrouterPageId) { targetPageId = restoredState.ɵrouterPageId; } else { - // If we're replacing the URL or doing a silent navigation, we do not want to increment the - // page id because we aren't pushing a new entry to history. - if (extras.replaceUrl || extras.skipLocationChange) { - targetPageId = this.browserPageId ?? 0; - } else { - targetPageId = (this.browserPageId ?? 0) + 1; - } + // Otherwise, targetPageId should be the next number in the event of a `pushState` + // navigation. + targetPageId = (this.browserPageId ?? 0) + 1; } } else { // This is unused when `canceledNavigationResolution` is not computed. @@ -779,13 +775,20 @@ export class Router { /** @internal */ setBrowserUrl(url: UrlTree, transition: NavigationTransition) { const path = this.urlSerializer.serialize(url); - const state = { - ...transition.extras.state, - ...this.generateNgRouterState(transition.id, transition.targetPageId) - }; if (this.location.isCurrentPathEqualTo(path) || !!transition.extras.replaceUrl) { + // replacements do not update the target page + const currentBrowserPageId = + this.canceledNavigationResolution === 'computed' ? this.browserPageId : undefined; + const state = { + ...transition.extras.state, + ...this.generateNgRouterState(transition.id, currentBrowserPageId) + }; this.location.replaceState(path, '', state); } else { + const state = { + ...transition.extras.state, + ...this.generateNgRouterState(transition.id, transition.targetPageId) + }; this.location.go(path, '', state); } } @@ -797,16 +800,9 @@ export class Router { */ restoreHistory(transition: NavigationTransition, restoringFromCaughtError = false) { if (this.canceledNavigationResolution === 'computed') { - const targetPagePosition = this.currentPageId - transition.targetPageId; - // The navigator change the location before triggered the browser event, - // so we need to go back to the current url if the navigation is canceled. - // Also, when navigation gets cancelled while using url update strategy eager, then we need to - // go back. Because, when `urlUpdateStrategy` is `eager`; `setBrowserUrl` method is called - // before any verification. - const browserUrlUpdateOccurred = - (transition.source === 'popstate' || this.urlUpdateStrategy === 'eager' || - this.currentUrlTree === this.getCurrentNavigation()?.finalUrl); - if (browserUrlUpdateOccurred && targetPagePosition !== 0) { + const currentBrowserPageId = this.browserPageId ?? this.currentPageId; + const targetPagePosition = this.currentPageId - currentBrowserPageId; + if (targetPagePosition !== 0) { this.location.historyGo(targetPagePosition); } else if ( this.currentUrlTree === this.getCurrentNavigation()?.finalUrl && diff --git a/packages/router/test/computed_state_restoration.spec.ts b/packages/router/test/computed_state_restoration.spec.ts index cbd8fedeceb..3831113242d 100644 --- a/packages/router/test/computed_state_restoration.spec.ts +++ b/packages/router/test/computed_state_restoration.spec.ts @@ -357,6 +357,55 @@ describe('`restoredState#ɵrouterPageId`', () => { const location = TestBed.inject(Location); const router = TestBed.inject(Router); router.urlUpdateStrategy = 'eager'; + let allowNavigation = true; + router.resetConfig([ + {path: 'initial', children: []}, + {path: 'redirectFrom', redirectTo: 'redirectTo'}, + {path: 'redirectTo', children: [], canActivate: [() => allowNavigation]}, + ]); + + // already at '2' from the `beforeEach` navigations + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 2})); + router.navigateByUrl('/initial'); + advance(fixture); + expect(location.path()).toEqual('/initial'); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 3})); + + TestBed.inject(MyCanActivateGuard).redirectTo = null; + + router.navigateByUrl('redirectTo'); + advance(fixture); + expect(location.path()).toEqual('/redirectTo'); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 4})); + + // Navigate to different URL but get redirected to same URL should result in same page id + router.navigateByUrl('redirectFrom'); + advance(fixture); + expect(location.path()).toEqual('/redirectTo'); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 4})); + + // Back and forward should have page IDs 1 apart + location.back(); + advance(fixture); + expect(location.path()).toEqual('/initial'); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 3})); + location.forward(); + advance(fixture); + expect(location.path()).toEqual('/redirectTo'); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 4})); + + // Rejected navigation after redirect to same URL should have the same page ID + allowNavigation = false; + router.navigateByUrl('redirectFrom'); + advance(fixture); + expect(location.path()).toEqual('/redirectTo'); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 4})); + })); + + it('urlUpdateStrategy="eager", redirectTo with same url, and guard reject', fakeAsync(() => { + const location = TestBed.inject(Location); + const router = TestBed.inject(Router); + router.urlUpdateStrategy = 'eager'; TestBed.inject(MyCanActivateGuard).redirectTo = router.createUrlTree(['unguarded']); router.navigateByUrl('/third');