Skip to content

Commit

Permalink
fix routing bug when bfcache is hit following an mpa navigation (#54081)
Browse files Browse the repository at this point in the history
When an mpa navigation takes place, we currently push the user to the new route and suspend the page indefinitely (x-ref: #49058). When navigating back, if the browser opts into using the [bfcache](https://web.dev/bfcache/), it will remain suspended and `pushRef.mpaNavigation` will be true. This means that anything that would cause the component to re-render will trigger the mpa navigation again (such as hovering over another `Link`, as reported in #53347)

This PR checks to see if bfcache is being used by observing `PageTransitionEvent.persisted` and if so, resets the router state to clear out `pushRef`. 

Closes NEXT-1511
Fixes #53347
  • Loading branch information
ztanner committed Aug 16, 2023
1 parent 86d2ead commit 0b3e366
Show file tree
Hide file tree
Showing 9 changed files with 131 additions and 7 deletions.
22 changes: 22 additions & 0 deletions packages/next/src/client/components/app-router.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -367,6 +367,28 @@ function Router({
}, [appRouter, cache, prefetchCache, tree])
}

useEffect(() => {
// If the app is restored from bfcache, it's possible that
// pushRef.mpaNavigation is true, which would mean that any re-render of this component
// would trigger the mpa navigation logic again from the lines below.
// This will restore the router to the initial state in the event that the app is restored from bfcache.
function handlePageShow(event: PageTransitionEvent) {
if (!event.persisted) return

dispatch({
type: ACTION_RESTORE,
url: new URL(window.location.href),
tree: window.history.state,
})
}

window.addEventListener('pageshow', handlePageShow)

return () => {
window.removeEventListener('pageshow', handlePageShow)
}
}, [dispatch, initialTree])

// When mpaNavigation flag is set do a hard navigation to the new url.
// Infinitely suspend because we don't actually want to rerender any child
// components with the new URL and any entangled state updates shouldn't
Expand Down
3 changes: 2 additions & 1 deletion test/lib/browsers/base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,8 @@ export abstract class BrowserInterface implements PromiseLike<any> {
async setup(
browserName: string,
locale: string,
javaScriptEnabled: boolean
javaScriptEnabled: boolean,
headless: boolean
): Promise<void> {}
async close(): Promise<void> {}
async quit(): Promise<void> {}
Expand Down
9 changes: 7 additions & 2 deletions test/lib/browsers/playwright.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,12 @@ export class Playwright extends BrowserInterface {
this.eventCallbacks[event]?.delete(cb)
}

async setup(browserName: string, locale: string, javaScriptEnabled: boolean) {
const headless = !!process.env.HEADLESS
async setup(
browserName: string,
locale: string,
javaScriptEnabled: boolean,
headless: boolean
) {
let device

if (process.env.DEVICE_NAME) {
Expand Down Expand Up @@ -106,6 +110,7 @@ export class Playwright extends BrowserInterface {
return await chromium.launch({
devtools: !launchOptions.headless,
...launchOptions,
ignoreDefaultArgs: ['--disable-back-forward-cache'],
})
}
}
Expand Down
10 changes: 7 additions & 3 deletions test/lib/browsers/selenium.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ const {
BROWSERSTACK,
BROWSERSTACK_USERNAME,
BROWSERSTACK_ACCESS_KEY,
HEADLESS,
CHROME_BIN,
LEGACY_SAFARI,
SKIP_LOCAL_SELENIUM_SERVER,
Expand Down Expand Up @@ -46,7 +45,12 @@ export class Selenium extends BrowserInterface {
private browserName: string

// TODO: support setting locale
async setup(browserName: string, locale: string, javaScriptEnabled: boolean) {
async setup(
browserName: string,
locale: string,
javaScriptEnabled: boolean,
headless: boolean
) {
if (browser) return
this.browserName = browserName

Expand Down Expand Up @@ -155,7 +159,7 @@ export class Selenium extends BrowserInterface {
let firefoxOptions = new FireFoxOptions()
let safariOptions = new SafariOptions()

if (HEADLESS) {
if (headless) {
const screenSize = { width: 1280, height: 720 }
chromeOptions = chromeOptions.headless().windowSize(screenSize) as any
firefoxOptions = firefoxOptions.headless().windowSize(screenSize)
Expand Down
10 changes: 9 additions & 1 deletion test/lib/next-webdriver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ export default async function webdriver(
beforePageLoad?: (page: any) => void
locale?: string
disableJavaScript?: boolean
headless?: boolean
}
): Promise<BrowserInterface> {
let CurrentInterface: new () => BrowserInterface
Expand All @@ -82,6 +83,7 @@ export default async function webdriver(
beforePageLoad,
locale,
disableJavaScript,
headless,
} = options

// we import only the needed interface
Expand All @@ -101,7 +103,13 @@ export default async function webdriver(

const browser = new CurrentInterface()
const browserName = process.env.BROWSER_NAME || 'chrome'
await browser.setup(browserName, locale, !disableJavaScript)
await browser.setup(
browserName,
locale,
!disableJavaScript,
// allow headless to be overwritten for a particular test
typeof headless !== 'undefined' ? headless : !!process.env.HEADLESS
)
;(global as any).browserName = browserName

const fullUrl = getFullUrl(
Expand Down
8 changes: 8 additions & 0 deletions test/production/export-routing/app/layout.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
export default function Layout({ children }) {
return (
<html>
<head></head>
<body>{children}</body>
</html>
)
}
16 changes: 16 additions & 0 deletions test/production/export-routing/app/page.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
'use client'
import React from 'react'
import Link from 'next/link'

export default function Page() {
const [counter, setCounter] = React.useState(0)
return (
<div>
<button onClick={() => setCounter((c) => c + 1)}>
Trigger Re-Render
</button>
<div id="counter">{counter}</div>
<Link href="https://example.vercel.sh">External Page</Link>
</div>
)
}
54 changes: 54 additions & 0 deletions test/production/export-routing/index.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import { Server } from 'http'
import {
findPort,
nextBuild,
startStaticServer,
stopApp,
} from 'next-test-utils'
import webdriver from 'next-webdriver'
import { join } from 'path'

describe('export-routing', () => {
let port: number
let app: Server

beforeAll(async () => {
const appDir = __dirname
const exportDir = join(appDir, 'out')

await nextBuild(appDir, undefined, { cwd: appDir })
port = await findPort()
app = await startStaticServer(exportDir, undefined, port)
})

afterAll(() => {
stopApp(app)
})

it('should not suspend indefinitely when page is restored from bfcache after an mpa navigation', async () => {
// bfcache is not currently supported by CDP, so we need to run this particular test in headed mode
// https://bugs.chromium.org/p/chromium/issues/detail?id=1317959
if (!process.env.CI && process.env.HEADLESS) {
console.warn('This test can only run in headed mode. Skipping...')
return
}

const browser = await webdriver(port, '/index.html', { headless: false })

await browser.elementByCss('a[href="https://example.vercel.sh"]').click()
await browser.waitForCondition(
'window.location.origin === "https://example.vercel.sh"'
)

// this will never resolve in the failure case, as the page will be suspended indefinitely
await browser.back()

expect(await browser.elementByCss('#counter').text()).toBe('0')

// click the button
await browser.elementByCss('button').click()

// counter should be 1
expect(await browser.elementByCss('#counter').text()).toBe('1')
})
})
6 changes: 6 additions & 0 deletions test/production/export-routing/next.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
/** @type {import('next').NextConfig} */
const nextConfig = {
output: 'export',
}

module.exports = nextConfig

0 comments on commit 0b3e366

Please sign in to comment.