From ea1365d7d3de0adcc96c55569da20b65d638ae69 Mon Sep 17 00:00:00 2001 From: Nebojsa Cvetkovic Date: Fri, 11 Nov 2022 17:22:52 +0000 Subject: [PATCH 1/3] fix(router): restore 'history.state' on popstate even if navigationId missing If `history.pushState()` or `history.replaceState()` were called manually without including the `navigationId` field the state was being incorrectly discarded - that logic was for maintaining the original behavior of `NavigationStart.restoredState`. Improves on #28176, fully fixes #28108, see also #28954 --- packages/common/testing/src/location_mock.ts | 4 +++- packages/router/src/router.ts | 20 ++++++++++++++------ packages/router/test/integration.spec.ts | 13 ++++++++++++- 3 files changed, 29 insertions(+), 8 deletions(-) diff --git a/packages/common/testing/src/location_mock.ts b/packages/common/testing/src/location_mock.ts index d609392202f4e..caf7184690956 100644 --- a/packages/common/testing/src/location_mock.ts +++ b/packages/common/testing/src/location_mock.ts @@ -103,13 +103,15 @@ export class SpyLocation implements Location { path = this.prepareExternalUrl(path); const history = this._history[this._historyIndex]; + + history.state = state; + if (history.path == path && history.query == query) { return; } history.path = path; history.query = query; - history.state = state; const url = path + (query.length > 0 ? ('?' + query) : ''); this.urlChanges.push('replace: ' + url); diff --git a/packages/router/src/router.ts b/packages/router/src/router.ts index ca1b24cf7371b..355ed37d2b057 100644 --- a/packages/router/src/router.ts +++ b/packages/router/src/router.ts @@ -1018,19 +1018,27 @@ export class Router { // hybrid apps. setTimeout(() => { const extras: NavigationExtras = {replaceUrl: true}; - // Navigations coming from Angular router have a navigationId state - // property. When this exists, restore the state. - const state = event.state?.navigationId ? event.state : null; - if (state) { - const stateCopy = {...state} as Partial; + + // TODO: Simplify state management #28954 + + // NavigationStart.restoredState was originally set to either + // {{ navigationdId: number }} | null, so preserve this behavior + // even though we now include the rest of the state along with it (since #27198) + const restoredState = event.state?.navigationId ? event.state : null; + + // Separate to NavigationStart.restoredState, we must also restore the state to + // history.state and generate a new navigationId, since it will be overwritten + if (event.state) { + const stateCopy = {...event.state} as Partial; delete stateCopy.navigationId; delete stateCopy.ɵrouterPageId; if (Object.keys(stateCopy).length !== 0) { extras.state = stateCopy; } } + const urlTree = this.parseUrl(event['url']!); - this.scheduleNavigation(urlTree, source, state, extras); + this.scheduleNavigation(urlTree, source, restoredState, extras); }, 0); } }); diff --git a/packages/router/test/integration.spec.ts b/packages/router/test/integration.spec.ts index e7b57a3c33b42..95a1d7b602abc 100644 --- a/packages/router/test/integration.spec.ts +++ b/packages/router/test/integration.spec.ts @@ -163,7 +163,7 @@ describe('Integration', () => { } }); - const state = {foo: 'bar'}; + let state: any = {foo: 'bar'}; router.navigateByUrl('/simple', {state}); tick(); location.back(); @@ -173,6 +173,17 @@ describe('Integration', () => { expect(navigation.extras.state).toBeDefined(); expect(navigation.extras.state).toEqual(state); + + // Manually set state rather than using navigate() + state = {bar: 'foo'}; + location.replaceState(location.path(), '', state); + location.back(); + tick(); + location.forward(); + tick(); + + expect(navigation.extras.state).toBeDefined(); + expect(navigation.extras.state).toEqual(state); }))); it('should navigate correctly when using `Location#historyGo', From 26a47a03ea426bbac8621d788b3f23a784d872fd Mon Sep 17 00:00:00 2001 From: Nebojsa Cvetkovic Date: Sat, 19 Nov 2022 14:47:52 +0000 Subject: [PATCH 2/3] fixup! fix(router): restore 'history.state' on popstate even if navigationId missing --- packages/router/src/router.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/router/src/router.ts b/packages/router/src/router.ts index 355ed37d2b057..14122724842e8 100644 --- a/packages/router/src/router.ts +++ b/packages/router/src/router.ts @@ -1019,11 +1019,11 @@ export class Router { setTimeout(() => { const extras: NavigationExtras = {replaceUrl: true}; - // TODO: Simplify state management #28954 - // NavigationStart.restoredState was originally set to either - // {{ navigationdId: number }} | null, so preserve this behavior - // even though we now include the rest of the state along with it (since #27198) + // {{ navigationdId: number }} | null, so preserve this behavior for apps that + // rely on it (even though we include the entire state since #27198). + // The stored navigationId is used by the RouterScroller to retrieve the scroll + // position for the page. const restoredState = event.state?.navigationId ? event.state : null; // Separate to NavigationStart.restoredState, we must also restore the state to From f969ca6cbfe521a4c9c1d39741346f86f0ad84ee Mon Sep 17 00:00:00 2001 From: Nebojsa Cvetkovic Date: Sat, 19 Nov 2022 21:04:11 +0000 Subject: [PATCH 3/3] fixup! fix(router): restore 'history.state' on popstate even if navigationId missing --- packages/router/src/router.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/router/src/router.ts b/packages/router/src/router.ts index 14122724842e8..9dfbb3d6290c1 100644 --- a/packages/router/src/router.ts +++ b/packages/router/src/router.ts @@ -1019,9 +1019,12 @@ export class Router { setTimeout(() => { const extras: NavigationExtras = {replaceUrl: true}; - // NavigationStart.restoredState was originally set to either - // {{ navigationdId: number }} | null, so preserve this behavior for apps that - // rely on it (even though we include the entire state since #27198). + // TODO: restoredState should always include the entire state, regardless + // of navigationId. This requires a breaking change to update the type on + // NavigationStart’s restoredState, which currently requires navigationId + // to always be present. The Router used to only restore history state if + // a navigationId was present. + // The stored navigationId is used by the RouterScroller to retrieve the scroll // position for the page. const restoredState = event.state?.navigationId ? event.state : null;