Skip to content

Enable PPR tests for test suites#58708

Closed
wyattjoh wants to merge 15 commits into
feat/force-dynamic-pprfrom
tests/expand-test-suites-for-ppr
Closed

Enable PPR tests for test suites#58708
wyattjoh wants to merge 15 commits into
feat/force-dynamic-pprfrom
tests/expand-test-suites-for-ppr

Conversation

@wyattjoh

@wyattjoh wyattjoh commented Nov 20, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

This adds a new set of test suites that enable PPR by default to catch regressions on the app directory. Once PPR is the default, these test suites can be deleted.

This also introduces a slight variant to the test manifest file format to have it opt-in by default rather than opt-out. This is signaled by the "version": 2 added to the manifest.

The test coverage can be debugged with the following script: https://gist.github.com/wyattjoh/331240e1dd9fd913a2412f9cb9c0de54

@ijjk ijjk added created-by: Next.js team PRs by the Next.js team. type: next labels Nov 20, 2023
@wyattjoh

wyattjoh commented Nov 20, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@ijjk

ijjk commented Nov 20, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Stats from current PR

Default Build (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary vercel/next.js tests/expand-test-suites-for-ppr Change
buildDuration 10.7s 11s ⚠️ +242ms
buildDurationCached 6.2s 6.1s N/A
nodeModulesSize 199 MB 199 MB ⚠️ +82.9 kB
nextStartRea..uration (ms) 429ms 425ms N/A
Client Bundles (main, webpack)
vercel/next.js canary vercel/next.js tests/expand-test-suites-for-ppr Change
199-HASH.js gzip 28.7 kB 28.7 kB N/A
3f784ff6-HASH.js gzip 53.3 kB 53.3 kB ✓
494.HASH.js gzip 180 B 181 B N/A
framework-HASH.js gzip 45.2 kB 45.2 kB ✓
main-app-HASH.js gzip 241 B 239 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 ✓
Overall change 100 kB 100 kB ✓
Legacy Client Bundles (polyfills)
vercel/next.js canary vercel/next.js tests/expand-test-suites-for-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 tests/expand-test-suites-for-ppr Change
_app-HASH.js gzip 194 B 195 B N/A
_error-HASH.js gzip 182 B 181 B N/A
amp-HASH.js gzip 501 B 503 B N/A
css-HASH.js gzip 322 B 323 B N/A
dynamic-HASH.js gzip 2.5 kB 2.5 kB ✓
edge-ssr-HASH.js gzip 253 B 255 B N/A
head-HASH.js gzip 348 B 347 B N/A
hooks-HASH.js gzip 369 B 368 B N/A
image-HASH.js gzip 4.28 kB 4.27 kB N/A
index-HASH.js gzip 256 B 256 B ✓
link-HASH.js gzip 2.61 kB 2.6 kB N/A
routerDirect..HASH.js gzip 311 B 311 B ✓
script-HASH.js gzip 384 B 383 B N/A
withRouter-HASH.js gzip 307 B 308 B N/A
1afbb74e6ecf..834.css gzip 106 B 106 B ✓
Overall change 3.17 kB 3.17 kB ✓
Client Build Manifests
vercel/next.js canary vercel/next.js tests/expand-test-suites-for-ppr Change
_buildManifest.js gzip 484 B 483 B N/A
Overall change 0 B 0 B ✓
Rendered Page Sizes
vercel/next.js canary vercel/next.js tests/expand-test-suites-for-ppr Change
index.html gzip 529 B 527 B N/A
link.html gzip 539 B 540 B N/A
withRouter.html gzip 522 B 522 B ✓
Overall change 522 B 522 B ✓
Edge SSR bundle Size Overall increase ⚠️
vercel/next.js canary vercel/next.js tests/expand-test-suites-for-ppr Change
edge-ssr.js gzip 92.4 kB 92.4 kB N/A
page.js gzip 145 kB 145 kB ⚠️ +281 B
Overall change 145 kB 145 kB ⚠️ +281 B
Middleware size
vercel/next.js canary vercel/next.js tests/expand-test-suites-for-ppr Change
middleware-b..fest.js gzip 626 B 626 B ✓
middleware-r..fest.js gzip 150 B 151 B N/A
middleware.js gzip 24.8 kB 24.8 kB N/A
edge-runtime..pack.js gzip 1.92 kB 1.92 kB ✓
Overall change 2.55 kB 2.55 kB ✓
Next Runtimes Overall increase ⚠️
vercel/next.js canary vercel/next.js tests/expand-test-suites-for-ppr Change
app-page-exp...dev.js gzip 167 kB 167 kB ⚠️ +285 B
app-page-exp..prod.js gzip 93.3 kB 93.5 kB ⚠️ +287 B
app-page-tur..prod.js gzip 94 kB 94.3 kB ⚠️ +288 B
app-page-tur..prod.js gzip 88.6 kB 88.9 kB ⚠️ +286 B
app-page.run...dev.js gzip 137 kB 137 kB ⚠️ +287 B
app-page.run..prod.js gzip 87.9 kB 88.2 kB ⚠️ +290 B
app-route-ex...dev.js gzip 23.8 kB 23.8 kB N/A
app-route-ex..prod.js gzip 16.4 kB 16.4 kB N/A
app-route-tu..prod.js gzip 16.5 kB 16.4 kB N/A
app-route-tu..prod.js gzip 16 kB 16 kB N/A
app-route.ru...dev.js gzip 23.2 kB 23.2 kB N/A
app-route.ru..prod.js gzip 16 kB 16 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.8 kB 21.9 kB N/A
pages.runtim...dev.js gzip 22.5 kB 22.5 kB N/A
pages.runtim..prod.js gzip 21.8 kB 21.9 kB N/A
server.runti..prod.js gzip 49.1 kB 49.1 kB N/A
Overall change 696 kB 698 kB ⚠️ +1.72 kB
Diff details
Diff for page.js

Diff too large to display

Diff for edge-ssr.js

Diff too large to display

Diff for 199-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

Commit: fec438b

@wyattjoh wyattjoh force-pushed the tests/expand-test-suites-for-ppr branch from 79411ee to baa8746 Compare November 20, 2023 23:47
@ijjk

ijjk commented Nov 20, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Tests Passed

@wyattjoh wyattjoh force-pushed the tests/expand-test-suites-for-ppr branch from 3e3d029 to f8821a4 Compare November 21, 2023 06:14
@wyattjoh wyattjoh marked this pull request as ready for review November 21, 2023 07:12
@wyattjoh wyattjoh requested review from S3Prototype and StephDietz and removed request for a team November 21, 2023 07:12
@ghost

ghost commented Nov 21, 2023 •
edited by ghost
Loading

Copy link
Copy Markdown

Notifying the following users due to files changed in this PR based on this repo's notify modifiers:

@timneutkens, @ijjk, @shuding, @huozhi:

packages/next/src/server/config.ts

ztanner
ztanner reviewed Nov 21, 2023
View reviewed changes
Comment thread packages/next/src/server/config.ts Outdated
Comment thread packages/next/src/server/config.ts Outdated
@feedthejim

feedthejim commented Nov 21, 2023

Copy link
Copy Markdown
Contributor

I'm surprised this is all green for parallel routes and interception, can you double-check @wyattjoh ? otherwise, this is extremely good news

@wyattjoh wyattjoh force-pushed the tests/expand-test-suites-for-ppr branch from 44843b7 to a210699 Compare November 21, 2023 14:46
@wyattjoh wyattjoh marked this pull request as draft November 21, 2023 14:57
ztanner
ztanner reviewed Nov 21, 2023
View reviewed changes
Comment thread packages/next/src/server/base-server.ts Outdated
} else if (isAppPageRouteModule(routeModule)) {
if (
!opts.experimental.ppr &&
!renderOpts.experimental.ppr &&

@ztanner ztanner Nov 21, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

curious why this is changing?

@wyattjoh wyattjoh Nov 21, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The rest of the render code inspects the renderOpts object which is the merged opts + other spreads. This just brings it in-line to be accessing the same instance rather than this one which was technically accessing a higher instance. In an ideal world, the construction of renderOpts would occur out of scope of this function such that it just gets the "complete" version.

Comment thread packages/next/src/server/dev/next-dev-server.ts
isrFlushToDisk: this.nextConfig.experimental.isrFlushToDisk,
maxMemoryCacheSize: this.nextConfig.experimental.isrMemoryCacheSize,
ppr: this.nextConfig.experimental.ppr === true,
ppr: this.renderOpts.experimental.ppr,

@ztanner ztanner Nov 21, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

also wondering why this is changing. At first glance to me it seems like a bug that everything else reads from this.nextConfig.experimental except for this property

@wyattjoh wyattjoh Nov 21, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Because the enablement of PPR is dependant on the runtime + page kind, the experimental object was added to RenderOpts within app, enabling us to scope down the usage of renderOpts to specific page types. Ideally, we'd want to scope down access of each of the config options to each of the specific route implementaitons and runtimes.

In the most ideal world, we wouldn't pass around these huge bag-of-properties and instead rely on DI/IOC to isolate the configuration passing to the specific functions/classes/modules that require specific information from the configuration/render layers. That would solve the problem of PPR changes leaking into and breaking pages running inside pages/.

@wyattjoh wyattjoh force-pushed the tests/expand-test-suites-for-ppr branch from d93b41f to 4cc0557 Compare November 21, 2023 19:38
@wyattjoh wyattjoh changed the base branch from canary to feat/force-dynamic-ppr November 22, 2023 17:41
@wyattjoh wyattjoh force-pushed the tests/expand-test-suites-for-ppr branch from 4cc0557 to a2e7e3d Compare November 22, 2023 17:41
@wyattjoh wyattjoh mentioned this pull request Nov 22, 2023
Merged
@wyattjoh wyattjoh force-pushed the feat/force-dynamic-ppr branch from 3c14d7a to 1a06780 Compare November 22, 2023 17:54
@wyattjoh wyattjoh force-pushed the feat/force-dynamic-ppr branch from 77b2404 to 4118b3b Compare November 22, 2023 18:41
@wyattjoh wyattjoh force-pushed the tests/expand-test-suites-for-ppr branch from 1009358 to f46c895 Compare November 22, 2023 18:52
@wyattjoh wyattjoh force-pushed the tests/expand-test-suites-for-ppr branch 2 times, most recently from a89b157 to 628b0bb Compare November 23, 2023 00:38
@wyattjoh wyattjoh force-pushed the tests/expand-test-suites-for-ppr branch from 628b0bb to 5a94e70 Compare November 23, 2023 01:06
@wyattjoh wyattjoh force-pushed the tests/expand-test-suites-for-ppr branch 3 times, most recently from a5b16d0 to dc4c574 Compare November 23, 2023 05:18
@wyattjoh wyattjoh force-pushed the tests/expand-test-suites-for-ppr branch from dc4c574 to fec438b Compare November 23, 2023 05:29
@wyattjoh wyattjoh marked this pull request as ready for review November 23, 2023 05:53
@wyattjoh wyattjoh requested a review from a team as a code owner November 23, 2023 05:53
@ztanner ztanner mentioned this pull request Nov 28, 2023
Merged
kodiakhq Bot pushed a commit that referenced this pull request Nov 29, 2023
@ztanner @wyattjoh
Enable PPR tests for test suites (#59030)
809164d 
Cherry-picks #58708 without the dependency on #58779

Co-authored-by: Wyatt Johnson <[email protected]>
@ztanner

ztanner commented Nov 29, 2023

Copy link
Copy Markdown
Member

Closing since #59030 landed

@ztanner ztanner closed this Nov 29, 2023
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Dec 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ztanner ztanner ztanner left review comments
@timneutkens timneutkens Awaiting requested review from timneutkens
@ijjk ijjk Awaiting requested review from ijjk
@shuding shuding Awaiting requested review from shuding
@huozhi huozhi Awaiting requested review from huozhi
@feedthejim feedthejim Awaiting requested review from feedthejim
@S3Prototype S3Prototype Awaiting requested review from S3Prototype S3Prototype was automatically assigned from vercel/devex
@StephDietz StephDietz Awaiting requested review from StephDietz StephDietz was automatically assigned from vercel/devex

Assignees

No one assigned

Labels

created-by: Next.js team PRs by the Next.js team. locked type: next

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@wyattjoh @ijjk @feedthejim @ztanner @timneutkens