Skip to content
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 routing bug when bfcache is hit following an mpa navigation #54081

Merged
merged 4 commits into from
Aug 16, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
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'],
Copy link
Member Author

@ztanner ztanner Aug 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a very confusing line... but this is saying "don't disable bfcache by default", since Playwright automatically disables it (ref)

Our tests pass seem to pass with it enabled, but I'm not opposed to having this be an option instead Realizing this won’t really make a difference in CI since we’re running them headless, added an option to force non-headless in 6ee7015

})
}
}
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