Skip to content

Commit

Permalink
Retry manifest file loading only in dev mode (#73900)
Browse files Browse the repository at this point in the history
In #73891 we added another manifest to be loaded in `loadComponents`
(initially unconditionally). This uncovered a flakiness in prod mode
when attempting to load an optional manifest. The non-existent manifest
is attempted to be loaded three times with 100ms delay between attempts,
before giving up. For some reason the increased loading time leads to
more test flakiness.

To mitigate this, we're limiting the retry behaviour to the dev mode,
which matches the original intention when this was introduced in #45244.
  • Loading branch information
unstubbable authored and ztanner committed Dec 24, 2024
1 parent df392a1 commit 8a95639
Show file tree
Hide file tree
Showing 5 changed files with 41 additions and 8 deletions.
3 changes: 3 additions & 0 deletions packages/next/src/build/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1614,6 +1614,7 @@ export async function isPageStatic({
distDir,
page: originalAppPath || page,
isAppPath: pageType === 'app',
isDev: false,
})
}
const Comp = componentsResult.Component as NextComponentType | undefined
Expand Down Expand Up @@ -1869,6 +1870,7 @@ export async function hasCustomGetInitialProps({
distDir,
page: page,
isAppPath: false,
isDev: false,
})
let mod = components.ComponentMod

Expand All @@ -1895,6 +1897,7 @@ export async function getDefinedNamedExports({
distDir,
page: page,
isAppPath: false,
isDev: false,
})

return Object.keys(components.ComponentMod).filter((key) => {
Expand Down
1 change: 1 addition & 0 deletions packages/next/src/export/worker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,7 @@ async function exportPageImpl(
distDir,
page,
isAppPath: isAppDir,
isDev: false,
})

// Handle App Routes.
Expand Down
1 change: 1 addition & 0 deletions packages/next/src/server/dev/static-paths-worker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ export async function loadStaticPaths({
// In `pages/`, the page is the same as the pathname.
page: page || pathname,
isAppPath,
isDev: true,
})

if (isAppPath) {
Expand Down
30 changes: 23 additions & 7 deletions packages/next/src/server/load-components.ts
Original file line number Diff line number Diff line change
Expand Up @@ -118,12 +118,13 @@ export async function evalManifestWithRetries<T extends object>(

async function loadClientReferenceManifest(
manifestPath: string,
entryName: string
entryName: string,
attempts?: number
) {
try {
const context = await evalManifestWithRetries<{
__RSC_MANIFEST: { [key: string]: ClientReferenceManifest }
}>(manifestPath)
}>(manifestPath, attempts)
return context.__RSC_MANIFEST[entryName]
} catch (err) {
return undefined
Expand All @@ -134,10 +135,12 @@ async function loadComponentsImpl<N = any>({
distDir,
page,
isAppPath,
isDev,
}: {
distDir: string
page: string
isAppPath: boolean
isDev: boolean
}): Promise<LoadComponentsReturnType<N>> {
let DocumentMod = {}
let AppMod = {}
Expand All @@ -151,6 +154,12 @@ async function loadComponentsImpl<N = any>({
// Make sure to avoid loading the manifest for metadata route handlers.
const hasClientManifest = isAppPath && !isMetadataRoute(page)

// In dev mode we retry loading a manifest file to handle a race condition
// that can occur while app and pages are compiling at the same time, and the
// build-manifest is still being written to disk while an app path is
// attempting to load.
const manifestLoadAttempts = isDev ? 3 : 1

// Load the manifest files first
const [
buildManifest,
Expand All @@ -159,15 +168,20 @@ async function loadComponentsImpl<N = any>({
clientReferenceManifest,
serverActionsManifest,
] = await Promise.all([
loadManifestWithRetries<BuildManifest>(join(distDir, BUILD_MANIFEST)),
loadManifestWithRetries<BuildManifest>(
join(distDir, BUILD_MANIFEST),
manifestLoadAttempts
),
loadManifestWithRetries<ReactLoadableManifest>(
join(distDir, REACT_LOADABLE_MANIFEST)
join(distDir, REACT_LOADABLE_MANIFEST),
manifestLoadAttempts
),
// This manifest will only exist in Pages dir && Production && Webpack.
isAppPath || process.env.TURBOPACK
? undefined
: loadManifestWithRetries<DynamicCssManifest>(
join(distDir, `${DYNAMIC_CSS_MANIFEST}.json`)
join(distDir, `${DYNAMIC_CSS_MANIFEST}.json`),
manifestLoadAttempts
).catch(() => undefined),
hasClientManifest
? loadClientReferenceManifest(
Expand All @@ -177,12 +191,14 @@ async function loadComponentsImpl<N = any>({
'app',
page.replace(/%5F/g, '_') + '_' + CLIENT_REFERENCE_MANIFEST + '.js'
),
page.replace(/%5F/g, '_')
page.replace(/%5F/g, '_'),
manifestLoadAttempts
)
: undefined,
isAppPath
? loadManifestWithRetries<ActionManifest>(
join(distDir, 'server', SERVER_REFERENCE_MANIFEST + '.json')
join(distDir, 'server', SERVER_REFERENCE_MANIFEST + '.json'),
manifestLoadAttempts
).catch(() => null)
: null,
])
Expand Down
14 changes: 13 additions & 1 deletion packages/next/src/server/next-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -177,11 +177,14 @@ export default class NextNodeServer extends BaseServer<

protected cleanupListeners = new AsyncCallbackSet()
protected internalWaitUntil: WaitUntil | undefined
private isDev: boolean

constructor(options: Options) {
// Initialize super class
super(options)

this.isDev = options.dev ?? false

/**
* This sets environment variable to be used at the time of SSR by head.tsx.
* Using this from process.env allows targeting SSR by calling
Expand Down Expand Up @@ -213,11 +216,13 @@ export default class NextNodeServer extends BaseServer<
distDir: this.distDir,
page: '/_document',
isAppPath: false,
isDev: this.isDev,
}).catch(() => {})
loadComponents({
distDir: this.distDir,
page: '/_app',
isAppPath: false,
isDev: this.isDev,
}).catch(() => {})
}

Expand Down Expand Up @@ -278,11 +283,17 @@ export default class NextNodeServer extends BaseServer<
distDir: this.distDir,
page,
isAppPath: false,
isDev: this.isDev,
}).catch(() => {})
}

for (const page of Object.keys(appPathsManifest || {})) {
await loadComponents({ distDir: this.distDir, page, isAppPath: true })
await loadComponents({
distDir: this.distDir,
page,
isAppPath: true,
isDev: this.isDev,
})
.then(async ({ ComponentMod }) => {
// we need to ensure fetch is patched before we require the page,
// otherwise if the fetch is patched by user code, we will be patching it
Expand Down Expand Up @@ -789,6 +800,7 @@ export default class NextNodeServer extends BaseServer<
distDir: this.distDir,
page: pagePath,
isAppPath,
isDev: this.isDev,
})

if (
Expand Down

0 comments on commit 8a95639

Please sign in to comment.