Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Page Info Cleanup #59430

Merged
merged 3 commits into from
Dec 12, 2023
Merged

Page Info Cleanup #59430

merged 3 commits into from
Dec 12, 2023

Conversation

wyattjoh
Copy link
Member

@wyattjoh wyattjoh commented Dec 9, 2023

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 ijjk added created-by: Next.js team PRs by the Next.js team type: next labels Dec 9, 2023
@wyattjoh
Copy link
Member Author

wyattjoh commented Dec 9, 2023

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

Comment on lines 353 to 380
function createAppDataRouteInfo(
page: string,
{
experimentalPPR,
isRouteHandler,
}: {
experimentalPPR: boolean | undefined
isRouteHandler: boolean
}
): DataRouteRouteInfo {
const normalizedRoute = normalizePagePath(page)

// If the page is not a route handler 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 (experimentalPPR) {
prefetchDataRoute = path.posix.join(
`${normalizedRoute}${RSC_PREFETCH_SUFFIX}`
)
}

return { dataRoute, prefetchDataRoute }
}

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@wyattjoh wyattjoh marked this pull request as ready for review December 9, 2023 02:27
@ijjk
Copy link
Member

ijjk commented Dec 9, 2023

Stats from current PR

Default Build
General Overall increase ⚠️
vercel/next.js canary vercel/next.js refactor/page-info Change
buildDuration 11.1s 11.2s ⚠️ +162ms
buildDurationCached 6.4s 6.4s N/A
nodeModulesSize 200 MB 200 MB ⚠️ +2.29 kB
nextStartRea..uration (ms) 407ms 409ms N/A
Client Bundles (main, webpack)
vercel/next.js canary vercel/next.js refactor/page-info 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 240 B 241 B N/A
main-HASH.js gzip 31.7 kB 31.6 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/page-info 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/page-info 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.61 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/page-info Change
_buildManifest.js gzip 484 B 482 B N/A
Overall change 0 B 0 B
Rendered Page Sizes
vercel/next.js canary vercel/next.js refactor/page-info Change
index.html gzip 530 B 527 B N/A
link.html gzip 542 B 540 B N/A
withRouter.html gzip 524 B 524 B
Overall change 524 B 524 B
Edge SSR bundle Size
vercel/next.js canary vercel/next.js refactor/page-info Change
edge-ssr.js gzip 93.7 kB 93.7 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/page-info Change
middleware-b..fest.js gzip 627 B 622 B N/A
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.07 kB 2.07 kB
Next Runtimes
vercel/next.js canary vercel/next.js refactor/page-info Change
app-page-exp...dev.js gzip 168 kB 168 kB
app-page-exp..prod.js gzip 93.9 kB 93.9 kB
app-page-tur..prod.js gzip 94.7 kB 94.7 kB
app-page-tur..prod.js gzip 89.2 kB 89.2 kB
app-page.run...dev.js gzip 138 kB 138 kB
app-page.run..prod.js gzip 88.5 kB 88.5 kB
app-route-ex...dev.js gzip 23.9 kB 23.9 kB
app-route-ex..prod.js gzip 16.6 kB 16.6 kB
app-route-tu..prod.js gzip 16.6 kB 16.6 kB
app-route-tu..prod.js gzip 16.2 kB 16.2 kB
app-route.ru...dev.js gzip 23.4 kB 23.4 kB
app-route.ru..prod.js gzip 16.2 kB 16.2 kB
pages-api-tu..prod.js gzip 9.37 kB 9.37 kB
pages-api.ru...dev.js gzip 9.64 kB 9.64 kB
pages-api.ru..prod.js gzip 9.37 kB 9.37 kB
pages-turbo...prod.js gzip 21.9 kB 21.9 kB
pages.runtim...dev.js gzip 22.5 kB 22.5 kB
pages.runtim..prod.js gzip 21.9 kB 21.9 kB
server.runti..prod.js gzip 49.4 kB 49.4 kB
Overall change 929 kB 929 kB
Commit: a79e9a4

@ijjk
Copy link
Member

ijjk commented Dec 9, 2023

Tests Passed

packages/next/src/build/index.ts Outdated Show resolved Hide resolved
packages/next/src/build/index.ts Outdated Show resolved Hide resolved
@@ -349,6 +350,34 @@ function pageToRoute(page: string) {
}
}

function createAppDataRouteInfo(
Copy link
Contributor

Choose a reason for hiding this comment

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

This function seems odd to me

experimentalPPR will always be false when isRouteHandler is true. So having two boolean flags seems suspect already. My guess is you should have two functions one for route handlers and one for renders. Then just call the right one based on the situation. You can also then just read from config to determine if you need the PPR prefetch route

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately we use the same larger build function here to build each of the pages (pages and routes included), so the point of extracting this was to allow the "app data routes" to be defined by the dependencies (isRouteHandler and experimentalPPR).

You can also then just read from config to determine if you need the PPR prefetch route

experimentalPPR means something specific here, it means that this particular route is enabled for PPR. I'm currently looking into cases like when there's a segment config of dynamic = "force-static" that should actually ensure that the route doesn't opt into PPR behaviors, and should instead be outputted on infrastructure as a purely static route.

So technically having this function here extracting this logic is preparing us for that particular distinction, which I think is important for documenting.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does a route handler having a dataRoute or prefetchDataRoute ever make sense?

Maybe my issue is with the polymorphism of the route type. Reading the code it is really hard to track that a route handler will always have a null/undefined value for these two items.

Something like this reads more verbosely but IMO much clearer in intention and behavior.

let dataRoute, prefetchDataRoute;
if (isRouteHandler) {
  // Route Handlers never have a dataRoute or pefetchDataRoute
  dataRoute = null;
  prefetchDataRoute = undefined;
} else {
  // render pages always have a dataRoute and if PPR is enabled for this page or route segment
  // it must also have prefetchDataRoute
  const normalizedRoute = normalizePagePath(...)
  dataRoute = dataRouteFromNormalizedRoute(normalizedRoute);
  
  if (experimentalPPR) {
    prefetchDataRoute = prefetchDataRouteFromNormalizedRoute(normalizedRoute);
  } else {
    prefetchDataRoute = undefined;
  }
}

The fact that this needs to be duplicated in a few places is unfortunate but I think that could be solved by forking the route handler treatment and render handlers at a higher level. By putting that inside createAppDataRouteInfo you obuscate the fact that routeHandlers will never have these routes

@wyattjoh wyattjoh merged commit ed12b55 into canary Dec 12, 2023
69 checks passed
@wyattjoh wyattjoh deleted the refactor/page-info branch December 12, 2023 21:37
ztanner added a commit that referenced this pull request Dec 13, 2023
ztanner added a commit that referenced this pull request Dec 13, 2023
ztanner added a commit that referenced this pull request Dec 13, 2023
This appears to be causing a build issue and requires deeper
investigation. Reverting for now.

- Reverts #59430 

[slack
x-ref](https://vercel.slack.com/archives/C06AARWFZFB/p1702491612961359)

Closes NEXT-1870
wyattjoh added a commit that referenced this pull request Dec 19, 2023
@wyattjoh wyattjoh mentioned this pull request Dec 19, 2023
wyattjoh added a commit that referenced this pull request Dec 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants