Skip to content

Commit

Permalink
Fix build worker callback arg missing correct page path (#61347)
Browse files Browse the repository at this point in the history
### What

The `arg` in the worker callback is alwasy `any`, when we access the
page path the argument could be in different form as the arg types are
different.

This PR align the argument type to object for `isPageStatic`,
`getDefinedNamedExports`, `hasCustomGetInitialProps` methods in static
worker. So they can share the similar shape of type of argument when
accessing `page` path. This will avoid the case that logged `page` in
the warning is `undefined`

Import the helpers from utils instead of workers as the worker itself
don't need to contain other exports that is not used for static worker.

### Why

This PR align the callback type of callback argument type of static
worker, so that we could get the actual page path value in a type-safe
way. We have 4 methods for static worker, `exportPage`, `isPageStatic`,
`getDefinedNamedExports`, `hasCustomGetInitialProps`, which the rest of
3 methods share the same format of warnings but their argument are
different. It's easily ended up with wrong argument type, and log with a
bad page path in the warning.


Closes NEXT-2289
  • Loading branch information
huozhi authored Jan 31, 2024
1 parent 92125f0 commit be37d3e
Show file tree
Hide file tree
Showing 6 changed files with 57 additions and 42 deletions.
2 changes: 1 addition & 1 deletion packages/next/src/build/handle-externals.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
NODE_ESM_RESOLVE_OPTIONS,
NODE_RESOLVE_OPTIONS,
} from './webpack-config'
import { isWebpackAppLayer, isWebpackServerLayer } from './worker'
import { isWebpackAppLayer, isWebpackServerLayer } from './utils'
import type { NextConfigComplete } from '../server/config-shared'
import { normalizePathSep } from '../shared/lib/page-path/normalize-path-sep'
const reactPackagesRegex = /^(react|react-dom|react-server-dom-webpack)($|\/)/
Expand Down
60 changes: 31 additions & 29 deletions packages/next/src/build/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -564,19 +564,32 @@ function getNumberOfWorkers(config: NextConfigComplete) {
}

const staticWorkerPath = require.resolve('./worker')
const staticWorkerExposedMethods = [
'hasCustomGetInitialProps',
'isPageStatic',
'getDefinedNamedExports',
'exportPage',
] as const
type StaticWorker = typeof import('./worker') & Worker
type PageDataCollectionKeys = Exclude<
(typeof staticWorkerExposedMethods)[number],
'exportPage'
>

function createStaticWorker(
config: NextConfigComplete,
incrementalCacheIpcPort?: number,
incrementalCacheIpcValidationKey?: string
) {
): StaticWorker {
let infoPrinted = false
const timeout = config.staticPageGenerationTimeout || 0

return new Worker(staticWorkerPath, {
timeout: timeout * 1000,
logger: Log,
onRestart: (method, [arg], attempts) => {
onRestart: (method, args, attempts) => {
if (method === 'exportPage') {
const [arg] = args as Parameters<StaticWorker['exportPage']>
const pagePath = arg.path
if (attempts >= 3) {
throw new Error(
Expand All @@ -587,7 +600,8 @@ function createStaticWorker(
`Restarted static page generation for ${pagePath} because it took more than ${timeout} seconds`
)
} else {
const pagePath = arg.path
const [arg] = args as Parameters<StaticWorker[PageDataCollectionKeys]>
const pagePath = arg.page
if (attempts >= 2) {
throw new Error(
`Collecting page data for ${pagePath} is still timing out after 2 attempts. See more info here https://nextjs.org/docs/messages/page-data-collection-timeout`
Expand Down Expand Up @@ -615,20 +629,8 @@ function createStaticWorker(
},
},
enableWorkerThreads: config.experimental.workerThreads,
exposedMethods: [
'hasCustomGetInitialProps',
'isPageStatic',
'getDefinedNamedExports',
'exportPage',
],
}) as Worker &
Pick<
typeof import('./worker'),
| 'hasCustomGetInitialProps'
| 'isPageStatic'
| 'getDefinedNamedExports'
| 'exportPage'
>
exposedMethods: staticWorkerExposedMethods,
}) as StaticWorker
}

async function writeFullyStaticExport(
Expand Down Expand Up @@ -1612,12 +1614,12 @@ export default async function build(
nonStaticErrorPageSpan.traceAsyncFn(
async () =>
hasCustomErrorPage &&
(await pagesStaticWorkers.hasCustomGetInitialProps(
'/_error',
(await pagesStaticWorkers.hasCustomGetInitialProps({
page: '/_error',
distDir,
runtimeEnvConfig,
false
))
checkingApp: false,
}))
)

const errorPageStaticResult = nonStaticErrorPageSpan.traceAsyncFn(
Expand All @@ -1640,18 +1642,18 @@ export default async function build(
const appPageToCheck = '/_app'

const customAppGetInitialPropsPromise =
pagesStaticWorkers.hasCustomGetInitialProps(
appPageToCheck,
pagesStaticWorkers.hasCustomGetInitialProps({
page: appPageToCheck,
distDir,
runtimeEnvConfig,
true
)
checkingApp: true,
})

const namedExportsPromise = pagesStaticWorkers.getDefinedNamedExports(
appPageToCheck,
const namedExportsPromise = pagesStaticWorkers.getDefinedNamedExports({
page: appPageToCheck,
distDir,
runtimeEnvConfig
)
runtimeEnvConfig,
})

// eslint-disable-next-line @typescript-eslint/no-shadow
let isNextImageImported: boolean | undefined
Expand Down
27 changes: 18 additions & 9 deletions packages/next/src/build/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1788,12 +1788,17 @@ export async function isPageStatic({
})
}

export async function hasCustomGetInitialProps(
page: string,
distDir: string,
runtimeEnvConfig: any,
export async function hasCustomGetInitialProps({
page,
distDir,
runtimeEnvConfig,
checkingApp,
}: {
page: string
distDir: string
runtimeEnvConfig: any
checkingApp: boolean
): Promise<boolean> {
}): Promise<boolean> {
require('../shared/lib/runtime-config.external').setConfig(runtimeEnvConfig)

const components = await loadComponents({
Expand All @@ -1812,11 +1817,15 @@ export async function hasCustomGetInitialProps(
return mod.getInitialProps !== mod.origGetInitialProps
}

export async function getDefinedNamedExports(
page: string,
distDir: string,
export async function getDefinedNamedExports({
page,
distDir,
runtimeEnvConfig,
}: {
page: string
distDir: string
runtimeEnvConfig: any
): Promise<ReadonlyArray<string>> {
}): Promise<ReadonlyArray<string>> {
require('../shared/lib/runtime-config.external').setConfig(runtimeEnvConfig)
const components = await loadComponents({
distDir,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
import { RouteKind } from '../../../../server/future/route-kind'
import { normalizePagePath } from '../../../../shared/lib/page-path/normalize-page-path'
import { decodeFromBase64, encodeToBase64 } from '../utils'
import { isInstrumentationHookFile } from '../../../worker'
import { isInstrumentationHookFile } from '../../../utils'
import { loadEntrypoint } from '../../../load-entrypoint'
import type { MappedPages } from '../../../build-context'

Expand Down
6 changes: 5 additions & 1 deletion packages/next/src/build/worker.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
import '../server/require-hook'

export * from './utils'
export {
getDefinedNamedExports,
hasCustomGetInitialProps,
isPageStatic,
} from './utils'
import exportPage from '../export/worker'
export { exportPage }
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ import {
isInstrumentationHookFile,
getPossibleMiddlewareFilenames,
getPossibleInstrumentationHookFilenames,
} from '../../../build/worker'
} from '../../../build/utils'
import {
createOriginalStackFrame,
getErrorSource,
Expand Down

0 comments on commit be37d3e

Please sign in to comment.