From 15e397816f98ec16839c30fd5c1ea01c7444fb84 Mon Sep 17 00:00:00 2001
From: Jason Aden <jasonaden@google.com>
Date: Thu, 20 Jun 2019 10:40:42 -0700
Subject: [PATCH] fix(router): adjust UrlTree redirect to replace URL if in
 eager update (#31168)

Without this change when using UrlTree redirects in `urlUpdateStrategy="eager"`, the URL would get updated to the target location, then redirected. This resulted in having an additional entry in the `history` and thus the `back` button would be broken (going back would land on the URL causing a new redirect).

Additionally, there was a bug where the redirect, even without `urlUpdateStrategy="eager"`, could create a history with too many entries. This was due to kicking off a new navigation within the navigation cancelling logic. With this PR the new navigation is pushed to the next tick with a `setTimeout`, allowing the page being redirected from to be cancelled before starting a new navigation.

Related to #27148

PR Close #31168
---
 packages/router/src/router.ts            | 44 ++++++++++++++++++------
 packages/router/test/integration.spec.ts | 33 ++++++++++++++++++
 2 files changed, 67 insertions(+), 10 deletions(-)

diff --git a/packages/router/src/router.ts b/packages/router/src/router.ts
index c19621c0c8..63a417e826 100644
--- a/packages/router/src/router.ts
+++ b/packages/router/src/router.ts
@@ -739,10 +739,26 @@ export class Router {
                   const navCancel =
                       new NavigationCancel(t.id, this.serializeUrl(t.extractedUrl), e.message);
                   eventsSubject.next(navCancel);
-                  t.resolve(false);
 
-                  if (redirecting) {
-                    this.navigateByUrl(e.url);
+                  // When redirecting, we need to delay resolving the navigation promise and push
+                  // it to the redirect navigation
+                  if (!redirecting) {
+                    t.resolve(false);
+                  } else {
+                    // setTimeout is required so this navigation finishes with the return EMPTY
+                    // below. If it isn't allowed to finish processing, there can be multiple
+                    // navigations to the same URL.
+                    setTimeout(() => {
+                      const mergedTree = this.urlHandlingStrategy.merge(e.url, this.rawUrlTree);
+                      const extras = {
+                        skipLocationChange: t.extras.skipLocationChange,
+                        replaceUrl: this.urlUpdateStrategy === 'eager'
+                      };
+
+                      return this.scheduleNavigation(
+                          mergedTree, 'imperative', null, extras,
+                          {resolve: t.resolve, reject: t.reject, promise: t.promise});
+                    }, 0);
                   }
 
                   /* All other errors should reset to the router's internal URL reference to the
@@ -1056,7 +1072,8 @@ export class Router {
 
   private scheduleNavigation(
       rawUrl: UrlTree, source: NavigationTrigger, restoredState: RestoredState|null,
-      extras: NavigationExtras): Promise<boolean> {
+      extras: NavigationExtras,
+      priorPromise?: {resolve: any, reject: any, promise: Promise<boolean>}): Promise<boolean> {
     const lastNavigation = this.getTransition();
     // If the user triggers a navigation imperatively (e.g., by using navigateByUrl),
     // and that navigation results in 'replaceState' that leads to the same URL,
@@ -1081,13 +1098,20 @@ export class Router {
       return Promise.resolve(true);  // return value is not used
     }
 
-    let resolve: any = null;
-    let reject: any = null;
+    let resolve: any;
+    let reject: any;
+    let promise: Promise<boolean>;
+    if (priorPromise) {
+      resolve = priorPromise.resolve;
+      reject = priorPromise.reject;
+      promise = priorPromise.promise;
 
-    const promise = new Promise<boolean>((res, rej) => {
-      resolve = res;
-      reject = rej;
-    });
+    } else {
+      promise = new Promise<boolean>((res, rej) => {
+        resolve = res;
+        reject = rej;
+      });
+    }
 
     const id = ++this.navigationId;
     this.setTransition({
diff --git a/packages/router/test/integration.spec.ts b/packages/router/test/integration.spec.ts
index 3766ef7065..c54ceba954 100644
--- a/packages/router/test/integration.spec.ts
+++ b/packages/router/test/integration.spec.ts
@@ -2464,6 +2464,39 @@ describe('Integration', () => {
                [NavigationEnd, '/'],
              ]);
            })));
+
+        it('replaces URL when URL is updated eagerly so back button can still work',
+           fakeAsync(inject([Router, Location], (router: Router, location: SpyLocation) => {
+             router.urlUpdateStrategy = 'eager';
+             router.resetConfig([
+               {path: '', component: SimpleCmp},
+               {path: 'one', component: RouteCmp, canActivate: ['returnUrlTree']},
+               {path: 'redirected', component: SimpleCmp}
+             ]);
+             const fixture = createRoot(router, RootCmp);
+             router.navigateByUrl('/one');
+
+             tick();
+
+             expect(location.path()).toEqual('/redirected');
+             expect(location.urlChanges).toEqual(['replace: /', '/one', 'replace: /redirected']);
+           })));
+
+        it('should resolve navigateByUrl promise after redirect finishes',
+           fakeAsync(inject([Router, Location], (router: Router, location: SpyLocation) => {
+             let resolvedPath = '';
+             router.urlUpdateStrategy = 'eager';
+             router.resetConfig([
+               {path: '', component: SimpleCmp},
+               {path: 'one', component: RouteCmp, canActivate: ['returnUrlTree']},
+               {path: 'redirected', component: SimpleCmp}
+             ]);
+             const fixture = createRoot(router, RootCmp);
+             router.navigateByUrl('/one').then(v => { resolvedPath = location.path(); });
+
+             tick();
+             expect(resolvedPath).toBe('/redirected');
+           })));
       });
 
       describe('runGuardsAndResolvers', () => {