From 286b2807de61dcd6e24ced5c142fbc6eda9dfbec Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Tue, 10 Aug 2021 10:16:01 -0700 Subject: [PATCH] fix(router): eagerly update internal state on browser-triggered navigations (#43102) The management of `browserUrlTree` currently has several problems with correctly tracking the actual state of the browser. This change makes the Router eagerly update the `browserUrlTree` when handling navigations triggered by browser events (i.e., not 'imperative'). This is because with those types of navigations, the browser URL bar is _already_ updated. If we do not update the internal tracking of the `browserUrlTree`, we will be out of sync with the real URL if the navigation is rejected. It would be best if we could remove `browserUrlTree` completely, but doing that would require a lot more investigation and is blocked by #27059 because the SpyLocation used in tests does not emulate real browser behavior. fixes #43101 PR Close #43102 --- .../router/bundle.golden_symbols.json | 3 +++ packages/router/src/router.ts | 21 ++++++++++++++++--- packages/router/test/integration.spec.ts | 6 +++++- .../test/regression_integration.spec.ts | 11 +++------- 4 files changed, 29 insertions(+), 12 deletions(-) diff --git a/packages/core/test/bundling/router/bundle.golden_symbols.json b/packages/core/test/bundling/router/bundle.golden_symbols.json index ce42fcc3451..dec8ec5a667 100644 --- a/packages/core/test/bundling/router/bundle.golden_symbols.json +++ b/packages/core/test/bundling/router/bundle.golden_symbols.json @@ -1526,6 +1526,9 @@ { "name": "isArrayLike" }, + { + "name": "isBrowserTriggeredNavigation" + }, { "name": "isCommandWithOutlets" }, diff --git a/packages/router/src/router.ts b/packages/router/src/router.ts index db34072a358..f1641ac8588 100644 --- a/packages/router/src/router.ts +++ b/packages/router/src/router.ts @@ -636,6 +636,12 @@ export class Router { (this.onSameUrlNavigation === 'reload' ? true : urlTransition) && this.urlHandlingStrategy.shouldProcessUrl(t.rawUrl); + // If the source of the navigation is from a browser event, the URL is + // already updated. We already need to sync the internal state. + if (isBrowserTriggeredNavigation(t.source)) { + this.browserUrlTree = t.rawUrl; + } + if (processCurrentUrl) { return of(t).pipe( // Fire NavigationStart event @@ -954,7 +960,12 @@ export class Router { this.urlHandlingStrategy.merge(e.url, this.rawUrlTree); const extras = { skipLocationChange: t.extras.skipLocationChange, - replaceUrl: this.urlUpdateStrategy === 'eager' + // The URL is already updated at this point if we have 'eager' URL + // updates or if the navigation was triggered by the browser (back + // button, URL bar, etc). We want to replace that item in history if + // the navigation is rejected. + replaceUrl: this.urlUpdateStrategy === 'eager' || + isBrowserTriggeredNavigation(t.source) }; this.scheduleNavigation( @@ -1385,8 +1396,8 @@ export class Router { const lastNavigation = this.getTransition(); // 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 = - source !== 'imperative' && lastNavigation?.source === 'imperative'; + 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). @@ -1549,3 +1560,7 @@ function validateCommands(commands: string[]): void { } } } + +function isBrowserTriggeredNavigation(source: 'imperative'|'popstate'|'hashchange') { + return source !== 'imperative'; +} diff --git a/packages/router/test/integration.spec.ts b/packages/router/test/integration.spec.ts index b3bc41553b9..3e9210b418a 100644 --- a/packages/router/test/integration.spec.ts +++ b/packages/router/test/integration.spec.ts @@ -1230,7 +1230,11 @@ describe('Integration', () => { location.simulateHashChange('/blocked'); advance(fixture); - expectEvents(recordedEvents, [[NavigationStart, '/blocked']]); + expectEvents(recordedEvents, [ + [NavigationStart, '/blocked'], + [NavigationStart, '/simple'], + [NavigationEnd, '/simple'], + ]); })); }); diff --git a/packages/router/test/regression_integration.spec.ts b/packages/router/test/regression_integration.spec.ts index 371431b9e95..9df54ddeae1 100644 --- a/packages/router/test/regression_integration.spec.ts +++ b/packages/router/test/regression_integration.spec.ts @@ -266,14 +266,9 @@ describe('Integration', () => { location.simulateHashChange('/one'); advance(fixture); - const BASE_ERROR_MESSAGE = - 'This asserts current behavior, which is incorrect. When #28561 is fixed, it should be: '; - - expect(location.path()).toEqual('/one', BASE_ERROR_MESSAGE + '/'); - const urlChanges = ['replace: /', 'hash: /one']; - expect(location.urlChanges) - .toEqual( - urlChanges, BASE_ERROR_MESSAGE + JSON.stringify(urlChanges.concat('replace: /'))); + expect(location.path()).toEqual('/'); + const urlChanges = ['replace: /', 'hash: /one', 'replace: /']; + expect(location.urlChanges).toEqual(urlChanges); })); }); });