Build and generate cleanup #59420

wyattjoh · 2023-12-08T19:33:08Z

This is taking a pass at strengthening the Typescript types and organization of some of the build and generate steps. This is aligned with improving Partial Pre-Rendering (PPR) support as it delegates detection of PPR to the static analysis/generation stage. This ensures that when we want to tweak the logic of what pages are configured for PPR, that we only need to do so in a single place.

This also added some runtime checks for values to prevent unexpected failures.

Existing test suites are sufficient to verify the new behavior.

Closes NEXT-1834

wyattjoh · 2023-12-08T19:33:19Z

Current dependencies on/for this PR:

canary
- PR Build and generate cleanup #59420 👈

This stack of pull requests is managed by Graphite.

ijjk · 2023-12-08T19:35:36Z

Tests Passed

ijjk · 2023-12-08T21:39:40Z

Stats from current PR

Default Build

General Overall increase ⚠️

	vercel/next.js canary	vercel/next.js refactor/build-info-cleanup	Change
buildDuration	10.8s	10.7s	N/A
buildDurationCached	6s	5.9s	N/A
nodeModulesSize	199 MB	199 MB	⚠️ +19.9 kB
nextStartRea..uration (ms)	423ms	423ms	✓

Client Bundles (main, webpack)

	vercel/next.js canary	vercel/next.js refactor/build-info-cleanup	Change
170-HASH.js gzip	26.7 kB	26.7 kB	N/A
199.HASH.js gzip	181 B	185 B	N/A
3f784ff6-HASH.js gzip	53.3 kB	53.3 kB	✓
framework-HASH.js gzip	45.2 kB	45.2 kB	✓
main-app-HASH.js gzip	242 B	243 B	N/A
main-HASH.js gzip	31.7 kB	31.7 kB	N/A
webpack-HASH.js gzip	1.7 kB	1.7 kB	N/A
Overall change	98.5 kB	98.5 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	vercel/next.js refactor/build-info-cleanup	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	vercel/next.js refactor/build-info-cleanup	Change
_app-HASH.js gzip	195 B	194 B	N/A
_error-HASH.js gzip	183 B	182 B	N/A
amp-HASH.js gzip	501 B	501 B	✓
css-HASH.js gzip	321 B	321 B	✓
dynamic-HASH.js gzip	2.5 kB	2.5 kB	N/A
edge-ssr-HASH.js gzip	255 B	255 B	✓
head-HASH.js gzip	349 B	350 B	N/A
hooks-HASH.js gzip	368 B	369 B	N/A
image-HASH.js gzip	4.27 kB	4.27 kB	N/A
index-HASH.js gzip	255 B	256 B	N/A
link-HASH.js gzip	2.6 kB	2.6 kB	N/A
routerDirect..HASH.js gzip	311 B	309 B	N/A
script-HASH.js gzip	384 B	384 B	✓
withRouter-HASH.js gzip	307 B	306 B	N/A
1afbb74e6ecf..834.css gzip	106 B	106 B	✓
Overall change	1.57 kB	1.57 kB	✓

Client Build Manifests

	vercel/next.js canary	vercel/next.js refactor/build-info-cleanup	Change
_buildManifest.js gzip	483 B	483 B	✓
Overall change	483 B	483 B	✓

Rendered Page Sizes

	vercel/next.js canary	vercel/next.js refactor/build-info-cleanup	Change
index.html gzip	529 B	529 B	✓
link.html gzip	542 B	542 B	✓
withRouter.html gzip	524 B	524 B	✓
Overall change	1.59 kB	1.59 kB	✓

Edge SSR bundle Size

	vercel/next.js canary	vercel/next.js refactor/build-info-cleanup	Change
edge-ssr.js gzip	93.5 kB	93.6 kB	N/A
page.js gzip	146 kB	146 kB	N/A
Overall change	0 B	0 B	✓

Middleware size

	vercel/next.js canary	vercel/next.js refactor/build-info-cleanup	Change
middleware-b..fest.js gzip	622 B	622 B	✓
middleware-r..fest.js gzip	151 B	151 B	✓
middleware.js gzip	37.4 kB	37.4 kB	N/A
edge-runtime..pack.js gzip	1.92 kB	1.92 kB	✓
Overall change	2.7 kB	2.7 kB	✓

Next Runtimes

	vercel/next.js canary	vercel/next.js refactor/build-info-cleanup	Change
app-page-exp...dev.js gzip	168 kB	168 kB	N/A
app-page-exp..prod.js gzip	93.9 kB	94 kB	N/A
app-page-tur..prod.js gzip	94.7 kB	94.7 kB	N/A
app-page-tur..prod.js gzip	89.2 kB	89.3 kB	N/A
app-page.run...dev.js gzip	138 kB	138 kB	N/A
app-page.run..prod.js gzip	88.6 kB	88.6 kB	N/A
app-route-ex...dev.js gzip	24 kB	24 kB	✓
app-route-ex..prod.js gzip	16.7 kB	16.7 kB	✓
app-route-tu..prod.js gzip	16.7 kB	16.7 kB	✓
app-route-tu..prod.js gzip	16.3 kB	16.3 kB	✓
app-route.ru...dev.js gzip	23.4 kB	23.4 kB	✓
app-route.ru..prod.js gzip	16.3 kB	16.3 kB	✓
pages-api-tu..prod.js gzip	9.37 kB	9.4 kB	N/A
pages-api.ru...dev.js gzip	9.64 kB	9.67 kB	N/A
pages-api.ru..prod.js gzip	9.37 kB	9.39 kB	N/A
pages-turbo...prod.js gzip	21.9 kB	22 kB	N/A
pages.runtim...dev.js gzip	22.6 kB	22.6 kB	N/A
pages.runtim..prod.js gzip	21.9 kB	22 kB	N/A
server.runti..prod.js gzip	49.4 kB	49.4 kB	N/A
Overall change	113 kB	113 kB	✓

Diff details

Diff for page.js

Diff too large to display

Diff for edge-ssr.js

Diff too large to display

Diff for 170-HASH.js

Diff too large to display

Diff for app-page-exp..ntime.dev.js

failed to diff

Diff for app-page-exp..time.prod.js

Diff too large to display

Diff for app-page-tur..time.prod.js

Diff too large to display

Diff for app-page-tur..time.prod.js

Diff too large to display

Diff for app-page.runtime.dev.js

Diff too large to display

Diff for app-page.runtime.prod.js

Diff too large to display

Diff for pages-api-tu..time.prod.js

Diff too large to display

Diff for pages-api.runtime.dev.js

Diff too large to display

Diff for pages-api.ru..time.prod.js

Diff too large to display

Diff for pages-turbo...time.prod.js

Diff too large to display

Diff for pages.runtime.dev.js

Diff too large to display

Diff for pages.runtime.prod.js

Diff too large to display

Diff for server.runtime.prod.js

Diff too large to display

Commit: e9e957e

wyattjoh · 2023-12-08T22:46:55Z

packages/next/src/build/index.ts

+function createAppDataRouteInfo(
+  page: string,
+  {
+    supportsPPR,
+    isRouteHandler,
+  }: {
+    supportsPPR: boolean
+    isRouteHandler: boolean
+  }
+): DataRouteRouteInfo {
+  const normalizedRoute = normalizePagePath(page)
+
+  // If the page is not a route handler and the page is either doesn't support
+  // PPR or is not PPR, we need to generate a data route.
+  let dataRoute: string | null = null
+  if (!isRouteHandler) {
+    dataRoute = path.posix.join(`${normalizedRoute}${RSC_SUFFIX}`)
+  }
+
+  let prefetchDataRoute: string | undefined
+  if (supportsPPR) {
+    prefetchDataRoute = path.posix.join(
+      `${normalizedRoute}${RSC_PREFETCH_SUFFIX}`
+    )
+  }
+
+  return { dataRoute, prefetchDataRoute }
+}


Pulling this into it's own function de-duplicates this logic.

wyattjoh · 2023-12-08T22:48:22Z

packages/next/src/build/index.ts

