loading.tsx should have no effect on partial rendering when PPR is enabled #59196
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ijjk
added
area: tests
created-by: Next.js team
Tests Passed |
Stats from current PRDefault BuildGeneral Overall increase
|
| vercel/next.js canary | acdlite/next.js ppr-loading-tsx | Change | |
|---|---|---|---|
| buildDuration | 11s | 11.1s | N/A |
| buildDurationCached | 6.2s | 6.3s | N/A |
| nodeModulesSize | 199 MB | 199 MB | |
| nextStartRea..uration (ms) | 408ms | 407ms | N/A |
Client Bundles (main, webpack)
| vercel/next.js canary | acdlite/next.js ppr-loading-tsx | 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 | acdlite/next.js ppr-loading-tsx | Change | |
|---|---|---|---|
| polyfills-HASH.js gzip | 31 kB | 31 kB | ✓ |
| Overall change | 31 kB | 31 kB | ✓ |
Client Pages
| vercel/next.js canary | acdlite/next.js ppr-loading-tsx | 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 | acdlite/next.js ppr-loading-tsx | Change | |
|---|---|---|---|
| _buildManifest.js gzip | 483 B | 483 B | ✓ |
| Overall change | 483 B | 483 B | ✓ |
Rendered Page Sizes
| vercel/next.js canary | acdlite/next.js ppr-loading-tsx | Change | |
|---|---|---|---|
| index.html gzip | 529 B | 529 B | ✓ |
| link.html gzip | 542 B | 542 B | ✓ |
| withRouter.html gzip | 524 B | 525 B | N/A |
| Overall change | 1.07 kB | 1.07 kB | ✓ |
Edge SSR bundle Size
| vercel/next.js canary | acdlite/next.js ppr-loading-tsx | 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 | acdlite/next.js ppr-loading-tsx | Change | |
|---|---|---|---|
| middleware-b..fest.js gzip | 623 B | 624 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 | acdlite/next.js ppr-loading-tsx | 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.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.6 kB | 22.6 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 | 257 kB | 257 kB | ✓ |
Diff details
Diff for page.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
acdlite
requested review from
timneutkens,
ijjk,
shuding,
huozhi,
feedthejim,
ztanner and
wyattjoh
as code owners
acdlite
force-pushed
the
ppr-loading-tsx
branch
4 times, most recently
from
609a2e5
to
5ada678
Compare
ztanner
reviewed
Dec 8, 2023
| }) | ||
| }) | ||
|
|
||
| // NOTE: I've intentionally not yet moved these helpers into a separate |
There was a problem hiding this comment.
Should these things be part of test-data-service or is it intentionally separate?
There was a problem hiding this comment.
Eventually I'll probably move them to something like test-data-service/writer and test-data-service/reader but I just kept the writer ones in here for now
acdlite
force-pushed
the
ppr-loading-tsx
branch
3 times, most recently
from
48303ea
to
b07f956
Compare
Creates an e2e test case for prefetching and navigating when PPR is enabled. While I was writing this test, I noticed that it's currently pretty difficult to test all the scenarios that PPR is designed to handle, so I gave special attention to setting up a testing strategy that I hope will make this easier going forward. The overall pattern is based on how we've been testing concurrent rendering features in the React repo for many years: - In the e2e test, spin up an HTTP server for responding to requests sent by the test app. This simulates the data service that would be used in a real Next.js application, whether it's direct db access, an ORM, or a higher-level data access layer. The e2e test can observe when individual requests are received, and control the timing of when the data is fulfilled, without needing to mock any lower level I/O. (We're already using a similar pattern to test fetch deduping: https://github.com/vercel/next.js/blob/a3616d33edc40e7717b258f2636f280e30a3bccb/test/e2e/app-dir/app-fetch-deduping/app-fetch-deduping.test.ts#L8-L29) - Each time a request is received, write to an event log. Then assert on the result of the log at different points throughout the test. This helps catch subtle mistakes where the order of events is not expected, or the same event happens more than it should. (I wrote some test helpers, but to avoid early abstraction, I've intentionally not moved them into a separate module.)
acdlite
force-pushed
the
ppr-loading-tsx
branch
from
b07f956
to
0f36cfc
Compare
ztanner
approved these changes
Dec 8, 2023
Before PPR, the way instant navigations work in Next.js is we prefetch everything up to the first route segment that defines a loading.js boundary. The rest of the tree is defered until the actual navigation. It does not take into account whether the data is dynamic — even if the tree is completely static, it will still defer everything inside the loading boundary. The approach with PPR is different — we prefetch as deeply as possible, and only defer when dynamic data is accessed. If so, we only defer the nearest parent Suspense boundary of the dynamic data access, regardless of whether the boundary is defined by loading.js or a normal <Suspense> component in userspace. This PR removes the partial behavior of loading.js when the PPR flag is enabled. In effect, loading.js now acts like a regular Suspense boundary with no additional special behavior. Note that in practice this usually means we'll end up prefetching more than we were before PPR, which may or may not be considered a performance regression by some apps. The plan is to address this before General Availability of PPR by introducing granular per-segment fetching, so we can reuse as much of the tree as possible during both prefetches and dynamic navigations. But during the beta period, we should be clear about this trade off in our communications.
acdlite
force-pushed
the
ppr-loading-tsx
branch
from
c37335d
to
1d4c4a7
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before PPR, the way instant navigations work in Next.js is we prefetch everything up to the first route segment that defines a loading.js boundary. The rest of the tree is defered until the actual navigation. It does not take into account whether the data is dynamic — even if the tree is completely static, it will still defer everything inside the loading boundary.
The approach with PPR is different — we prefetch as deeply as possible, and only defer when dynamic data is accessed. If so, we only defer the nearest parent Suspense boundary of the dynamic data access, regardless of whether the boundary is defined by loading.js or a normal component in userspace.
This PR removes the partial behavior of loading.js when the PPR flag is enabled. In effect, loading.js now acts like a regular Suspense boundary with no additional special behavior.
Note that in practice this usually means we'll end up prefetching more than we were before PPR, which may or may not be considered a performance regression by some apps. The plan is to address this before General Availability of PPR by introducing granular per-segment fetching, so we can reuse as much of the tree as possible during both prefetches and dynamic navigations. But during the beta period, we should be clear about this trade off in our communications.
Testing strategy
While I was writing a test, I noticed that it's currently pretty difficult to test all the scenarios that PPR is designed to handle, so I gave special attention to setting up a testing strategy that I hope will make this easier going forward. The overall pattern is based on how we've been testing concurrent rendering features in the React repo for many years:
(I wrote some test helpers, but to avoid early abstraction, I've intentionally not moved them into a separate module.)
Closes NEXT-1779