fix: scroll restoration #35770

AkifumiSato · 2022-03-31T08:02:30Z

When experimental.scrollRestoration = true, the restoration position may not be appropriate on the screen when getServerSidePropos is executed.
This is due to the fact that the _idx inside the Router is sequentially numbered.

This modification restores the position of the browser back after reloading.

before.mov

after.mov

Also, the key of the history is published from Router. (see #34980)

Bug

Related issues linked using fixes #number
Integration tests added
Errors have helpful link attached, see contributing.md

Feature

Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
Related issues linked using fixes #number
Integration tests added
Documentation added
Telemetry added. In case of a feature if it's used or not.
Errors have helpful link attached, see contributing.md

Documentation / Examples

Make sure the linting passes by running yarn lint

ijjk

Hi, can we add a test case for this?

AkifumiSato · 2022-04-01T09:16:43Z

@ijjk Added testing.
Without this correction, the test will fail.

ijjk · 2022-04-01T16:00:00Z

packages/next/shared/lib/router/router.ts

            forcedScroll = JSON.parse(v!)
          } catch {
            forcedScroll = { x: 0, y: 0 }
          }
        }
      }
    }
-    this._idx = idx


Why was this changed from index based to a random key?

The index is reset to zero on reload, resulting in inconsistencies in the session storage information and restoring the scroll position to an unintended position when browesr back or forward.

AkifumiSato · 2022-04-13T12:46:32Z

@ijjk Are there any other code modifications needed?

ijjk · 2022-04-13T16:55:35Z

docs/api-reference/next/router.md

