From e16dab82a153e02faad3d9f99efdacd3989fe6ca Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Wed, 2 Mar 2022 09:10:49 -0800 Subject: [PATCH] refactor(router): Remove special logic for hybrid apps (#45240) This commit removes special (undocumented) logic in the Router code that is meant to prevent duplicate navigations that result from location syncs in AngularJS/Angular hybrid applications. The duplicate navigations can occur when both the Router and the AngularJS sync code detect a location change via a popstate/hashchange event. When this happens, the Angular Router schedules a navigation to sync itself with the browser, but the hybrid listener may also schedule an additional navigation. There are a few reasons this logic should not be included in the Router: * This special logic is not tree shakeable so it introduces a bundle size cost for all applications, most of which don't need it. * There have been many updates to the routing pipeline to tolerate duplicate navigations. That is, duplicate navigations can happen and routing should still complete successfully. * https://github.com/angular/angular/commit/0e8548f667e5fdefa3ac7cdf1ba47e3e17011ffc * https://github.com/angular/angular/commit/9e039ca68bfae5328f3fc1f16fabd7673c466a25 * The logic is really in the wrong place: The hybrid sync code should be the location to handle this. If duplicate navigations are meant to be avoided, the hybrid sync code should have handling to _not_ trigger duplicate navs. * This logic _also_ used to exist because the mock location helper used for test incorrectly triggered popstate events during router navigations. In order to avoid unexpected behavior in tests, this logic needed to be added. This incorrect mocking may also have been put in place because the upgrade module _would_ see a location change event and trigger a duplicate navigation. The location mock has since been updated to match real browser behavior so this is no longer necessary. The upgrade module has also been updated to not trigger duplicate navigations. The following commits are related to this: * https://github.com/angular/angular/commit/202a1a56314af4ddb99c476f974536a10e390319 * https://github.com/angular/angular/commit/c6a93001eb74374b0fbc6aea1286fe1183d21382 Side note: The `setTimeout` in the location change listener is used to ensure the ordering of duplicate navigations was consistent. You can see that the logic being removed here expects the imperative navigation to precede the popstate/hashchange. With the removal of this code, the `setTimeout` no longer serves a purpose. However, it has been found that tests can rely on this behavior (incorrectly) because they expect the navigation to be complete but in reality, it hasn't even started because the test has not flushed the timeout. Removing the timeout would be a breaking change as a result. PR Close #45240 --- packages/router/src/router.ts | 15 --------------- packages/router/test/integration.spec.ts | 20 -------------------- 2 files changed, 35 deletions(-) diff --git a/packages/router/src/router.ts b/packages/router/src/router.ts index 4057b8e8927..1604191f80a 100644 --- a/packages/router/src/router.ts +++ b/packages/router/src/router.ts @@ -1354,21 +1354,6 @@ export class Router { return Promise.resolve(false); } - // 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; - const browserNavPrecededByRouterNav = isBrowserTriggeredNavigation(source) && lastNavigation && - !isBrowserTriggeredNavigation(lastNavigation.source); - 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 - } - let resolve: any; let reject: any; let promise: Promise; diff --git a/packages/router/test/integration.spec.ts b/packages/router/test/integration.spec.ts index bf9c54b2ecd..a560323200d 100644 --- a/packages/router/test/integration.spec.ts +++ b/packages/router/test/integration.spec.ts @@ -1204,26 +1204,6 @@ describe('Integration', () => { }); }); - it('should ignore the duplicate resulting from a location sync', fakeAsync(() => { - const router = TestBed.inject(Router); - const fixture = createRoot(router, RootCmp); - const location = TestBed.inject(Location) as SpyLocation; - router.resetConfig([{path: 'simple', component: SimpleCmp, canActivate: ['in1Second']}]); - - const recordedEvents: any[] = []; - router.events.forEach(e => onlyNavigationStartAndEnd(e) && recordedEvents.push(e)); - - // setTimeout used so this navigation resolves at the same time as the one that results - // from the location PopStateEvent (see Router#setUpLocationChangeListener). - setTimeout(() => { - router.navigateByUrl('/simple'); - }, 0); - location.simulateUrlPop('/simple'); - tick(1000); - advance(fixture); - expectEvents(recordedEvents, [[NavigationStart, '/simple'], [NavigationEnd, '/simple']]); - })); - it('should reset location if a navigation by location is successful', fakeAsync(() => { const router = TestBed.inject(Router); const location = TestBed.inject(Location) as SpyLocation;