Skip to content

Commit 5883e05

Browse files
atscottAndrewKushnir
atscott
authored and
AndrewKushnir
committed
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. * 0e8548f * 9e039ca * 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: * 202a1a5 * c6a9300 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
1 parent 30c52ac commit 5883e05

2 files changed

Lines changed: 0 additions & 35 deletions

File tree

‎packages/router/src/router.ts‎

Lines changed: 0 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1361,21 +1361,6 @@ export class Router {
13611361
return Promise.resolve(false);
13621362
}
13631363

1364-
// Duplicate navigations may be triggered by attempts to sync AngularJS and
1365-
// Angular router states. We have the setTimeout in the location listener to
1366-
// ensure the imperative nav is scheduled before the browser nav.
1367-
const lastNavigation = this.transitions.value;
1368-
const browserNavPrecededByRouterNav = isBrowserTriggeredNavigation(source) && lastNavigation &&
1369-
!isBrowserTriggeredNavigation(lastNavigation.source);
1370-
const navToSameUrl = lastNavigation.rawUrl.toString() === rawUrl.toString();
1371-
const lastNavigationInProgress = lastNavigation.id === this.currentNavigation?.id;
1372-
// We consider duplicates as ones that goes to the same URL while the first
1373-
// is still processing.
1374-
const isDuplicateNav = navToSameUrl && lastNavigationInProgress;
1375-
if (browserNavPrecededByRouterNav && isDuplicateNav) {
1376-
return Promise.resolve(true); // return value is not used
1377-
}
1378-
13791364
let resolve: any;
13801365
let reject: any;
13811366
let promise: Promise<boolean>;

‎packages/router/test/integration.spec.ts‎

Lines changed: 0 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1204,26 +1204,6 @@ describe('Integration', () => {
12041204
});
12051205
});
12061206

1207-
it('should ignore the duplicate resulting from a location sync', fakeAsync(() => {
1208-
const router = TestBed.inject(Router);
1209-
const fixture = createRoot(router, RootCmp);
1210-
const location = TestBed.inject(Location) as SpyLocation;
1211-
router.resetConfig([{path: 'simple', component: SimpleCmp, canActivate: ['in1Second']}]);
1212-
1213-
const recordedEvents: any[] = [];
1214-
router.events.forEach(e => onlyNavigationStartAndEnd(e) && recordedEvents.push(e));
1215-
1216-
// setTimeout used so this navigation resolves at the same time as the one that results
1217-
// from the location PopStateEvent (see Router#setUpLocationChangeListener).
1218-
setTimeout(() => {
1219-
router.navigateByUrl('/simple');
1220-
}, 0);
1221-
location.simulateUrlPop('/simple');
1222-
tick(1000);
1223-
advance(fixture);
1224-
expectEvents(recordedEvents, [[NavigationStart, '/simple'], [NavigationEnd, '/simple']]);
1225-
}));
1226-
12271207
it('should reset location if a navigation by location is successful', fakeAsync(() => {
12281208
const router = TestBed.inject(Router);
12291209
const location = TestBed.inject(Location) as SpyLocation;

0 commit comments

Comments
 (0)