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.

Sign up for GitHub

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

Jump to bottom

Ensure "[Fast Refresh] rebuilding" logs have a matching "[Fast Refresh] done" log in Webpack #67968

Merged
merged 4 commits into from
Jul 20, 2024
Merged

Ensure "[Fast Refresh] rebuilding" logs have a matching "[Fast Refresh] done" log in Webpack #67968

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6e7e6e1
Ensure "[Fast Refresh] rebuilding" logs have a matching "[Fast Refres…
eps1lon Jul 19, 2024
2906145
Remove dev server restart workaround since flakiness is presumably fixed
eps1lon Jul 19, 2024
5192a94
Debug why some tests start with "mac"
eps1lon Jul 19, 2024
7c68fa5
Ensure we only start next test until current one is cleaned up
eps1lon Jul 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions packages/next/src/client/components/react-dev-overlay/app/hot-reloader-client.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@ function onFastRefresh(
updatedModules: ReadonlyArray<string>
) {
dispatcher.onBuildOk()

reportHmrLatency(sendMessage, updatedModules)

dispatcher.onRefresh()
Expand Down Expand Up @@ -161,6 +160,7 @@ function tryApplyUpdates(
) {
if (!isUpdateAvailable() || !canApplyUpdates()) {
dispatcher.onBuildOk()
reportHmrLatency(sendMessage, [])
return
}

Expand Down Expand Up @@ -430,7 +430,6 @@ function processMessage(
router.hmrRefresh()
dispatcher.onRefresh()
})
reportHmrLatency(sendMessage, [])

if (process.env.__NEXT_TEST_MODE) {
if (self.__NEXT_HMR_CB) {
Expand Down
161 changes: 96 additions & 65 deletions test/development/app-hmr/hmr.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ import { retry, waitFor } from 'next-test-utils'

const envFile = '.env.development.local'

const isPPREnabledByDefault = process.env.__NEXT_EXPERIMENTAL_PPR === 'true'

describe(`app-dir-hmr`, () => {
const { next } = nextTestSetup({
files: __dirname,
Expand Down Expand Up @@ -57,12 +55,11 @@ describe(`app-dir-hmr`, () => {
})

it('should update server components after navigating to a page with a different runtime', async () => {
const envContent = await next.readFile(envFile)

const browser = await next.browser('/env/node')
await browser.loadPage(`${next.url}/env/edge`)
await browser.eval('window.__TEST_NO_RELOAD = true')

expect(await browser.elementByCss('p').text()).toBe('mac')
await next.patchFile(envFile, 'MY_DEVICE="ipad"')

try {
Expand All @@ -73,70 +70,77 @@ describe(`app-dir-hmr`, () => {
const fastRefreshLogs = logs.filter((log) => {
return log.message.startsWith('[Fast Refresh]')
})
// FIXME: 3+ "rebuilding" but single "done" is confusing.
// FIXME: 3+ "rebuilding" but no "done" is confusing.
// There may actually be more "rebuilding" but not reliably.
// To ignore this flakiness, we just assert on subset matches.
// Once the bug is fixed, each "rebuilding" should be paired with a "done in" exactly.
expect(fastRefreshLogs).toEqual(
expect.arrayContaining([
{ source: 'log', message: '[Fast Refresh] rebuilding' },
{ source: 'log', message: '[Fast Refresh] rebuilding' },
{
source: 'log',
message: expect.stringContaining('[Fast Refresh] done in '),
},
{ source: 'log', message: '[Fast Refresh] rebuilding' },
])
)
// FIXME: Turbopack should have matching "done in" for each "rebuilding"
expect(logs).not.toEqual(
expect.arrayContaining([
expect.objectContaining({
message: expect.stringContaining('[Fast Refresh] done in'),
source: 'log',
}),
])
)
})
} else {
await retry(
async () => {
const fastRefreshLogs = logs.filter((log) => {
return log.message.startsWith('[Fast Refresh]')
const envValue = await browser.elementByCss('p').text()
const mpa = await browser.eval(
'window.__TEST_NO_RELOAD === undefined'
)
// Used to be flaky but presumably no longer is.
// If this flakes again, please add the received value as a commnet.
expect({ envValue, mpa }).toEqual({
envValue: 'ipad',
mpa: false,
})
// FIXME: Should be either a single "rebuilding"+"done" or the last "rebuilding" should be followed by "done"
expect(fastRefreshLogs).toEqual([
{ source: 'log', message: '[Fast Refresh] rebuilding' },
{ source: 'log', message: '[Fast Refresh] rebuilding' },
{
source: 'log',
message: expect.stringContaining('[Fast Refresh] done in '),
},
{ source: 'log', message: '[Fast Refresh] rebuilding' },
])
},
// Very slow Hot Update for some reason.
// May be related to receiving 3 rebuild events but only one finish event
5000
)

const fastRefreshLogs = logs.filter((log) => {
return log.message.startsWith('[Fast Refresh]')
})
expect(fastRefreshLogs).toEqual([
{ source: 'log', message: '[Fast Refresh] rebuilding' },
{
source: 'log',
message: expect.stringContaining('[Fast Refresh] done in '),
},
{ source: 'log', message: '[Fast Refresh] rebuilding' },
{ source: 'log', message: '[Fast Refresh] rebuilding' },
{
source: 'log',
message: expect.stringContaining('[Fast Refresh] done in '),
},
{
source: 'log',
message: expect.stringContaining('[Fast Refresh] done in '),
},
])
}
const envValue = await browser.elementByCss('p').text()
const mpa = await browser.eval('window.__TEST_NO_RELOAD === undefined')
// Flaky sometimes in Webpack:
// A. misses update and just receives `{ envValue: 'mac', mpa: false }`
// B. triggers error on server resulting in MPA: `{ envValue: 'ipad', mpa: true }` and server logs: ⨯ [TypeError: Cannot read properties of undefined (reading 'polyfillFiles')] ⨯ [TypeError: Cannot read properties of null (reading 'default')]
// A is more common than B.
expect({ envValue, mpa }).toEqual({
envValue:
isPPREnabledByDefault && !process.env.TURBOPACK
? // FIXME: Should be 'ipad' but PPR+Webpack swallows the update reliably
'mac'
: 'ipad',
mpa: false,
})
} finally {
await next.patchFile(envFile, envContent)
// TOOD: use sandbox instead
await next.patchFile(envFile, 'MY_DEVICE="mac"')
await retry(async () => {
expect(await browser.elementByCss('p').text()).toBe('mac')
})
}
})

it('should update server components pages when env files is changed (nodejs)', async () => {
// If "should update server components after navigating to a page with a different runtime" failed, the dev server is in a corrupted state.
// Restart fixes this.
await next.stop()
await next.start()

const envContent = await next.readFile(envFile)
const browser = await next.browser('/env/node')
expect(await browser.elementByCss('p').text()).toBe('mac')
await next.patchFile(envFile, 'MY_DEVICE="ipad"')
Expand All @@ -158,25 +162,36 @@ describe(`app-dir-hmr`, () => {
expect(await browser.elementByCss('p').text()).toBe('ipad')
})

expect(logs).toEqual(
expect.arrayContaining([
expect.objectContaining({
message: expect.stringContaining('[Fast Refresh] done in'),
source: 'log',
}),
])
)
if (process.env.TURBOPACK) {
// FIXME: Turbopack should have matching "done in" for each "rebuilding"
expect(logs).not.toEqual(
expect.arrayContaining([
expect.objectContaining({
message: expect.stringContaining('[Fast Refresh] done in'),
source: 'log',
}),
])
)
} else {
expect(logs).toEqual(
expect.arrayContaining([
expect.objectContaining({
message: expect.stringContaining('[Fast Refresh] done in'),
source: 'log',
}),
])
)
}
} finally {
await next.patchFile(envFile, envContent)
// TOOD: use sandbox instead
await next.patchFile(envFile, 'MY_DEVICE="mac"')
await retry(async () => {
expect(await browser.elementByCss('p').text()).toBe('mac')
})
}
})

it('should update server components pages when env files is changed (edge)', async () => {
// Restart to work around a bug highlighted in the flakiness of "should update server components after navigating to a page with a different runtime"
await next.stop()
await next.start()

const envContent = await next.readFile(envFile)
const browser = await next.browser('/env/edge')
expect(await browser.elementByCss('p').text()).toBe('mac')
await next.patchFile(envFile, 'MY_DEVICE="ipad"')
Expand All @@ -198,16 +213,32 @@ describe(`app-dir-hmr`, () => {
expect(await browser.elementByCss('p').text()).toBe('ipad')
})

expect(logs).toEqual(
expect.arrayContaining([
expect.objectContaining({
message: expect.stringContaining('[Fast Refresh] done in'),
source: 'log',
}),
])
)
if (process.env.TURBOPACK) {
// FIXME: Turbopack should have matching "done in" for each "rebuilding"
expect(logs).not.toEqual(
expect.arrayContaining([
expect.objectContaining({
message: expect.stringContaining('[Fast Refresh] done in'),
source: 'log',
}),
])
)
} else {
expect(logs).toEqual(
expect.arrayContaining([
expect.objectContaining({
message: expect.stringContaining('[Fast Refresh] done in'),
source: 'log',
}),
])
)
}
} finally {
await next.patchFile(envFile, envContent)
// TOOD: use sandbox instead
await next.patchFile(envFile, 'MY_DEVICE="mac"')
await retry(async () => {
expect(await browser.elementByCss('p').text()).toBe('mac')
})
}
})

Expand Down
Loading