-
Notifications
You must be signed in to change notification settings - Fork 27k
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
[PPR] Dynamic API Debugging #61798
[PPR] Dynamic API Debugging #61798
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Stats from current PRDefault Build (Increase detected
|
vercel/next.js canary | vercel/next.js feat/ppr-error-stacks | Change | |
---|---|---|---|
buildDuration | 14.2s | 14.1s | N/A |
buildDurationCached | 8.7s | 6.3s | N/A |
nodeModulesSize | 198 MB | 198 MB | |
nextStartRea..uration (ms) | 402ms | 410ms | N/A |
Client Bundles (main, webpack) Overall increase ⚠️
vercel/next.js canary | vercel/next.js feat/ppr-error-stacks | Change | |
---|---|---|---|
2453-HASH.js gzip | 30.8 kB | 31 kB | |
3304.HASH.js gzip | 181 B | 181 B | ✓ |
3f784ff6-HASH.js gzip | 53.7 kB | 53.7 kB | ✓ |
8299-HASH.js gzip | 5.04 kB | 5.04 kB | N/A |
framework-HASH.js gzip | 45.2 kB | 45.2 kB | ✓ |
main-app-HASH.js gzip | 242 B | 242 B | ✓ |
main-HASH.js gzip | 32.2 kB | 32.2 kB | N/A |
webpack-HASH.js gzip | 1.68 kB | 1.68 kB | N/A |
Overall change | 130 kB | 130 kB |
Legacy Client Bundles (polyfills)
vercel/next.js canary | vercel/next.js feat/ppr-error-stacks | Change | |
---|---|---|---|
polyfills-HASH.js gzip | 31 kB | 31 kB | ✓ |
Overall change | 31 kB | 31 kB | ✓ |
Client Pages
vercel/next.js canary | vercel/next.js feat/ppr-error-stacks | Change | |
---|---|---|---|
_app-HASH.js gzip | 196 B | 197 B | N/A |
_error-HASH.js gzip | 184 B | 184 B | ✓ |
amp-HASH.js gzip | 505 B | 505 B | ✓ |
css-HASH.js gzip | 324 B | 325 B | N/A |
dynamic-HASH.js gzip | 2.5 kB | 2.5 kB | N/A |
edge-ssr-HASH.js gzip | 258 B | 258 B | ✓ |
head-HASH.js gzip | 352 B | 352 B | ✓ |
hooks-HASH.js gzip | 370 B | 371 B | N/A |
image-HASH.js gzip | 4.21 kB | 4.21 kB | ✓ |
index-HASH.js gzip | 259 B | 259 B | ✓ |
link-HASH.js gzip | 2.67 kB | 2.67 kB | N/A |
routerDirect..HASH.js gzip | 314 B | 312 B | N/A |
script-HASH.js gzip | 386 B | 386 B | ✓ |
withRouter-HASH.js gzip | 309 B | 309 B | ✓ |
1afbb74e6ecf..834.css gzip | 106 B | 106 B | ✓ |
Overall change | 6.57 kB | 6.57 kB | ✓ |
Client Build Manifests
vercel/next.js canary | vercel/next.js feat/ppr-error-stacks | Change | |
---|---|---|---|
_buildManifest.js gzip | 481 B | 484 B | N/A |
Overall change | 0 B | 0 B | ✓ |
Rendered Page Sizes
vercel/next.js canary | vercel/next.js feat/ppr-error-stacks | Change | |
---|---|---|---|
index.html gzip | 529 B | 529 B | ✓ |
link.html gzip | 541 B | 541 B | ✓ |
withRouter.html gzip | 524 B | 523 B | N/A |
Overall change | 1.07 kB | 1.07 kB | ✓ |
Edge SSR bundle Size
vercel/next.js canary | vercel/next.js feat/ppr-error-stacks | Change | |
---|---|---|---|
edge-ssr.js gzip | 95.3 kB | 95.3 kB | N/A |
page.js gzip | 3.04 kB | 3.04 kB | N/A |
Overall change | 0 B | 0 B | ✓ |
Middleware size
vercel/next.js canary | vercel/next.js feat/ppr-error-stacks | Change | |
---|---|---|---|
middleware-b..fest.js gzip | 624 B | 625 B | N/A |
middleware-r..fest.js gzip | 151 B | 151 B | ✓ |
middleware.js gzip | 25.5 kB | 25.5 kB | N/A |
edge-runtime..pack.js gzip | 839 B | 839 B | ✓ |
Overall change | 990 B | 990 B | ✓ |
Next Runtimes Overall increase ⚠️
vercel/next.js canary | vercel/next.js feat/ppr-error-stacks | Change | |
---|---|---|---|
app-page-exp...dev.js gzip | 170 kB | 170 kB | |
app-page-exp..prod.js gzip | 96.8 kB | 97 kB | |
app-page-tur..prod.js gzip | 98.5 kB | 98.7 kB | |
app-page-tur..prod.js gzip | 92.8 kB | 93 kB | |
app-page.run...dev.js gzip | 144 kB | 144 kB | |
app-page.run..prod.js gzip | 91.3 kB | 91.5 kB | |
app-route-ex...dev.js gzip | 21.3 kB | 21.4 kB | N/A |
app-route-ex..prod.js gzip | 15 kB | 15.1 kB | N/A |
app-route-tu..prod.js gzip | 15 kB | 15.1 kB | N/A |
app-route-tu..prod.js gzip | 14.8 kB | 14.8 kB | N/A |
app-route.ru...dev.js gzip | 21 kB | 21 kB | N/A |
app-route.ru..prod.js gzip | 14.8 kB | 14.8 kB | N/A |
pages-api-tu..prod.js gzip | 9.55 kB | 9.55 kB | ✓ |
pages-api.ru...dev.js gzip | 9.82 kB | 9.82 kB | ✓ |
pages-api.ru..prod.js gzip | 9.55 kB | 9.55 kB | ✓ |
pages-turbo...prod.js gzip | 22.5 kB | 22.5 kB | ✓ |
pages.runtim...dev.js gzip | 23.1 kB | 23.1 kB | ✓ |
pages.runtim..prod.js gzip | 22.4 kB | 22.4 kB | ✓ |
server.runti..prod.js gzip | 50.9 kB | 51 kB | N/A |
Overall change | 790 kB | 791 kB |
build cache Overall increase ⚠️
vercel/next.js canary | vercel/next.js feat/ppr-error-stacks | Change | |
---|---|---|---|
0.pack gzip | 1.57 MB | 1.57 MB | |
index.pack gzip | 106 kB | 106 kB | |
Overall change | 1.67 MB | 1.68 MB |
Diff details
Diff for middleware.js
Diff too large to display
Diff for edge-ssr.js
Diff too large to display
Diff for 2453-HASH.js
Diff too large to display
Diff for app-page-exp..ntime.dev.js
Diff too large to display
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
failed to diff
Diff for app-page.runtime.prod.js
Diff too large to display
Diff for app-route-ex..ntime.dev.js
Diff too large to display
Diff for app-route-ex..time.prod.js
Diff too large to display
Diff for app-route-tu..time.prod.js
Diff too large to display
Diff for app-route-tu..time.prod.js
Diff too large to display
Diff for app-route.runtime.dev.js
Diff too large to display
Diff for app-route.ru..time.prod.js
Diff too large to display
Diff for server.runtime.prod.js
Diff too large to display
for (const stack of stacks) { | ||
error(stack) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems noisy for something that isn't an error case. Imagine you are happy with the level of dynamism of the app, you probably don't want logs on every build or revalidate that remind you of all the places you are triggering dynamic bailouts.
Maybe this is something we write to a manifest instead and there is some tooling you can do to print nicely dynamic usages after the build. And vercel can highlight this as part of the build output if you want to go investigate it after the fact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't going to log every dynamic bailout, it will only log when there was a dynamic bailout but no postponed state generated, indicating that there was a errant try/catch
present and React wasn't able to identify it as a postpone error.
ad3c368
to
05d8eca
Compare
05d8eca
to
7d1f9f3
Compare
.filter((line) => { | ||
// Exclude Next.js internals from the stack trace. | ||
if (line.includes('node_modules/next/')) { | ||
return false | ||
} | ||
|
||
// Exclude anonymous functions from the stack trace. | ||
if (line.includes(' (<anonymous>)')) { | ||
return false | ||
} | ||
|
||
// Exclude Node.js internals from the stack trace. | ||
if (line.includes(' (node:')) { | ||
return false | ||
} | ||
|
||
return true | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This formatting may be controversial, we can remove it if desired.
} & Pick< | ||
// Pull some properties from RenderOptsPartial so that the docs are also | ||
// mirrored. | ||
RenderOptsPartial, | ||
| 'originalPathname' | ||
| 'supportsDynamicHTML' | ||
| 'isRevalidate' | ||
| 'isBot' | ||
| 'nextExport' | ||
| 'isDraftMode' | ||
| 'isDebugPPRSkeleton' | ||
> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe something to follow up on later but having to repeat these values just so they can be exposed in async context is probably not good long term. We should maybe just make renderOpts available through a generalization of the staticGenerationContext (it's not really about staticGeneration only anymore anyway)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd argue in favour of narrowing the types here. The biggest problem with RenderOpts
is that it's overloaded, and holds way too many fields. Scoping down the fields pulled from here is a small step towards breaking the type up into smaller parts.
/** | ||
* When true, stack information will also be tracked during dynamic access. | ||
*/ | ||
isDebugSkeleton: boolean | undefined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this isn't really a property of the prerenderState but a property of the render itself. this object should generally hold mutable state about the the render and isDebugSkeleton shouldn't be mutated by the render itself so it shouldn't really live here
if ( | ||
staticGenerationStore.prerenderState && | ||
usedDynamicAPIs(staticGenerationStore.prerenderState) && | ||
staticGenerationStore.prerenderState?.isDebugSkeleton |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused about how this is supposed to work. We only have a prerenderState during build (and revalidate) but we shouldn't have a __nextppronly
query param at this time so won't this be false anytime the prerenderState exists?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The __nextppronly
query parameter is a stop-gap until we have more comprehensive debugging available for suspense and/or partial prerendering. While running with next dev
, when partial prerendering is enabled, you can show the static shell only. This pull request extends this to also print the Dynamic API's used that influenced the static shell generation. Any Dynamic API not called during the static shell generation doesn't affect it (yet).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in Dev the prerenderState exists when the query param is set?
7d1f9f3
to
316efb1
Compare
316efb1
to
1821cc8
Compare
Failing test suitesCommit: 1821cc8
Expand output● Export with default map › production mode › should export with folder that has dot in name
● Export with default map › production mode › should export an amp only page to clean path
● Export with default map › production mode › should export hybrid amp page correctly
● Export with default map › production mode › should export nested hybrid amp page correctly
● Export with default map › production mode › should export nested hybrid amp page correctly with folder
● Export with default map › production mode › should export hybrid index amp page correctly
Read more about building and testing Next.js in contributing.md. |
What?
This adds dynamic usage logging when the
__nextppronly
query parameter is set and partial prerendering has been enabled. This will print the stack trace for the accesses for all Dynamic API's that were called. This includes those API's that were called during the flight render if they were called before the static shell was ready.Why?
To take the most advantage of partial prerendering, it's important to track where Dynamic API's are called so developers can determine what has caused part of the component tree not to be included in the static shell. This also helps debug situations where the error thrown by Dynamic API's (used internally for tracking and interruption) are caught but not re-thrown, but due to the implementation of the flight renderer provided by React, we are unable to distinguish between errors that are swallowed by user code or by the flight renderer.
How?
Instead of using a boolean to track if a dynamic API was used, this actually captures the stack at the callsite, allowing developers to find the location in the codebase that invoked it.
Closes NEXT-2399