From a3ee4748975f6f538f948983b3782630f2630f97 Mon Sep 17 00:00:00 2001
From: Andrew Scott <atscott@google.com>
Date: Tue, 28 Sep 2021 16:34:45 -0700
Subject: [PATCH] refactor(router): Remove use of browserUrlTree in
 scheduleNavigation (#43632)

There are no added tests here because the change is generally equivalent
to what was there before. The goal of that piece of code is to prevent
duplicate navigations due to the location synchronization code between
AngularJS and Angular. That is, the Angular router listens to the
`popstate` event and triggers navigations, but so does the AngularJS
router and triggers navigations through `router.navigateByUrl`. The
`setTimeout` in the Angular Router's `setUpLocationChangeListener` is
there to make this bit of code work: the 'popstate'/'hashchange'
navigation will necessarily come after the imperative nav triggered by
AngularJS's location sync.

Anyhow, in the long run, I would like to get rid of this bit of code
altogether. We should not have special handling for these cases. The
AngularJS/Angular location sync should either:

1. Not trigger a duplicate navigation in the first place
or
2. Be tolerant to processing the duplicate navigation.

In the short term, this change benefits the router while we investigate
removing this logic completely because now browserUrlTree has a single
responsiblity: to support `onSameUrlNavigation==='reload'`. This means
that we can work towards removing the `browserUrlTree` tracking from the
Router code at some point as well and have the `onSameUrlNavigation`
logic be part of a pre-navigation hook that can be handled outside the
`Router`. This may or may not be possible, but the change here opens up
that change for investigation.

PR Close #43632
---
 packages/router/src/router.ts | 26 +++++++++-----------------
 1 file changed, 9 insertions(+), 17 deletions(-)

diff --git a/packages/router/src/router.ts b/packages/router/src/router.ts
index 20e5736e8c7..9c532512965 100644
--- a/packages/router/src/router.ts
+++ b/packages/router/src/router.ts
@@ -1353,26 +1353,18 @@ export class Router {
       return Promise.resolve(false);
     }
 
-    // * 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.
+    // Duplicate navigations may be triggered by attempts to sync AngularJS and
+    // Angular router states. We have the setTimeout in the location listener to
+    // ensure the imperative nav is scheduled before the browser nav.
     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 &&
         !isBrowserTriggeredNavigation(lastNavigation.source);
-    const lastNavigationSucceeded = this.lastSuccessfulId === lastNavigation.id;
-    // If the last navigation succeeded or is in flight, we can use the rawUrl as the comparison.
-    // However, if it failed, we should compare to the final result (urlAfterRedirects).
-    const lastNavigationUrl = (lastNavigationSucceeded || this.currentNavigation) ?
-        lastNavigation.rawUrl :
-        (lastNavigation.urlAfterRedirects ?? this.browserUrlTree);
-    const duplicateNav = lastNavigationUrl.toString() === rawUrl.toString();
-    if (browserNavPrecededByRouterNav && duplicateNav) {
+    const navToSameUrl = lastNavigation.rawUrl.toString() === rawUrl.toString();
+    const lastNavigationInProgress = lastNavigation.id === this.currentNavigation?.id;
+    // We consider duplicates as ones that goes to the same URL while the first
+    // is still processing.
+    const isDuplicateNav = navToSameUrl && lastNavigationInProgress;
+    if (browserNavPrecededByRouterNav && isDuplicateNav) {
       return Promise.resolve(true);  // return value is not used
     }