fix(router): restore 'history.state' on popstate even if navigationId missing (#48033)

nebkat · dylhunn · commit fa5528fb5f0f · 2022-11-21T13:46:17.000-08:00
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 PR Close #48033
diff --git 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
@@ -1017,19 +1017,30 @@ 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<RestoredState>;
+
+            // 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;
+
+            // 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<RestoredState>;
               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
@@ -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',