@@ -2373,33 +2390,27 @@ export default async function build(
                hasDynamicData = true
                // we might have determined during prerendering that this page
                // used dynamic data
-                pageInfos.set(route, {
-                  ...(pageInfos.get(route) as PageInfo),
+                pageInfoRegistry.patch(route, {


This .patch method checks if the underlying page exists first, otherwise it throws. This ensures that we don't put the registry into an invalid state.

wyattjoh · 2023-12-08T22:49:21Z

packages/next/src/build/utils.ts

@@ -1253,7 +1296,7 @@ export async function buildAppStaticPaths({
  fetchCacheKeyPrefix?: string
  maxMemoryCacheSize?: number
  requestHeaders: IncrementalCache['requestHeaders']
-  ppr: boolean
+  nextConfigPPREnabled: boolean


The renaming here dramatically improves the clarity of the logic.

wyattjoh · 2023-12-08T22:50:26Z

packages/next/src/build/utils.ts

+        // this page statically unless PPR is enabled, in which case the static
+        // shell can always be prerendered.
+        if (appConfig.dynamic === 'force-dynamic') {
+          appConfig.revalidate = isPPR ? false : 0


When a page is going to be rendered using PPR, if force-dynamic is set on the segment config, the entire page at the root will postpone, resulting in a completly static (and safe) payload. This can be cached for lifetime of the build.

wyattjoh · 2023-12-08T22:51:34Z

packages/next/src/build/index.ts

+             */
+            const supportsPPR = !isRouteHandler && nextConfigPPREnabled
+
+            const experimentalPPR = isPPR ? true : undefined


We're now using the information yielded from the static analysis to determine if the page is static or not rather than duplicating the logic here.

wyattjoh · 2023-12-08T22:51:55Z

packages/next/src/build/utils.ts

+  const parentSegments: string[] = []
+
+  const stack: LoaderTree[] = [tree]
+  while (stack.length > 0) {


This change replaced the recursion with a loop/stack.

wyattjoh · 2023-12-08T22:53:14Z

packages/next/src/lib/interop-default.ts

+export function interopDefault<T = any>(mod: unknown): T | undefined {
+  // If the module is falsy (like `null`), just return undefined.
+  if (!mod) return undefined
+
+  // If the module has a default export (named 'default'), return that.
+  if (
+    (typeof mod === 'object' || typeof mod === 'function') &&
+    'default' in mod &&
+    mod.default
+  ) {
+    return mod.default as T
+  }
+
+  return mod as T


The typing updates here improve the reliability of the return and add optional typescript types to the interopted modules.

I'm open to adjusting this to always return unknown, but the any return here was too dangerous and resulted in a bug.

wyattjoh · 2023-12-08T22:54:06Z

packages/next/src/server/load-components.ts

+    Document = interopDefault(DocumentMod)
+    App = interopDefault(AppMod)


These interoped values are now correctly including undefined, ensuring that downstream methods check the existence of the values prior to usage.

wyattjoh · 2023-12-08T22:54:45Z

packages/next/src/server/render.tsx

@@ -452,15 +452,17 @@ export async function renderToHTMLImpl(
    runtime: globalRuntime,
    isExperimentalCompile,
  } = renderOpts
+
+  if (!renderOpts.Component) {
+    throw new Error('Invariant: Cannot render without a Component')


Invarients here ensure that we don't have mystery failures due to passing around an any which was undefined.

gnoff · 2023-12-08T23:54:48Z

@wyattjoh are you able to reopen as separate PRs? It seems like there are 3 or 4 distinct refactors going on here and it would help me track them if we could keep them separate? For instance your explanatory comments basically make good PR descriptions

gnoff · 2023-12-08T23:58:24Z

packages/next/src/build/page-info.ts

+   */
+  public get(page: string, throwIfUndefined: true): PageInfo
+  public get(page: string, throwIfUndefined?: boolean): PageInfo | undefined
+  public get(page: string, throwIfUndefined?: boolean): PageInfo | undefined {


If the callsite knows whether a throw is necessary then the callsite should just thrown when this map doesn't return anything

gnoff · 2023-12-09T00:01:59Z

packages/next/src/build/page-info.ts

+ * build output.
+ */
+
+export class PageInfoRegistry extends Map<string, PageInfo> {


extending a built-in in this way IMO obfuscates what the registry can do. For instance if I see the type of the registry is a map I know I can do Map-like stuff to it but if I see it is some class I have to build a new mental model of it's some base thing + some additional capabilities.

I'd argue that this particular abstraction isn't valuable and a simple patchPageInfo function would suffice everyone we call registry.patch and a expectPageInfo() function would suffice if you want to throw on missing page data. Then the other places where you interact with the page info as a regular Map are much more clear about what that object is

gnoff · 2023-12-09T00:32:15Z

packages/next/src/build/utils.ts

+  const parentSegments: string[] = []
+
+  const stack: LoaderTree[] = [tree]
+  while (stack.length > 0) {


Seems like the stack is unnecessary, it only ever has one element in it. Just track this with a variablelike

let currentLoaderTree = tree while(currentLoaderTree) { ... currentLoaderTree = parallelRoutes.children }

gnoff · 2023-12-09T00:33:46Z

packages/next/src/build/utils.ts

@@ -1393,6 +1440,44 @@ export async function buildAppStaticPaths({
  )
 }

+function resolveAppConfig(generateParams: GenerateParams): AppConfig {


This function is only called in one place, consider just constructing the AppConfig while you are producing the GenerateParams. No reason to re-traverse each layer when you are already doing it

This updates the `collectGenerateParams` method to use a loop rather than being recursive as well as updating some of the Typescript types. This is a follow up of #59420. Closes NEXT-1839

This updates the some of the logic around updating `PageInfo` entries in the `pageInfos` map. This is a followup to #59420. Closes NEXT-1838

ijjk added created-by: Next.js team PRs by the Next.js team. type: next labels Dec 8, 2023

wyattjoh force-pushed the refactor/build-info-cleanup branch 2 times, most recently from 040358f to dde8b6f Compare December 8, 2023 21:32

wyattjoh force-pushed the refactor/build-info-cleanup branch 3 times, most recently from 339fe87 to 2da50b5 Compare December 8, 2023 22:38

refactor: cleans up the logic to de-duplicate some checks

e9e957e

wyattjoh commented Dec 8, 2023

View reviewed changes

wyattjoh force-pushed the refactor/build-info-cleanup branch from 2da50b5 to e9e957e Compare December 8, 2023 22:55

wyattjoh marked this pull request as ready for review December 8, 2023 22:55

wyattjoh requested review from timneutkens, ijjk, shuding, huozhi, ztanner, feedthejim and a team as code owners December 8, 2023 22:55

gnoff reviewed Dec 9, 2023

View reviewed changes

wyattjoh closed this Dec 9, 2023

This was referenced Dec 9, 2023

Page Info Cleanup #59430

Merged

Generate Params Cleanup #59431

Merged

wyattjoh added a commit that referenced this pull request Dec 12, 2023

Page Info Cleanup (#59430)

ed12b55

This updates the some of the logic around updating `PageInfo` entries in the `pageInfos` map. This is a followup to #59420. Closes NEXT-1838

github-actions bot added the locked label Dec 23, 2023

github-actions bot locked as resolved and limited conversation to collaborators Dec 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build and generate cleanup #59420

Build and generate cleanup #59420

wyattjoh commented Dec 8, 2023 •

edited

Loading

wyattjoh commented Dec 8, 2023

ijjk commented Dec 8, 2023 •

edited

Loading

ijjk commented Dec 8, 2023 •

edited

Loading

wyattjoh Dec 8, 2023

wyattjoh Dec 8, 2023

wyattjoh Dec 8, 2023

wyattjoh Dec 8, 2023

wyattjoh Dec 8, 2023

wyattjoh Dec 8, 2023

wyattjoh Dec 8, 2023

wyattjoh Dec 8, 2023

wyattjoh Dec 8, 2023

gnoff commented Dec 8, 2023

gnoff Dec 8, 2023

gnoff Dec 9, 2023

gnoff Dec 9, 2023

gnoff Dec 9, 2023

		Document = interopDefault(DocumentMod)
		App = interopDefault(AppMod)

Build and generate cleanup #59420

Build and generate cleanup #59420

Conversation

wyattjoh commented Dec 8, 2023 • edited Loading

wyattjoh commented Dec 8, 2023

ijjk commented Dec 8, 2023 • edited Loading

Tests Passed

ijjk commented Dec 8, 2023 • edited Loading

Stats from current PR

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnoff commented Dec 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wyattjoh commented Dec 8, 2023 •

edited

Loading

ijjk commented Dec 8, 2023 •

edited

Loading

ijjk commented Dec 8, 2023 •

edited

Loading