Skip to content

Commit

Permalink
fix: env hot reload for RSC pages (#51930)
Browse files Browse the repository at this point in the history
### Issue
When you edit .env* files, the pages under app dir that using env vars are not triggering hot reload

### Fix
Triggering serverComponentChanges hot reload action when we detect env or tsconfig related change. There's a time period that we need to wait before the compilation is finished. First we save a flag `reloadOnDone` if we need to reload when after compilation is done, by determining if `envChange` is `true` (we already know this in dev server). Then in the compilation hooks, we refresh RSC page once it's finished.

### Extra change 

since we're checking `event.action` in client hot reloader, and throwing error for unknown action, filter devPagesManifestUpdate out from unexpected action as it sometimes triggered as error in console. Introduced in #51516
Fixes NEXT-1261
  • Loading branch information
huozhi committed Jul 3, 2023
1 parent 10605a1 commit c87a1b1
Show file tree
Hide file tree
Showing 10 changed files with 96 additions and 19 deletions.
Expand Up @@ -428,7 +428,7 @@ function processMessage(
router.fastRefresh()
dispatcher.onRefresh()
})
} else {
} else if (pageRes.status === 404) {
// We are still on the page,
// dispatch an error so it's caught by the NotFound handler
dispatcher.onNotFound()
Expand All @@ -437,6 +437,9 @@ function processMessage(
}
return
}
case 'devPagesManifestUpdate': {
return
}
default: {
throw new Error('Unexpected action ' + obj.action)
}
Expand Down
8 changes: 6 additions & 2 deletions packages/next/src/client/dev/amp-dev.ts
Expand Up @@ -85,8 +85,12 @@ addMessageListener((event) => {
try {
const message = JSON.parse(event.data)

// action `serverError` is not for amp-dev
if (message.action === 'serverError') return
// actions which are not related to amp-dev
if (
message.action === 'serverError' ||
message.action === 'devPagesManifestUpdate'
)
return
if (message.action === 'sync' || message.action === 'built') {
if (!message.hash) {
return
Expand Down
2 changes: 0 additions & 2 deletions packages/next/src/client/dev/error-overlay/hot-dev-client.ts
Expand Up @@ -276,8 +276,6 @@ function processMessage(obj: any) {
return handleSuccess()
}
case 'serverComponentChanges': {
// Server component changes don't apply to `pages`.
// TODO-APP: Remove reload once the correct overlay is rendered on initial page load in app dir
window.location.reload()
return
}
Expand Down
Expand Up @@ -45,7 +45,10 @@ export default () => {
}
return
}
if (obj.action === 'serverError') {
if (
obj.action === 'serverError' ||
obj.action === 'devPagesManifestUpdate'
) {
return
}
throw new Error('Unexpected action ' + obj.action)
Expand Down
32 changes: 25 additions & 7 deletions packages/next/src/server/dev/hot-reloader.ts
Expand Up @@ -201,6 +201,7 @@ export default class HotReloader {
staleness: 'unknown',
installed: '0.0.0',
}
private reloadAfterInvalidation: boolean = false
public multiCompiler?: webpack.MultiCompiler
public activeConfigs?: Array<
UnwrapPromise<ReturnType<typeof getBaseWebpackConfig>>
Expand Down Expand Up @@ -338,6 +339,14 @@ export default class HotReloader {
}
}

public async refreshServerComponents(): Promise<void> {
this.send({
action: 'serverComponentChanges',
// TODO: granular reloading of changes
// entrypoints: serverComponentChanges,
})
}

public onHMR(req: IncomingMessage, _socket: Duplex, head: Buffer) {
wsServer.handleUpgrade(req, req.socket, head, (client) => {
this.webpackHotMiddleware?.onHMR(client)
Expand Down Expand Up @@ -1201,6 +1210,9 @@ export default class HotReloader {
)

this.multiCompiler.hooks.done.tap('NextjsHotReloaderForServer', () => {
const reloadAfterInvalidation = this.reloadAfterInvalidation
this.reloadAfterInvalidation = false

const serverOnlyChanges = difference<string>(
changedServerPages,
changedClientPages
Expand Down Expand Up @@ -1233,12 +1245,12 @@ export default class HotReloader {
})
}

if (changedServerComponentPages.size || changedCSSImportPages.size) {
this.send({
action: 'serverComponentChanges',
// TODO: granular reloading of changes
// entrypoints: serverComponentChanges,
})
if (
changedServerComponentPages.size ||
changedCSSImportPages.size ||
reloadAfterInvalidation
) {
this.refreshServerComponents()
}

changedClientPages.clear()
Expand Down Expand Up @@ -1336,7 +1348,13 @@ export default class HotReloader {
]
}

public invalidate() {
public invalidate(
{ reloadAfterInvalidation }: { reloadAfterInvalidation: boolean } = {
reloadAfterInvalidation: false,
}
) {
// Cache the `reloadAfterInvalidation` flag, and use it to reload the page when compilation is done
this.reloadAfterInvalidation = reloadAfterInvalidation
const outputPath = this.multiCompiler?.outputPath
return outputPath && getInvalidator(outputPath)?.invalidate()
}
Expand Down
11 changes: 7 additions & 4 deletions packages/next/src/server/dev/next-dev-server.ts
Expand Up @@ -440,6 +440,7 @@ export default class DevServer extends Server {
const pagesPageFilePaths = new Map<string, string>()

let envChange = false
let clientRouterFilterChange = false
let tsconfigChange = false
let conflictingPageChange = 0

Expand Down Expand Up @@ -636,7 +637,7 @@ export default class DevServer extends Server {
JSON.stringify(previousClientRouterFilters) !==
JSON.stringify(clientRouterFilters)
) {
envChange = true
clientRouterFilterChange = true
previousClientRouterFilters = clientRouterFilters
}
}
Expand All @@ -651,7 +652,7 @@ export default class DevServer extends Server {
.catch(() => {})
}

if (envChange || tsconfigChange) {
if (clientRouterFilterChange || envChange || tsconfigChange) {
if (envChange) {
this.loadEnvConfig({
dev: true,
Expand Down Expand Up @@ -713,7 +714,7 @@ export default class DevServer extends Server {
})
}

if (envChange) {
if (envChange || clientRouterFilterChange) {
config.plugins?.forEach((plugin: any) => {
// we look for the DefinePlugin definitions so we can
// update them on the active compilers
Expand Down Expand Up @@ -743,7 +744,9 @@ export default class DevServer extends Server {
})
}
})
this.hotReloader?.invalidate()
this.hotReloader?.invalidate({
reloadAfterInvalidation: envChange,
})
}

if (nestedMiddleware.length > 0) {
Expand Down
1 change: 1 addition & 0 deletions test/development/app-hmr/.env.development.local
@@ -0,0 +1 @@
MY_DEVICE="mac"
5 changes: 5 additions & 0 deletions test/development/app-hmr/app/env/edge/page.jsx
@@ -0,0 +1,5 @@
export default function Page() {
return <p>{process.env.MY_DEVICE}</p>
}

export const runtime = 'edge'
3 changes: 3 additions & 0 deletions test/development/app-hmr/app/env/node/page.jsx
@@ -0,0 +1,3 @@
export default function Page() {
return <p>{process.env.MY_DEVICE}</p>
}
43 changes: 41 additions & 2 deletions test/development/app-hmr/hmr.test.ts
@@ -1,15 +1,16 @@
import { createNextDescribe } from 'e2e-utils'
import { check } from 'next-test-utils'

const envFile = '.env.development.local'

createNextDescribe(
`app-dir-hmr`,
{
files: __dirname,
},
({ next }) => {
({ next, isTurbopack }) => {
describe('filesystem changes', () => {
it('should not break when renaming a folder', async () => {
console.log(next.url)
const browser = await next.browser('/folder')
const text = await browser.elementByCss('h1').text()
expect(text).toBe('Hello')
Expand All @@ -33,6 +34,44 @@ createNextDescribe(
await next.renameFolder('app/folder-renamed', 'app/folder')
}
})

if (!isTurbopack) {
it('should update server components pages when env files is changed (nodejs)', async () => {
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"')

try {
await check(async () => {
expect(await browser.elementByCss('p').text()).toBe('ipad')
return 'success'
}, /success/)
} finally {
await next.patchFile(envFile, envContent)
}
})

it('should update server components pages when env files is changed (edge)', async () => {
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"')

try {
await check(async () => {
expect(await browser.elementByCss('p').text()).toBe('ipad')
return 'success'
}, /success/)
} finally {
await next.patchFile(envFile, envContent)
}
})
}

it('should have no unexpected action error for hmr', async () => {
expect(next.cliOutput).not.toContain('Unexpected action')
})
})
}
)

0 comments on commit c87a1b1

Please sign in to comment.