Skip to content
Prev Previous commit
Next Next commit
Wait for pending Webpack Hot Updates before evaluating JS from RSC re…
…sponses

The Webpack runtime always tries to be minimal.
Since all pages share the same chunk, the prod runtime supports all pages.
However, in dev, the webpack runtime only supports the current page.

If we navigate to a new page, the Webpack runtime may need more functionality. Previously, we eagerly evaluated the RSC payload before any Hot Update was applied. This could lead to runtime errors.

For RSC payloads specifically, we just fell back to an MPA navigation.
We could continue to rely on this fallback. It may be disorienting though since we flash the error toast. We would also need to adjust this logic since `createFromFetch` no longer throws in these cases in later React version and instead lets the nearest Error Boundary handle these cases.
  • Loading branch information
eps1lon committed Jul 20, 2024
commit 36f7d76f2a819845c155c70894a7a4db120bbef6
Original file line number Diff line number Diff line change
Expand Up @@ -47,17 +47,35 @@ let __nextDevClientId = Math.round(Math.random() * 100 + Date.now())
let reloading = false
let startLatency: number | null = null

function onBeforeFastRefresh(dispatcher: Dispatcher, hasUpdates: boolean) {
let pendingHotUpdateWebpack = Promise.resolve()
let resolvePendingHotUpdateWebpack: () => void = () => {}
function setPendingHotUpdateWebpack() {
pendingHotUpdateWebpack = new Promise((resolve) => {
resolvePendingHotUpdateWebpack = () => {
resolve()
}
})
}

export function waitForWebpackRuntimeHotUpdate() {
return pendingHotUpdateWebpack
}

function handleBeforeHotUpdateWebpack(
dispatcher: Dispatcher,
hasUpdates: boolean
) {
if (hasUpdates) {
dispatcher.onBeforeRefresh()
}
}

function onFastRefresh(
function handleSuccessfulHotUpdateWebpack(
Copy link
Member Author

Choose a reason for hiding this comment

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

drive-by rename. We only call this in Webpack implying there's no Fast Refresh in Turbopack. Fast Refresh is also a React feature that's enabled by HMR so it is a bit confusing to invoke that name at all.

dispatcher: Dispatcher,
sendMessage: (message: string) => void,
updatedModules: ReadonlyArray<string>
) {
resolvePendingHotUpdateWebpack()
dispatcher.onBuildOk()
reportHmrLatency(sendMessage, updatedModules)

Expand Down Expand Up @@ -159,6 +177,7 @@ function tryApplyUpdates(
dispatcher: Dispatcher
) {
if (!isUpdateAvailable() || !canApplyUpdates()) {
resolvePendingHotUpdateWebpack()
dispatcher.onBuildOk()
reportHmrLatency(sendMessage, [])
return
Expand Down Expand Up @@ -281,12 +300,16 @@ function processMessage(
} else {
tryApplyUpdates(
function onBeforeHotUpdate(hasUpdates: boolean) {
onBeforeFastRefresh(dispatcher, hasUpdates)
handleBeforeHotUpdateWebpack(dispatcher, hasUpdates)
},
function onSuccessfulHotUpdate(webpackUpdatedModules: string[]) {
// Only dismiss it when we're sure it's a hot update.
// Otherwise it would flicker right before the reload.
onFastRefresh(dispatcher, sendMessage, webpackUpdatedModules)
handleSuccessfulHotUpdateWebpack(
dispatcher,
sendMessage,
webpackUpdatedModules
)
},
sendMessage,
dispatcher
Expand Down Expand Up @@ -320,6 +343,9 @@ function processMessage(
}
case HMR_ACTIONS_SENT_TO_BROWSER.BUILDING: {
startLatency = Date.now()
if (!process.env.TURBOPACK) {
setPendingHotUpdateWebpack()
}
Comment on lines 344 to +348
Copy link
Member Author

Choose a reason for hiding this comment

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

The downside here is that we always get a hot update on client-side navigations as far as I can tell. Even if the runtime doesn't get new functionality, it does get updates to getFullHash so we'd be waiting on something we don't necessarily need.

"use strict";
/*
 * ATTENTION: An "eval-source-map" devtool has been used.
 * This devtool is neither made for production nor for readable output files.
 * It uses "eval()" calls to create a separate source file with attached SourceMaps in the browser devtools.
 * If you are trying to read the output file, select a different devtool (https://webpack.js.org/configuration/devtool/)
 * or disable the default devtool with "devtool: false".
 * If you are looking for production-ready output files, see mode: "production" (https://webpack.js.org/configuration/mode/).
 */
self["webpackHotUpdate_N_E"]("webpack",{},
/******/ function(__webpack_require__) { // webpackRuntimeModules
/******/ /* webpack/runtime/getFullHash */
/******/ (() => {
/******/ 	__webpack_require__.h = () => ("d60fe58ebbed83dc")
/******/ })();
/******/ 
/******/ }
);

console.log('[Fast Refresh] rebuilding')
break
}
Expand Down Expand Up @@ -426,6 +452,7 @@ function processMessage(
reloading = true
return window.location.reload()
}
resolvePendingHotUpdateWebpack()
startTransition(() => {
router.hmrRefresh()
dispatcher.onRefresh()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import {
import { callServer } from '../../app-call-server'
import { PrefetchKind } from './router-reducer-types'
import { hexHash } from '../../../shared/lib/hash'
import { waitForWebpackRuntimeHotUpdate } from '../react-dev-overlay/app/hot-reloader-client'

export interface FetchServerResponseOptions {
readonly flightRouterState: FlightRouterState
Expand Down Expand Up @@ -180,6 +181,14 @@ export async function fetchServerResponse(
return doMpaNavigation(responseUrl.toString())
}

// We may navigate to a page that requires a different Webpack runtime.
// In prod, every page will have the same Webpack runtime.
// In dev, the Webpack runtime is minimal for each page.
// We need to ensure the Webpack runtime is updated before executing client-side JS of the new page.
if (process.env.NODE_ENV !== 'production') {
await waitForWebpackRuntimeHotUpdate()
}
Copy link
Member Author

@eps1lon eps1lon Jul 11, 2024

Choose a reason for hiding this comment

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

I guess we can safely ignore prefetches here @ztanner?

Copy link
Member

Choose a reason for hiding this comment

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

right, in dev this function won't see any prefetches

Copy link
Member Author

@eps1lon eps1lon Jul 12, 2024

Choose a reason for hiding this comment

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

It is however called with PrefetchKind.TEMPORARY. It's explained in PrefetchKind that TEMPORARY is also for navigations which makes the wording slightly confusing to me. I initially only checked for prefetchKind === undefined but that's incomplete. A navigation happens with prefetchKind === undefined || prefetchKind === PrefetchKind.TEMPORARY.

Copy link
Member

Choose a reason for hiding this comment

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

Which wording are you referring to?

Copy link
Member Author

@eps1lon eps1lon Jul 12, 2024

Choose a reason for hiding this comment

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

"prefetchKind" being defined even though we're not prefetching. In my head I'm mixing this in with prefetches in the browser e.g. <Link prefetch /> but it seems these are different concepts?

maybe this is just initially consuing because I haven't looked into 'temporary' vs undefined more. Just wanted to highlight it because Hendrik had the same initial impression.

Copy link
Member

@ztanner ztanner Jul 12, 2024

Choose a reason for hiding this comment

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

Yeah I agree it's a bit confusing. Part of what's confusing is that prefetches are tightly coupled to navigations. A navigation cannot exist without a prefetch (it's what the reducer acts upon when changing the router state), so that's why we have the concept of a "temporary" prefetch.

I don't think you're mixing the concepts though, <Link prefetch /> does indeed mostly map to what prefetchKind is, with the exception of the temporary one as you've noted. The comments in this file could use updating.

  • PrefetchKind.FULL = prefetch={true}
  • PrefetchKind.AUTO = prefetch left unspecified, or prefetch={null}
  • PrefetchKind.TEMPORARY = A placeholder for a navigation that occurred without a regular prefetch. For example if you have prefetch={false}, or you're in dev, a temporary prefetch is created. If we later discover a prefetch intent, TEMPORARY will get replaced with whatever that intent is.

Copy link
Member

Choose a reason for hiding this comment

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

I haven't looked into 'temporary' vs undefined more.

The undefined case is any other time we need to retrieve RSC data from the server when not directly associated with the navigation event. These come to mind but might not be exhaustive:

  • "Lazy fetching" missing segments in layout-router (this is pre-PPR behavior)
  • The dynamic data request in PPR (analogous to the lazy fetch above, but doesn't happen during render)
  • Refreshing segment data as a result of something like router.refresh()


// Handle the `fetch` readable stream that can be unwrapped by `React.use`.
const response: NavigationFlightResponse = await createFromFetch(
Promise.resolve(res),
Expand Down
13 changes: 4 additions & 9 deletions test/development/app-hmr/hmr.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,7 @@ describe(`app-dir-hmr`, () => {
.waitForElementByCss('[data-testid="new-runtime-functionality-page"]')

const logs = await browser.log()
// TODO: Should assert on all logs but these are cluttered with logs from our test utils (e.g. playwright tracing or webdriver)
if (process.env.TURBOPACK) {
// FIXME: logging "rebuilding" multiple times instead of closing it of with "done in"
// Should just not branch here and have the same logs as Webpack.
Expand Down Expand Up @@ -289,7 +290,6 @@ describe(`app-dir-hmr`, () => {
])
)
} else {
// TODO: Should assert on all logs but these are cluttered with logs from our test utils (e.g. playwright tracing or webdriver)
expect(logs).toEqual(
expect.arrayContaining([
{
Expand All @@ -302,21 +302,16 @@ describe(`app-dir-hmr`, () => {
},
])
)
expect(logs).toEqual(
expect(logs).not.toEqual(
expect.arrayContaining([
expect.objectContaining({
source: 'error',
}),
])
)
}
if (process.env.TURBOPACK) {
// No MPA navigation triggered
expect(await browser.eval('window.__TEST_NO_RELOAD')).toEqual(true)
} else {
// MPA navigation triggered
expect(await browser.eval('window.__TEST_NO_RELOAD')).toEqual(undefined)
}
// No MPA navigation triggered
expect(await browser.eval('window.__TEST_NO_RELOAD')).toEqual(true)
})
})
})