From dbe1e626f877a9ef7bbffc2f360bae809874790d Mon Sep 17 00:00:00 2001 From: Joe Haddad Date: Thu, 31 Dec 2020 11:08:12 -0500 Subject: [PATCH] fix(experimental scroll): use `sessionStorage` instead of `history` (#20633) This pull request adjusts our experimental scroll restoration behavior to use `sessionStorage` as opposed to `History#replaceState` to track scroll position. In addition, **it eliminates a scroll event listener** and only captures when a `pushState` event happens (thereby leaving state that needs snapshotted). These merely adjusts implementation detail, and is covered by existing tests: ``` test/integration/scroll-back-restoration/ ``` --- Fixes #16690 Fixes #17073 Fixes #20486 --- packages/next/client/index.tsx | 4 +- .../next/next-server/lib/router/router.ts | 105 +++++++++++------- .../build-output/test/index.test.js | 6 +- 3 files changed, 71 insertions(+), 44 deletions(-) diff --git a/packages/next/client/index.tsx b/packages/next/client/index.tsx index 8386a64671..c155ce3e1d 100644 --- a/packages/next/client/index.tsx +++ b/packages/next/client/index.tsx @@ -45,7 +45,7 @@ declare global { type RenderRouteInfo = PrivateRouteInfo & { App: AppComponent - scroll?: boolean + scroll?: { x: number; y: number } | null } type RenderErrorProps = Omit @@ -753,7 +753,7 @@ function doRender(input: RenderRouteInfo): Promise { } if (input.scroll) { - window.scrollTo(0, 0) + window.scrollTo(input.scroll.x, input.scroll.y) } } diff --git a/packages/next/next-server/lib/router/router.ts b/packages/next/next-server/lib/router/router.ts index 9b141348f8..3431db68ec 100644 --- a/packages/next/next-server/lib/router/router.ts +++ b/packages/next/next-server/lib/router/router.ts @@ -49,7 +49,10 @@ interface NextHistoryState { options: TransitionOptions } -type HistoryState = null | { __N: false } | ({ __N: true } & NextHistoryState) +type HistoryState = + | null + | { __N: false } + | ({ __N: true; idx: number } & NextHistoryState) let detectDomainLocale: typeof import('../i18n/detect-domain-locale').detectDomainLocale @@ -355,7 +358,7 @@ export type AppComponent = ComponentType type Subscription = ( data: PrivateRouteInfo, App: AppComponent, - resetScroll: boolean + resetScroll: { x: number; y: number } | null ) => Promise type BeforePopStateCallback = (state: NextHistoryState) => boolean @@ -367,7 +370,14 @@ type HistoryMethod = 'replaceState' | 'pushState' const manualScrollRestoration = process.env.__NEXT_SCROLL_RESTORATION && typeof window !== 'undefined' && - 'scrollRestoration' in window.history + 'scrollRestoration' in window.history && + !!(function () { + try { + let v = '__next' + // eslint-disable-next-line no-sequences + return sessionStorage.setItem(v, v), sessionStorage.removeItem(v), true + } catch (n) {} + })() const SSG_DATA_NOT_FOUND = Symbol('SSG_DATA_NOT_FOUND') @@ -445,6 +455,8 @@ export default class Router implements BaseRouter { defaultLocale?: string domainLocales?: DomainLocales + private _idx: number = 0 + static events: MittEmitter = mitt() constructor( @@ -555,27 +567,6 @@ export default class Router implements BaseRouter { if (process.env.__NEXT_SCROLL_RESTORATION) { if (manualScrollRestoration) { window.history.scrollRestoration = 'manual' - - let scrollDebounceTimeout: undefined | NodeJS.Timeout - - const debouncedScrollSave = () => { - if (scrollDebounceTimeout) clearTimeout(scrollDebounceTimeout) - - scrollDebounceTimeout = setTimeout(() => { - const { url, as: curAs, options } = history.state - this.changeState( - 'replaceState', - url, - curAs, - Object.assign({}, options, { - _N_X: window.scrollX, - _N_Y: window.scrollY, - }) - ) - }, 10) - } - - window.addEventListener('scroll', debouncedScrollSave) } } } @@ -607,7 +598,30 @@ export default class Router implements BaseRouter { return } - const { url, as, options } = state + let forcedScroll: { x: number; y: number } | undefined + const { url, as, options, idx } = state + if (process.env.__NEXT_SCROLL_RESTORATION) { + if (manualScrollRestoration) { + if (this._idx !== idx) { + // Snapshot current scroll position: + try { + sessionStorage.setItem( + '__next_scroll_' + this._idx, + JSON.stringify({ x: self.pageXOffset, y: self.pageYOffset }) + ) + } catch {} + + // Restore old scroll position: + try { + const v = sessionStorage.getItem('__next_scroll_' + idx) + forcedScroll = JSON.parse(v!) + } catch { + forcedScroll = { x: 0, y: 0 } + } + } + } + } + this._idx = idx const { pathname } = parseRelativeUrl(url) @@ -627,10 +641,11 @@ export default class Router implements BaseRouter { 'replaceState', url, as, - Object.assign({}, options, { + Object.assign<{}, TransitionOptions, TransitionOptions>({}, options, { shallow: options.shallow && this._shallow, locale: options.locale || this.defaultLocale, - }) + }), + forcedScroll ) } @@ -652,6 +667,19 @@ export default class Router implements BaseRouter { * @param options object you can define `shallow` and other options */ push(url: Url, as?: Url, options: TransitionOptions = {}) { + if (process.env.__NEXT_SCROLL_RESTORATION) { + // TODO: remove in the future when we update history before route change + // is complete, as the popstate event should handle this capture. + if (manualScrollRestoration) { + try { + // Snapshot scroll position right before navigating to a new page: + sessionStorage.setItem( + '__next_scroll_' + this._idx, + JSON.stringify({ x: self.pageXOffset, y: self.pageYOffset }) + ) + } catch {} + } + } ;({ url, as } = prepareUrlAs(this, url, as)) return this.change('pushState', url, as, options) } @@ -667,11 +695,12 @@ export default class Router implements BaseRouter { return this.change('replaceState', url, as, options) } - async change( + private async change( method: HistoryMethod, url: string, as: string, - options: TransitionOptions + options: TransitionOptions, + forcedScroll?: { x: number; y: number } ): Promise { if (!isLocalURL(url)) { window.location.href = url @@ -804,7 +833,7 @@ export default class Router implements BaseRouter { // TODO: do we need the resolved href when only a hash change? this.changeState(method, url, as, options) this.scrollToHash(cleanedAs) - this.notify(this.components[this.route], false) + this.notify(this.components[this.route], null) Router.events.emit('hashChangeComplete', as, routeProps) return true } @@ -1024,7 +1053,7 @@ export default class Router implements BaseRouter { query, cleanedAs, routeInfo, - !!options.scroll + forcedScroll || (options.scroll ? { x: 0, y: 0 } : null) ).catch((e) => { if (e.cancelled) error = error || e else throw e @@ -1035,12 +1064,6 @@ export default class Router implements BaseRouter { throw error } - if (process.env.__NEXT_SCROLL_RESTORATION) { - if (manualScrollRestoration && '_N_X' in options) { - window.scrollTo((options as any)._N_X, (options as any)._N_Y) - } - } - if (process.env.__NEXT_I18N_SUPPORT) { if (this.locale) { document.documentElement.lang = this.locale @@ -1083,6 +1106,7 @@ export default class Router implements BaseRouter { as, options, __N: true, + idx: this._idx = method !== 'pushState' ? this._idx : this._idx + 1, } as HistoryState, // Most browsers currently ignores this parameter, although they may use it in the future. // Passing the empty string here should be safe against future changes to the method. @@ -1250,7 +1274,7 @@ export default class Router implements BaseRouter { query: ParsedUrlQuery, as: string, data: PrivateRouteInfo, - resetScroll: boolean + resetScroll: { x: number; y: number } | null ): Promise { this.isFallback = false @@ -1497,7 +1521,10 @@ export default class Router implements BaseRouter { } } - notify(data: PrivateRouteInfo, resetScroll: boolean): Promise { + notify( + data: PrivateRouteInfo, + resetScroll: { x: number; y: number } | null + ): Promise { return this.sub( data, this.components['/_app'].Component as AppComponent, diff --git a/test/integration/build-output/test/index.test.js b/test/integration/build-output/test/index.test.js index 2b7498188d..91871b8d03 100644 --- a/test/integration/build-output/test/index.test.js +++ b/test/integration/build-output/test/index.test.js @@ -94,8 +94,8 @@ describe('Build Output', () => { expect(parseFloat(indexSize) - 266).toBeLessThanOrEqual(0) expect(indexSize.endsWith('B')).toBe(true) - // should be no bigger than 62.1 kb - expect(parseFloat(indexFirstLoad) - 62.1).toBeLessThanOrEqual(0) + // should be no bigger than 62.2 kb + expect(parseFloat(indexFirstLoad)).toBeCloseTo(62.2, 1) expect(indexFirstLoad.endsWith('kB')).toBe(true) expect(parseFloat(err404Size) - 3.7).toBeLessThanOrEqual(0) @@ -104,7 +104,7 @@ describe('Build Output', () => { expect(parseFloat(err404FirstLoad)).toBeCloseTo(65.3, 1) expect(err404FirstLoad.endsWith('kB')).toBe(true) - expect(parseFloat(sharedByAll) - 61.8).toBeLessThanOrEqual(0) + expect(parseFloat(sharedByAll)).toBeCloseTo(61.9, 1) expect(sharedByAll.endsWith('kB')).toBe(true) if (_appSize.endsWith('kB')) { -- GitLab