-
Notifications
You must be signed in to change notification settings - Fork 26.1k
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
Enable PPR for dynamic = "force-dynamic"
#58779
Conversation
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
3c14d7a
to
1a06780
Compare
8d1e7a7
to
ebe6477
Compare
1a06780
to
f8cec69
Compare
ebe6477
to
8cac08e
Compare
f8cec69
to
77b2404
Compare
Tests Passed |
Stats from current PRDefault Build (Increase detected
|
vercel/next.js canary | vercel/next.js feat/force-dynamic-ppr | Change | |
---|---|---|---|
buildDuration | 10.8s | 10.8s | N/A |
buildDurationCached | 5.9s | 6.1s | |
nodeModulesSize | 199 MB | 200 MB | |
nextStartRea..uration (ms) | 422ms | 420ms | N/A |
Client Bundles (main, webpack)
vercel/next.js canary | vercel/next.js feat/force-dynamic-ppr | Change | |
---|---|---|---|
170-HASH.js gzip | 30.6 kB | 30.3 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.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 feat/force-dynamic-ppr | 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/force-dynamic-ppr | 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 feat/force-dynamic-ppr | Change | |
---|---|---|---|
_buildManifest.js gzip | 483 B | 483 B | ✓ |
Overall change | 483 B | 483 B | ✓ |
Rendered Page Sizes
vercel/next.js canary | vercel/next.js feat/force-dynamic-ppr | Change | |
---|---|---|---|
index.html gzip | 528 B | 528 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 feat/force-dynamic-ppr | Change | |
---|---|---|---|
edge-ssr.js gzip | 93.5 kB | 93.5 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 feat/force-dynamic-ppr | Change | |
---|---|---|---|
middleware-b..fest.js gzip | 621 B | 624 B | N/A |
middleware-r..fest.js gzip | 151 B | 151 B | ✓ |
middleware.js gzip | 37.5 kB | 37.5 kB | N/A |
edge-runtime..pack.js gzip | 1.92 kB | 1.92 kB | ✓ |
Overall change | 2.07 kB | 2.07 kB | ✓ |
Next Runtimes Overall increase ⚠️
vercel/next.js canary | vercel/next.js feat/force-dynamic-ppr | Change | |
---|---|---|---|
app-page-exp...dev.js gzip | 168 kB | 168 kB | |
app-page-exp..prod.js gzip | 93.8 kB | 94 kB | |
app-page-tur..prod.js gzip | 94.5 kB | 94.7 kB | |
app-page-tur..prod.js gzip | 89.1 kB | 89.3 kB | |
app-page.run...dev.js gzip | 138 kB | 138 kB | |
app-page.run..prod.js gzip | 88.4 kB | 88.6 kB | |
app-route-ex...dev.js gzip | 24.2 kB | 24.2 kB | N/A |
app-route-ex..prod.js gzip | 16.8 kB | 16.8 kB | N/A |
app-route-tu..prod.js gzip | 16.9 kB | 16.8 kB | N/A |
app-route-tu..prod.js gzip | 16.4 kB | 16.4 kB | N/A |
app-route.ru...dev.js gzip | 23.6 kB | 23.6 kB | N/A |
app-route.ru..prod.js gzip | 16.4 kB | 16.4 kB | N/A |
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 | N/A |
pages.runtim...dev.js gzip | 22.6 kB | 22.6 kB | N/A |
pages.runtim..prod.js gzip | 21.9 kB | 21.9 kB | N/A |
server.runti..prod.js gzip | 49.4 kB | 49.4 kB | N/A |
Overall change | 700 kB | 701 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
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
Diff too large to display
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 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
8bbe107
to
550fc3a
Compare
77b2404
to
4118b3b
Compare
I assume there are not tests because this is tested in the following PR? |
if ( | ||
staticGenerationStore.forceDynamic && | ||
staticGenerationStore.isStaticGeneration && | ||
staticGenerationStore.experimental.ppr |
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.
Why is this a <MaybePostpone>
if you already checked the conditions? maybePostpone
checks the same things again. It could really just be <Postpone>
.
!staticGenerationStore.isStaticGeneration || |
It looks to me like the only special thing maybePostpone
does is set a special field on the static generation store. But this is fishy to me because don’t we need to bail out if React.postpone is called in userspace, too? So this suggests to me we’re either doing something redundant here or we’re missing logic elsewhere to handle the userspace postpone case.
This improves some of the typings around the `RenderResult` returned during renders. Previously it had a single large metadata object that was shared across both the pages and app render pipelines. To add more type safety, this splits the types used by each of the render pipelines into their own types while still allowing the default `RenderResult` to reference metadata from either render pipeline. This also improved the flight data generation for app renders. Previously, the promise was inlined, and errors were swallowed. With the advent of improvements in #58779 the postpone errors are no longer returned by the flight generation and are instead handled correctly inside the render by React so it can emit properly postponed flight data. Besides that there was some whitespace changes, so hiding whitespace differences during review should make it much clearer to review!
4118b3b
to
8b71aef
Compare
209e1c2
to
fa728c9
Compare
f2ee25f
to
e1319ce
Compare
uses: ./.github/workflows/build_reusable.yml | ||
with: | ||
nodeVersion: 18.17.0 | ||
afterBuild: node run-tests.js --timings -g ${{ matrix.group }}/12 -c ${TEST_CONCURRENCY} --type integration | ||
afterBuild: node run-tests.js --timings -g ${{ matrix.group }}2 -c ${TEST_CONCURRENCY} --type integration |
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.
@wyattjoh This seems like a typo that might introduce a bug? Seems like the 2
in ${{ matrix.group }}2
should be removed.
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.
Yup that was a mistake!
This makes some critical modifications to the app render pipeline when PPR has been enabled for pages with segments defining:
Importantly, it no longer modifies the revalidation time to zero for those pages, and now falls back to the provided default revalidation time. When static render occurs, if the page being rendered has a segment config defining
dynamic === "force-dynamic"
, then it will postpone at the root of the component tree. This ensures that no render code is executed for the page, as the entirety of the tree will have postponed. This fixes the bug where the flight prefetch wasn't generated correctly as well.