-
Notifications
You must be signed in to change notification settings - Fork 26.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: scroll restoration #35770
fix: scroll restoration #35770
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, can we add a test case for this?
8542cb5
to
d44923a
Compare
@ijjk Added testing. |
bd1793a
to
d44923a
Compare
forcedScroll = JSON.parse(v!) | ||
} catch { | ||
forcedScroll = { x: 0, y: 0 } | ||
} | ||
} | ||
} | ||
} | ||
this._idx = idx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this changed from index based to a random key?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Load
/person/1
(SSR)
+-----------+
| /person/1 |
| idx: 0 |
+-----------+
- Move to
/person/2
(pushState)
+-----------+-----------+
| /person/1 | /person/2 |
| idx: 0 | idx: 1 |
+-----------+-----------+
- Reload
/person/2
(SSR)
+-----------+-----------+
| /person/1 | /person/2 |
| idx: 0 | idx: 0 |
+-----------+-----------+
The idx
value and scroll position are accidentally shared between pages.
d44923a
to
d0a70e3
Compare
@ijjk Are there any other code modifications needed? |
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should expose this value publicly as it could cause users to rely on it in unintended ways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -149,6 +151,7 @@ class ServerRouter implements NextRouter { | |||
this.domainLocales = domainLocales | |||
this.isPreview = !!isPreview | |||
this.isLocaleDomain = !!isLocaleDomain | |||
this.key = createKey() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't need to have a unique key during SSR as the key is meant to be used on the client only right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right.
Should it be null
in SSR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think leaving this unset during SSR would be fine and doesn't need to be set anyways if it's not exposed publicly.
Stats from current PRDefault Build (Increase detected
|
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 |
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 | |
/ avg req/sec | 514.67 | 507.59 | |
/error-in-render failed reqs | 0 | 0 | ✓ |
/error-in-render total time (seconds) | 1.652 | 1.781 | |
/error-in-render avg req/sec | 1513.4 | 1403.36 |
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 | |
webpack-HASH.js gzip | 1.44 kB | 1.44 kB | ✓ |
Overall change | 72.5 kB | 72.5 kB |
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 | |
buildDurationCached | 7.4s | 7.2s | -202ms |
nodeModulesSize | 475 MB | 475 MB |
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 | |
webpack-HASH.js gzip | 1.45 kB | 1.45 kB | ✓ |
Overall change | 73.1 kB | 73.1 kB |
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
d0a70e3
to
4d152d5
Compare
The added test case seemed to be flaky, so it has been fixed. |
Failing test suitesCommit: 19c76df
Expand output● CLI Usage › info › should print output
Read more about building and testing Next.js in contributing.md. |
993e2d2
to
061a791
Compare
@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 |
changed key from index to random string
061a791
to
19c76df
Compare
I have created other PR that has been modified to not public the |
When
experimental.scrollRestoration = true
, the restoration position may not be appropriate on the screen whengetServerSidePropos
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
fixes #number
contributing.md
Feature
fixes #number
contributing.md
Documentation / Examples
yarn lint