fix(router): canceledNavigationResolution: 'computed' with redirects to the current URL (#49793)

atscott · thePunderWoman · commit cbca5817d8ff · 2023-04-14T14:55:06.000Z
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
diff --git 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
@@ -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');
