From 3791ae0c950dc9771f70ad5e9006113c5961b976 Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Fri, 2 Jul 2021 12:38:04 -0700 Subject: [PATCH] refactor(router): Adjust behavior for computed navigation restoration (#42751) When another navigation is triggered during an in-process navigation and the `canceledNavigationResolution` is `'computed'`, we should not attempt to restore the browser history using `history.go`. Doing that would trigger a third navigation through the router which would conflict with the new navigation that we were trying to process. Instead, we treat this as a redirect and skip the history restoration attempt. This acts similarly to returning `UrlTree` from a guard. Fixes issue described in https://github.com/angular/angular/pull/38884#issuecomment-863767152 PR Close #42751 --- packages/router/src/router.ts | 87 +++++++++----- packages/router/test/integration.spec.ts | 137 ++++++++++++++++++----- 2 files changed, 164 insertions(+), 60 deletions(-) diff --git a/packages/router/src/router.ts b/packages/router/src/router.ts index a6b7d52d54..b62672a85f 100644 --- a/packages/router/src/router.ts +++ b/packages/router/src/router.ts @@ -33,7 +33,6 @@ import {Checks, getAllRouteGuards} from './utils/preactivation'; import {isUrlTree} from './utils/type_guards'; - /** * @description * @@ -384,7 +383,6 @@ export const subsetMatchOptions: IsActiveMatchOptions = { queryParams: 'subset' }; - /** * @description * @@ -419,8 +417,20 @@ export class Router { /** * The id of the currently active page in the router. * Updated to the transition's target id on a successful navigation. + * + * This is used to track what page the router last activated. When an attempted navigation fails, + * the router can then use this to compute how to restore the state back to the previously active + * page. */ private currentPageId: number = 0; + /** + * The ɵrouterPageId of whatever page is currently active in the browser history. This is + * important for computing the target page id for new navigations because we need to ensure each + * page id in the browser history is 1 more than the previous entry. + */ + private get browserPageId(): number|undefined { + return (this.location.getState() as RestoredState | null)?.ɵrouterPageId; + } private configLoader: RouterConfigLoader; private ngModule: NgModuleRef; private console: Console; @@ -761,7 +771,7 @@ export class Router { filter(t => { if (!t.guardsResult) { - this.cancelNavigationTransition(t, ''); + this.cancelNavigationTransitionRestoreHistory(t, ''); return false; } return true; @@ -786,7 +796,7 @@ export class Router { next: () => dataResolved = true, complete: () => { if (!dataResolved) { - this.cancelNavigationTransition( + this.cancelNavigationTransitionRestoreHistory( t, `At least one route resolver didn't emit any value.`); } @@ -869,17 +879,28 @@ export class Router { * event is fired when a navigation gets cancelled but not caught by other * means. */ if (!completed && !errored) { - // Must reset to current URL tree here to ensure history.state is set. On a - // fresh page load, if a new navigation comes in before a successful - // navigation completes, there will be nothing in - // history.state.navigationId. This can cause sync problems with AngularJS - // sync code which looks for a value here in order to determine whether or - // not to handle a given popstate event or to leave it to the Angular - // router. - this.cancelNavigationTransition( - t, - `Navigation ID ${t.id} is not equal to the current navigation id ${ - this.navigationId}`); + const cancelationReason = `Navigation ID ${ + t.id} is not equal to the current navigation id ${this.navigationId}`; + if (this.canceledNavigationResolution === 'replace') { + // Must reset to current URL tree here to ensure history.state is set. On + // a fresh page load, if a new navigation comes in before a successful + // navigation completes, there will be nothing in + // history.state.navigationId. This can cause sync problems with + // AngularJS sync code which looks for a value here in order to determine + // whether or not to handle a given popstate event or to leave it to the + // Angular router. + this.cancelNavigationTransitionRestoreHistory(t, cancelationReason); + } else { + // We cannot trigger a `location.historyGo` if the + // cancellation was due to a new navigation before the previous could + // complete. This is because `location.historyGo` triggers a `popstate` + // which would also trigger another navigation. Instead, treat this as a + // redirect and do not reset the state. + this.cancelNavigationTransition(t, cancelationReason); + // TODO(atscott): The same problem happens here with a fresh page load + // and a new navigation before that completes where we won't set a page + // id. + } } // currentNavigation should always be reset to null here. If navigation was // successful, lastSuccessfulTransition will have already been set. Therefore @@ -993,9 +1014,9 @@ export class Router { if (!this.locationSubscription) { this.locationSubscription = this.location.subscribe(event => { const currentChange = this.extractLocationChangeInfoFromEvent(event); + // The `setTimeout` was added in #12160 and is likely to support Angular/AngularJS + // hybrid apps. if (this.shouldScheduleNavigation(this.lastLocationChangeInfo, currentChange)) { - // The `setTimeout` was added in #12160 and is likely to support Angular/AngularJS - // hybrid apps. setTimeout(() => { const {source, state, urlTree} = currentChange; const extras: NavigationExtras = {replaceUrl: true}; @@ -1213,15 +1234,7 @@ export class Router { const urlTree = isUrlTree(url) ? url : this.parseUrl(url); const mergedTree = this.urlHandlingStrategy.merge(urlTree, this.rawUrlTree); - let restoredState: RestoredState|null = null; - if (this.canceledNavigationResolution === 'computed') { - const isInitialPage = this.currentPageId === 0; - if (isInitialPage || extras.skipLocationChange || extras.replaceUrl) { - restoredState = this.location.getState() as RestoredState | null; - } - } - - return this.scheduleNavigation(mergedTree, 'imperative', restoredState, extras); + return this.scheduleNavigation(mergedTree, 'imperative', null, extras); } /** @@ -1345,6 +1358,7 @@ export class Router { if (this.disposed) { return Promise.resolve(false); } + // * Imperative navigations (router.navigate) might trigger additional navigations to the same // URL via a popstate event and the locationChangeListener. We should skip these duplicate // navs. Duplicates may also be triggered by attempts to sync AngularJS and Angular router @@ -1388,12 +1402,23 @@ export class Router { const id = ++this.navigationId; let targetPageId: number; if (this.canceledNavigationResolution === 'computed') { + const isInitialPage = this.currentPageId === 0; + if (isInitialPage) { + restoredState = this.location.getState() as RestoredState | null; + } // If the `ɵrouterPageId` exist in the state then `targetpageId` should have the value of - // `ɵrouterPageId` + // `ɵrouterPageId`. This is the case for something like a page refresh where we assign the + // target id to the previously set value for that page. if (restoredState && restoredState.ɵrouterPageId) { targetPageId = restoredState.ɵrouterPageId; } else { - targetPageId = this.currentPageId + 1; + // 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; + } } } else { // This is unused when `canceledNavigationResolution` is not computed. @@ -1453,7 +1478,7 @@ export class Router { * - triggers the `NavigationCancel` event * - resolves the transition promise with `false` */ - private cancelNavigationTransition(t: NavigationTransition, reason: string) { + private cancelNavigationTransitionRestoreHistory(t: NavigationTransition, reason: string) { if (this.canceledNavigationResolution === 'computed') { // 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. @@ -1470,6 +1495,10 @@ export class Router { } else { this.resetUrlToCurrentUrlTree(); } + this.cancelNavigationTransition(t, reason); + } + + private cancelNavigationTransition(t: NavigationTransition, reason: string) { const navCancel = new NavigationCancel(t.id, this.serializeUrl(t.extractedUrl), reason); this.triggerEvent(navCancel); t.resolve(false); diff --git a/packages/router/test/integration.spec.ts b/packages/router/test/integration.spec.ts index 4f14d123e5..fb5211f2d5 100644 --- a/packages/router/test/integration.spec.ts +++ b/packages/router/test/integration.spec.ts @@ -18,6 +18,7 @@ import {delay, filter, first, map, mapTo, tap} from 'rxjs/operators'; import {RouterInitializer} from '../src/router_module'; import {forEach} from '../src/utils/collection'; +import {isUrlTree} from '../src/utils/type_guards'; import {RouterTestingModule, SpyNgModuleFactoryLoader} from '../testing'; describe('Integration', () => { @@ -2740,7 +2741,6 @@ describe('Integration', () => { const url = path + (query.length > 0 ? ('?' + query) : ''); this.urlChanges.push(url); - this._subject.emit({'url': url, 'pop': false}); } replaceState(path: string, query: string = '', state: any = null) { @@ -2825,7 +2825,16 @@ describe('Integration', () => { @Injectable({providedIn: 'root'}) class MyCanActivateGuard implements CanActivate { allow: boolean = true; - canActivate(): boolean { + redirectTo: string|null|UrlTree = null; + + constructor(private router: Router) {} + + canActivate(): boolean|UrlTree { + if (typeof this.redirectTo === 'string') { + this.router.navigateByUrl(this.redirectTo); + } else if (isUrlTree(this.redirectTo)) { + return this.redirectTo; + } return this.allow; } } @@ -2879,16 +2888,23 @@ describe('Integration', () => { canActivate: [MyCanActivateGuard], resolve: [MyResolve] }, + { + path: 'unguarded', + component: SimpleCmp, + }, {path: 'loaded', loadChildren: () => of(LoadedModule), canLoad: ['alwaysFalse']} ]); router.navigateByUrl('/first'); advance(fixture); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 1})); router.navigateByUrl('/second'); advance(fixture); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 2})); router.navigateByUrl('/third'); advance(fixture); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 3})); location.back(); advance(fixture); @@ -2898,30 +2914,30 @@ describe('Integration', () => { const location = TestBed.inject(Location); const router = TestBed.inject(Router); expect(location.path()).toEqual('/second'); - expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 3})); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 2})); TestBed.inject(MyCanActivateGuard).allow = false; location.back(); advance(fixture); expect(location.path()).toEqual('/second'); - expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 3})); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 2})); TestBed.inject(MyCanActivateGuard).allow = true; location.back(); advance(fixture); expect(location.path()).toEqual('/first'); - expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 2})); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 1})); TestBed.inject(MyCanActivateGuard).allow = false; location.forward(); advance(fixture); expect(location.path()).toEqual('/first'); - expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 2})); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 1})); router.navigateByUrl('/second'); advance(fixture); expect(location.path()).toEqual('/first'); - expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 2})); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 1})); })); @@ -2933,24 +2949,24 @@ describe('Integration', () => { location.back(); advance(fixture); expect(location.path()).toEqual('/second'); - expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 3})); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 2})); location.forward(); advance(fixture); expect(location.path()).toEqual('/second'); - expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 3})); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 2})); router.navigateByUrl('third'); advance(fixture); expect(location.path()).toEqual('/second'); - expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 3})); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 2})); TestBed.inject(MyCanDeactivateGuard).allow = true; location.forward(); advance(fixture); expect(location.path()).toEqual('/third'); - expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 4})); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 3})); })); it('should work when using `NavigationExtras.skipLocationChange`', fakeAsync(() => { @@ -2958,19 +2974,19 @@ describe('Integration', () => { const router = TestBed.inject(Router); expect(location.path()).toEqual('/second'); - expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 3})); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 2})); router.navigateByUrl('/first', {skipLocationChange: true}); advance(fixture); - expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 3})); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 2})); router.navigateByUrl('/third'); advance(fixture); - expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 4})); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 3})); location.back(); advance(fixture); - expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 3})); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 2})); })); it('should work when using `NavigationExtras.replaceUrl`', fakeAsync(() => { @@ -2978,11 +2994,11 @@ describe('Integration', () => { const router = TestBed.inject(Router); expect(location.path()).toEqual('/second'); - expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 3})); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 2})); router.navigateByUrl('/first', {replaceUrl: true}); advance(fixture); - expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 3})); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 2})); expect(location.path()).toEqual('/first'); })); @@ -2993,7 +3009,7 @@ describe('Integration', () => { router.navigateByUrl('/loaded'); advance(fixture); expect(location.path()).toEqual('/second'); - expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 3})); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 2})); })); it('should work when resolve empty', fakeAsync(() => { @@ -3001,20 +3017,20 @@ describe('Integration', () => { const router = TestBed.inject(Router); expect(location.path()).toEqual('/second'); - expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 3})); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 2})); TestBed.inject(MyResolve).myresolve = EMPTY; location.back(); advance(fixture); - expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 3})); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 2})); expect(location.path()).toEqual('/second'); TestBed.inject(MyResolve).myresolve = of(2); location.back(); advance(fixture); - expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 2})); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 1})); expect(location.path()).toEqual('/first'); TestBed.inject(MyResolve).myresolve = EMPTY; @@ -3022,24 +3038,24 @@ describe('Integration', () => { // We should cancel the navigation to `/third` when myresolve is empty router.navigateByUrl('/third'); advance(fixture); - expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 2})); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 1})); expect(location.path()).toEqual('/first'); location.historyGo(2); advance(fixture); - expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 2})); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 1})); expect(location.path()).toEqual('/first'); TestBed.inject(MyResolve).myresolve = of(2); location.historyGo(2); advance(fixture); - expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 4})); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 3})); expect(location.path()).toEqual('/third'); TestBed.inject(MyResolve).myresolve = EMPTY; location.historyGo(-2); advance(fixture); - expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 4})); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 3})); expect(location.path()).toEqual('/third'); })); @@ -3049,38 +3065,97 @@ describe('Integration', () => { const router = TestBed.inject(Router); expect(location.path()).toEqual('/second'); - expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 3})); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 2})); router.navigateByUrl('/invalid').catch(() => null); advance(fixture); expect(location.path()).toEqual('/second'); - expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 3})); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 2})); location.back(); advance(fixture); expect(location.path()).toEqual('/first'); - expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 2})); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 1})); })); - it('should work when urlUpdateStrategy="eagar"', fakeAsync(() => { + it('should work when urlUpdateStrategy="eager"', fakeAsync(() => { const location = TestBed.inject(Location) as SpyLocation; const router = TestBed.inject(Router); expect(location.path()).toEqual('/second'); - expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 3})); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 2})); router.urlUpdateStrategy = 'eager'; TestBed.inject(MyCanActivateGuard).allow = false; router.navigateByUrl('/first'); advance(fixture); expect(location.path()).toEqual('/second'); - expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 3})); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 2})); location.back(); advance(fixture); expect(location.path()).toEqual('/second'); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 2})); + })); + + it('should work when CanActivate redirects', fakeAsync(() => { + const location = TestBed.inject(Location); + + TestBed.inject(MyCanActivateGuard).redirectTo = '/unguarded'; + location.back(); + advance(fixture); + expect(location.path()).toEqual('/unguarded'); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 2})); + + TestBed.inject(MyCanActivateGuard).redirectTo = null; + + location.back(); + advance(fixture); + expect(location.path()).toEqual('/first'); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 1})); + })); + + it('should work when CanActivate redirects and urlUpdateStrategy="eager"', fakeAsync(() => { + const location = TestBed.inject(Location); + const router = TestBed.inject(Router); + router.urlUpdateStrategy = 'eager'; + + TestBed.inject(MyCanActivateGuard).redirectTo = '/unguarded'; + router.navigateByUrl('/third'); + advance(fixture); + expect(location.path()).toEqual('/unguarded'); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 4})); + + TestBed.inject(MyCanActivateGuard).redirectTo = null; + + location.back(); + advance(fixture); + expect(location.path()).toEqual('/third'); expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 3})); })); + + it('should work when CanActivate redirects with UrlTree and urlUpdateStrategy="eager"', + fakeAsync(() => { + // Note that this test is different from the above case because we are able to specifically + // handle the `UrlTree` case as a proper redirect and set `replaceUrl: true` on the + // follow-up navigation. + const location = TestBed.inject(Location); + const router = TestBed.inject(Router); + router.urlUpdateStrategy = 'eager'; + + TestBed.inject(MyCanActivateGuard).redirectTo = router.createUrlTree(['unguarded']); + router.navigateByUrl('/third'); + advance(fixture); + expect(location.path()).toEqual('/unguarded'); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 3})); + + TestBed.inject(MyCanActivateGuard).redirectTo = null; + + location.back(); + advance(fixture); + expect(location.path()).toEqual('/second'); + expect(location.getState()).toEqual(jasmine.objectContaining({ɵrouterPageId: 2})); + })); }); describe('guards', () => { describe('CanActivate', () => {