@@ -52,6 +52,7 @@ The following is the definition of the `router` object returned by both [`useRou
 - `domainLocales`: `Array<{domain, defaultLocale, locales}>` - Any configured domain locales.
 - `isReady`: `boolean` - Whether the router fields are updated client-side and ready for use. Should only be used inside of `useEffect` methods and not for conditionally rendering on the server. See related docs for use case with [automatically statically optimized pages](/docs/advanced-features/automatic-static-optimization.md)
 - `isPreview`: `boolean` - Whether the application is currently in [preview mode](/docs/advanced-features/preview-mode.md).
+- `key`: `string` - String uniquely representing the current history entry. Reloading will reset it.


I don't think we should expose this value publicly as it could cause users to rely on it in unintended ways.

It is suggested to publish the history key in #34980 .
I also need this feature because I want to associate state and history and store them in storage.
Similar functionality is available in remix, and will be provided in the navigation-api being considered by the WICG.
https://github.com/remix-run/history/blob/main/docs/api-reference.md#locationkey
https://github.com/WICG/navigation-api#the-current-entry

The mentioned use case in the discussion wouldn't be solved here since the key is random and wouldn't give a signal to how many navigations or where in the navigation history the user is.

What about in terms of providing functionality equivalent to remix and WICG's navigation-api? This feature would solve the case where we want to link state and history, etc.

ijjk · 2022-04-13T17:03:09Z

packages/next/server/render.tsx

@@ -149,6 +151,7 @@ class ServerRouter implements NextRouter {
    this.domainLocales = domainLocales
    this.isPreview = !!isPreview
    this.isLocaleDomain = !!isLocaleDomain
+    this.key = createKey()


We shouldn't need to have a unique key during SSR as the key is meant to be used on the client only right?

Right.
Should it be null in SSR?

I think leaving this unset during SSR would be fine and doesn't need to be set anyways if it's not exposed publicly.

ijjk · 2022-04-16T02:37:17Z

Stats from current PR

Default Build (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	AkifumiSato/next.js history-unique-key	Change
buildDuration	20s	19.2s	-817ms
buildDurationCached	7.4s	7.4s	-1ms
nodeModulesSize	475 MB	475 MB	⚠️ +505 B

Page Load Tests Overall decrease ⚠️

	vercel/next.js canary	AkifumiSato/next.js history-unique-key	Change
/ failed reqs	0	0	✓
/ total time (seconds)	4.858	4.925	⚠️ +0.07
/ avg req/sec	514.67	507.59	⚠️ -7.08
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.652	1.781	⚠️ +0.13
/error-in-render avg req/sec	1513.4	1403.36	⚠️ -110.04

Client Bundles (main, webpack) Overall increase ⚠️

	vercel/next.js canary	AkifumiSato/next.js history-unique-key	Change
925.HASH.js gzip	179 B	179 B	✓
framework-HASH.js gzip	42 kB	42 kB	✓
main-HASH.js gzip	28.8 kB	28.8 kB	⚠️ +33 B
webpack-HASH.js gzip	1.44 kB	1.44 kB	✓
Overall change	72.5 kB	72.5 kB	⚠️ +33 B

Legacy Client Bundles (polyfills)

	vercel/next.js canary	AkifumiSato/next.js history-unique-key	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	AkifumiSato/next.js history-unique-key	Change
_app-HASH.js gzip	1.36 kB	1.36 kB	✓
_error-HASH.js gzip	193 B	193 B	✓
amp-HASH.js gzip	308 B	308 B	✓
css-HASH.js gzip	327 B	327 B	✓
dynamic-HASH.js gzip	2.69 kB	2.69 kB	✓
head-HASH.js gzip	359 B	359 B	✓
hooks-HASH.js gzip	920 B	920 B	✓
image-HASH.js gzip	5.7 kB	5.7 kB	✓
index-HASH.js gzip	263 B	263 B	✓
link-HASH.js gzip	2.64 kB	2.64 kB	✓
routerDirect..HASH.js gzip	320 B	320 B	✓
script-HASH.js gzip	391 B	391 B	✓
withRouter-HASH.js gzip	318 B	318 B	✓
85e02e95b279..7e3.css gzip	107 B	107 B	✓
Overall change	15.9 kB	15.9 kB	✓

Client Build Manifests

	vercel/next.js canary	AkifumiSato/next.js history-unique-key	Change
_buildManifest.js gzip	459 B	459 B	✓
Overall change	459 B	459 B	✓

Rendered Page Sizes Overall decrease ✓

	vercel/next.js canary	AkifumiSato/next.js history-unique-key	Change
index.html gzip	532 B	532 B	✓
link.html gzip	546 B	546 B	✓
withRouter.html gzip	528 B	527 B	-1 B
Overall change	1.61 kB	1.6 kB	-1 B

Diffs

Diff for main-HASH.js

@@ -2681,6 +2681,7 @@
         "locales",
         "defaultLocale",
         "isReady",
+        "key",
         "isPreview",
         "isLocaleDomain",
         "domainLocales"
@@ -4539,6 +4540,7 @@
       exports.isLocalURL = isLocalURL;
       exports.interpolateAs = interpolateAs;
       exports.resolveHref = resolveHref;
+      exports.createKey = createKey;
       exports["default"] = void 0;
       var _normalizeTrailingSlash = __webpack_require__(2700);
       var _routeLoader = __webpack_require__(2497);
@@ -4935,6 +4937,11 @@
             throw err;
           }));
       }
+      function createKey() {
+        return Math.random()
+          .toString(36)
+          .slice(2, 10);
+      }
       var Router = /*#__PURE__*/ (function() {
         function Router(pathname1, query1, as1, param) {
           var initialProps = param.initialProps,
@@ -4959,7 +4966,6 @@
           this.sdr = {};
           // In-flight middleware preflight requests
           this.sde = {};
-          this._idx = 0;
           this.onPopState = function(e) {
             var state = e.state;
             if (!state) {
@@ -4991,11 +4997,11 @@
             var url = state.url,
               as = state.as,
               options = state.options,
-              idx = state.idx;
+              key = state.key;
             if (false) {
               var v;
             }
-            _this._idx = idx;
+            _this.key = key;
             var pathname2 = (0, _parseRelativeUrl).parseRelativeUrl(url)
               .pathname;
             // Make sure we don't re-render on initial load,
@@ -5070,6 +5076,7 @@
             (self.__NEXT_DATA__.appGip && !self.__NEXT_DATA__.gsp) ||
             (!autoExportDynamic && !self.location.search && !false)
           );
+          this.key = createKey();
           if (false) {
           }
           this.state = {
@@ -5921,8 +5928,8 @@
                     as: as,
                     options: options,
                     __N: true,
-                    idx: (this._idx =
-                      method !== "pushState" ? this._idx : this._idx + 1)
+                    key: (this.key =
+                      method !== "pushState" ? this.key : createKey())
                   }, // Passing the empty string here should be safe against future changes to the method.
                   // https://developer.mozilla.org/en-US/docs/Web/API/History/replaceState
                   "",

Diff for index.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-5e4f859c88025629.js"
+      src="/_next/static/chunks/main-ea469ed096a5aa14.js"
       defer=""
     ></script>
     <script

Diff for link.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-5e4f859c88025629.js"
+      src="/_next/static/chunks/main-ea469ed096a5aa14.js"
       defer=""
     ></script>
     <script

Diff for withRouter.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-5e4f859c88025629.js"
+      src="/_next/static/chunks/main-ea469ed096a5aa14.js"
       defer=""
     ></script>
     <script

Default Build with SWC (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	AkifumiSato/next.js history-unique-key	Change
buildDuration	20.8s	21.2s	⚠️ +446ms
buildDurationCached	7.4s	7.2s	-202ms
nodeModulesSize	475 MB	475 MB	⚠️ +505 B

Page Load Tests Overall increase ✓

	vercel/next.js canary	AkifumiSato/next.js history-unique-key	Change
/ failed reqs	0	0	✓
/ total time (seconds)	4.827	4.641	-0.19
/ avg req/sec	517.93	538.66	+20.73
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	1.656	1.595	-0.06
/error-in-render avg req/sec	1509.48	1567.23	+57.75

Client Bundles (main, webpack) Overall increase ⚠️

	vercel/next.js canary	AkifumiSato/next.js history-unique-key	Change
925.HASH.js gzip	178 B	178 B	✓
framework-HASH.js gzip	42.2 kB	42.2 kB	✓
main-HASH.js gzip	29.2 kB	29.3 kB	⚠️ +38 B
webpack-HASH.js gzip	1.45 kB	1.45 kB	✓
Overall change	73.1 kB	73.1 kB	⚠️ +38 B

Legacy Client Bundles (polyfills)

	vercel/next.js canary	AkifumiSato/next.js history-unique-key	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	AkifumiSato/next.js history-unique-key	Change
_app-HASH.js gzip	1.35 kB	1.35 kB	✓
_error-HASH.js gzip	179 B	179 B	✓
amp-HASH.js gzip	312 B	312 B	✓
css-HASH.js gzip	324 B	324 B	✓
dynamic-HASH.js gzip	2.71 kB	2.71 kB	✓
head-HASH.js gzip	357 B	357 B	✓
hooks-HASH.js gzip	921 B	921 B	✓
image-HASH.js gzip	5.79 kB	5.79 kB	✓
index-HASH.js gzip	261 B	261 B	✓
link-HASH.js gzip	2.76 kB	2.76 kB	✓
routerDirect..HASH.js gzip	322 B	322 B	✓
script-HASH.js gzip	392 B	392 B	✓
withRouter-HASH.js gzip	317 B	317 B	✓
85e02e95b279..7e3.css gzip	107 B	107 B	✓
Overall change	16.1 kB	16.1 kB	✓

Client Build Manifests

	vercel/next.js canary	AkifumiSato/next.js history-unique-key	Change
_buildManifest.js gzip	456 B	456 B	✓
Overall change	456 B	456 B	✓

Rendered Page Sizes Overall decrease ✓

	vercel/next.js canary	AkifumiSato/next.js history-unique-key	Change
index.html gzip	529 B	529 B	✓
link.html gzip	543 B	543 B	✓
withRouter.html gzip	526 B	525 B	-1 B
Overall change	1.6 kB	1.6 kB	-1 B

Diffs

Diff for main-HASH.js

@@ -2681,6 +2681,7 @@
         "locales",
         "defaultLocale",
         "isReady",
+        "key",
         "isPreview",
         "isLocaleDomain",
         "domainLocales"
@@ -4539,6 +4540,7 @@
       exports.isLocalURL = isLocalURL;
       exports.interpolateAs = interpolateAs;
       exports.resolveHref = resolveHref;
+      exports.createKey = createKey;
       exports["default"] = void 0;
       var _normalizeTrailingSlash = __webpack_require__(2700);
       var _routeLoader = __webpack_require__(2497);
@@ -4935,6 +4937,11 @@
             throw err;
           }));
       }
+      function createKey() {
+        return Math.random()
+          .toString(36)
+          .slice(2, 10);
+      }
       var Router = /*#__PURE__*/ (function() {
         function Router(pathname1, query1, as1, param) {
           var initialProps = param.initialProps,
@@ -4959,7 +4966,6 @@
           this.sdr = {};
           // In-flight middleware preflight requests
           this.sde = {};
-          this._idx = 0;
           this.onPopState = function(e) {
             var state = e.state;
             if (!state) {
@@ -4991,11 +4997,11 @@
             var url = state.url,
               as = state.as,
               options = state.options,
-              idx = state.idx;
+              key = state.key;
             if (false) {
               var v;
             }
-            _this._idx = idx;
+            _this.key = key;
             var pathname2 = (0, _parseRelativeUrl).parseRelativeUrl(url)
               .pathname;
             // Make sure we don't re-render on initial load,
@@ -5070,6 +5076,7 @@
             (self.__NEXT_DATA__.appGip && !self.__NEXT_DATA__.gsp) ||
             (!autoExportDynamic && !self.location.search && !false)
           );
+          this.key = createKey();
           if (false) {
           }
           this.state = {
@@ -5921,8 +5928,8 @@
                     as: as,
                     options: options,
                     __N: true,
-                    idx: (this._idx =
-                      method !== "pushState" ? this._idx : this._idx + 1)
+                    key: (this.key =
+                      method !== "pushState" ? this.key : createKey())
                   }, // Passing the empty string here should be safe against future changes to the method.
                   // https://developer.mozilla.org/en-US/docs/Web/API/History/replaceState
                   "",

Diff for index.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-5e4f859c88025629.js"
+      src="/_next/static/chunks/main-ea469ed096a5aa14.js"
       defer=""
     ></script>
     <script

Diff for link.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-5e4f859c88025629.js"
+      src="/_next/static/chunks/main-ea469ed096a5aa14.js"
       defer=""
     ></script>
     <script

Diff for withRouter.html

@@ -19,7 +19,7 @@
       defer=""
     ></script>
     <script
-      src="/_next/static/chunks/main-5e4f859c88025629.js"
+      src="/_next/static/chunks/main-ea469ed096a5aa14.js"
       defer=""
     ></script>
     <script

Commit: 19c76df

AkifumiSato · 2022-04-16T12:53:24Z

The added test case seemed to be flaky, so it has been fixed.

ijjk · 2022-04-20T02:30:20Z

Failing test suites

Commit: 19c76df

yarn testheadless test/integration/cli/test/index.test.js

CLI Usage > info > should print output

Expand output

● CLI Usage › info › should print output

expect(received).toBe(expected) // Object.is equality

Expected: ""
Received: "warn  - Latest canary version not detected, detected: \"12.1.7-canary.1\", newest: \"12.1.7-canary.3\".
        Please try the latest canary version (`npm install next@canary`) to confirm the issue still exists before creating a new issue.
        Read more - https://nextjs.org/docs/messages/opening-an-issue
"

  496 |       // warning will show so skip this check for the stable release
  497 |       if (pkg.version.includes('-canary')) {
> 498 |         expect(info.stderr || '').toBe('')
      |                                   ^
  499 |       }
  500 |       expect(info.stdout).toMatch(
  501 |         new RegExp(`

  at Object.<anonymous> (integration/cli/test/index.test.js:498:35)

Read more about building and testing Next.js in contributing.md.

AkifumiSato · 2022-04-20T12:04:13Z

@ijjk The error in the test was not reproduced in my environment. I thought that it was probably a case of the system failing if the restore was not completed in time, so I fixed it to wait until routeChangeComplete.
If this still falls through, please advise.

changed key from index to random string

…eComplete

AkifumiSato · 2022-05-12T14:39:07Z

I have created other PR that has been modified to not public the key and will close it.
#36861

AkifumiSato requested review from timneutkens, ijjk, shuding, styfle, huozhi, padmaia and leerob as code owners March 31, 2022 08:02

ijjk added area: documentation type: next labels Mar 31, 2022

AkifumiSato mentioned this pull request Mar 31, 2022

feat: proposal Change router._idx to public field #35681

Closed

10 tasks

ijjk reviewed Mar 31, 2022

View reviewed changes

AkifumiSato force-pushed the history-unique-key branch from 8542cb5 to d44923a Compare April 1, 2022 09:14

AkifumiSato force-pushed the history-unique-key branch from bd1793a to d44923a Compare April 1, 2022 09:34

ijjk reviewed Apr 1, 2022

View reviewed changes

AkifumiSato force-pushed the history-unique-key branch from d44923a to d0a70e3 Compare April 13, 2022 12:29

ijjk reviewed Apr 13, 2022

View reviewed changes

AkifumiSato force-pushed the history-unique-key branch from d0a70e3 to 4d152d5 Compare April 16, 2022 12:47

AkifumiSato requested a review from balazsorban44 as a code owner April 16, 2022 12:47

AkifumiSato force-pushed the history-unique-key branch from 993e2d2 to 061a791 Compare April 20, 2022 11:59

AkifumiSato mentioned this pull request May 3, 2022

router state idx is not persist after user manually refresh the page #36662

Open

1 task

akfm.sato and others added 3 commits May 4, 2022 09:05

Fix scroll restoration

b1c044e

changed key from index to random string

Change Router's key to public field

1f5a92e

scroll restoration bug fix test add

a1b34fa

AkifumiSato added 2 commits May 4, 2022 09:07

Fixed flaky test

5fad036

Fixed reload-scroll-back-restoration test to wait until `routeChang…

19c76df

…eComplete

AkifumiSato force-pushed the history-unique-key branch from 061a791 to 19c76df Compare May 4, 2022 00:07

mramadanahmed mentioned this pull request May 7, 2022

implement persist for router state _idx #36680

Closed

10 tasks

Merge branch 'vercel:canary' into history-unique-key

dddcde7

AkifumiSato mentioned this pull request May 12, 2022

fix: Scroll restoration bug caused by idx reset to 0 on reload #36861

Merged

10 tasks

AkifumiSato closed this May 12, 2022

github-actions bot locked as resolved and limited conversation to collaborators Jun 12, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: scroll restoration #35770

fix: scroll restoration #35770

AkifumiSato commented Mar 31, 2022 •

edited

Loading

ijjk left a comment

AkifumiSato commented Apr 1, 2022 •

edited

Loading

ijjk Apr 1, 2022

AkifumiSato Apr 1, 2022 •

edited

Loading

AkifumiSato Apr 3, 2022

AkifumiSato commented Apr 13, 2022

ijjk Apr 13, 2022

AkifumiSato Apr 13, 2022 •

edited

Loading

ijjk Apr 13, 2022

AkifumiSato Apr 14, 2022 •

edited

Loading

ijjk Apr 13, 2022

AkifumiSato Apr 13, 2022

ijjk Apr 13, 2022

ijjk commented Apr 16, 2022 •

edited

Loading

Diffs

Diffs

AkifumiSato commented Apr 16, 2022 •

edited

Loading

ijjk commented Apr 20, 2022 •

edited

Loading

AkifumiSato commented Apr 20, 2022

AkifumiSato commented May 12, 2022

fix: scroll restoration #35770

fix: scroll restoration #35770

Conversation

AkifumiSato commented Mar 31, 2022 • edited Loading

Bug

Feature

Documentation / Examples

ijjk left a comment

Choose a reason for hiding this comment

AkifumiSato commented Apr 1, 2022 • edited Loading

ijjk Apr 1, 2022

Choose a reason for hiding this comment

AkifumiSato Apr 1, 2022 • edited Loading

Choose a reason for hiding this comment

AkifumiSato Apr 3, 2022

Choose a reason for hiding this comment

AkifumiSato commented Apr 13, 2022

ijjk Apr 13, 2022

Choose a reason for hiding this comment

AkifumiSato Apr 13, 2022 • edited Loading

Choose a reason for hiding this comment

ijjk Apr 13, 2022

Choose a reason for hiding this comment

AkifumiSato Apr 14, 2022 • edited Loading

Choose a reason for hiding this comment

ijjk Apr 13, 2022

Choose a reason for hiding this comment

AkifumiSato Apr 13, 2022

Choose a reason for hiding this comment

ijjk Apr 13, 2022

Choose a reason for hiding this comment

ijjk commented Apr 16, 2022 • edited Loading

Stats from current PR

Diffs

Diffs

AkifumiSato commented Apr 16, 2022 • edited Loading

ijjk commented Apr 20, 2022 • edited Loading

Failing test suites

AkifumiSato commented Apr 20, 2022

AkifumiSato commented May 12, 2022

AkifumiSato commented Mar 31, 2022 •

edited

Loading

AkifumiSato commented Apr 1, 2022 •

edited

Loading

AkifumiSato Apr 1, 2022 •

edited

Loading

AkifumiSato Apr 13, 2022 •

edited

Loading

AkifumiSato Apr 14, 2022 •

edited

Loading

ijjk commented Apr 16, 2022 •

edited

Loading

AkifumiSato commented Apr 16, 2022 •

edited

Loading

ijjk commented Apr 20, 2022 •

edited

Loading