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

[use-cache] Exclude warmup render when dynamicIO is off #75527

Merged
merged 2 commits into from
Feb 3, 2025

Conversation

gnoff
Copy link
Contributor

@gnoff gnoff commented Jan 31, 2025

when using use-cache standalone we don't need the dev warmup render becuase it is for addressing environment names of Prerender in DIO mode. This fixes a bug where rejected promises from cookies appeared in server logs when use cache was enabled without dynamicIO.

@gnoff gnoff requested a review from unstubbable January 31, 2025 18:44
@ijjk ijjk added created-by: Next.js team PRs by the Next.js team. type: next labels Jan 31, 2025
Copy link
Contributor

@unstubbable unstubbable left a comment

Choose a reason for hiding this comment

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

The fix makes sense to me. A test would be nice though, as I wasn't able to reproduce the mentioned issue easily.

@gnoff gnoff force-pushed the use-cache-exclude-warmup branch from f2827aa to 8f0e25d Compare February 1, 2025 07:21
@ijjk ijjk added the tests label Feb 1, 2025
@gnoff
Copy link
Contributor Author

gnoff commented Feb 1, 2025

Was tricky to repro. Needs to have a loading.js to get the wamrup to do anything useful and then need to have dangling promises that are not already handled like the built-in ones are

@unstubbable
Copy link
Contributor

Now that this is clarified we can delete this TODO comment:

// TODO(useCache): Should this be a PrerenderStorePPR if dynamicIO is not
// enabled?
const prerenderStore: PrerenderStore = {
type: 'prerender',

@gnoff gnoff force-pushed the use-cache-exclude-warmup branch from e4b282e to 95901b1 Compare February 1, 2025 17:13
@ijjk
Copy link
Member

ijjk commented Feb 3, 2025

Failing test suites

Commit: 95901b1

pnpm test-start-turbo test/e2e/app-dir/rewrite-headers/rewrite-headers.test.ts (turbopack)

  • rewrite-headers > middleware rewrite external Prefetch RSC (/hello/vercel) > should have the expected headers
  • rewrite-headers > middleware rewrite external RSC (/hello/vercel) > should have the expected headers
Expand output

● rewrite-headers › middleware rewrite external RSC (/hello/vercel) › should have the expected headers

expect(received).toEqual(expected) // deep equality

- Expected  - 1
+ Received  + 1

  Object {
-   "x-nextjs-rewritten-path": null,
+   "x-nextjs-rewritten-path": "/home",
    "x-nextjs-rewritten-query": null,
  }

  404 |         })
  405 |
> 406 |         expect(headers).toEqual(expected)
      |                         ^
  407 |       })
  408 |     }
  409 |   )

  at Object.toEqual (e2e/app-dir/rewrite-headers/rewrite-headers.test.ts:406:25)

● rewrite-headers › middleware rewrite external Prefetch RSC (/hello/vercel) › should have the expected headers

expect(received).toEqual(expected) // deep equality

- Expected  - 1
+ Received  + 1

  Object {
-   "x-nextjs-rewritten-path": null,
+   "x-nextjs-rewritten-path": "/home",
    "x-nextjs-rewritten-query": null,
  }

  404 |         })
  405 |
> 406 |         expect(headers).toEqual(expected)
      |                         ^
  407 |       })
  408 |     }
  409 |   )

  at Object.toEqual (e2e/app-dir/rewrite-headers/rewrite-headers.test.ts:406:25)

Read more about building and testing Next.js in contributing.md.

gnoff added 2 commits February 3, 2025 08:02
when using use-cache standalone we don't need the dev warmup render becuase it is for addressing environment names of Prerender in DIO mode. This fixes a bug where rejected promises from cookies appeared in server logs when use cache was enabled without dynamicIO.
@gnoff gnoff force-pushed the use-cache-exclude-warmup branch from 95901b1 to 29a0485 Compare February 3, 2025 16:02
@ijjk
Copy link
Member

ijjk commented Feb 3, 2025

Stats from current PR

Default Build (Increase detected ⚠️)
General
vercel/next.js canary gnoff/next.js use-cache-exclude-warmup Change
buildDuration 19.4s 17.7s N/A
buildDurationCached 16.8s 14.3s N/A
nodeModulesSize 391 MB 391 MB N/A
nextStartRea..uration (ms) 464ms 446ms N/A
Client Bundles (main, webpack)
vercel/next.js canary gnoff/next.js use-cache-exclude-warmup Change
5306-HASH.js gzip 54 kB 53.9 kB N/A
8276.HASH.js gzip 169 B 168 B N/A
8377-HASH.js gzip 5.46 kB 5.46 kB N/A
bccd1874-HASH.js gzip 52.9 kB 52.9 kB N/A
framework-HASH.js gzip 57.5 kB 57.5 kB N/A
main-app-HASH.js gzip 240 B 242 B N/A
main-HASH.js gzip 34.5 kB 34.4 kB N/A
webpack-HASH.js gzip 1.71 kB 1.71 kB N/A
Overall change 0 B 0 B
Legacy Client Bundles (polyfills)
vercel/next.js canary gnoff/next.js use-cache-exclude-warmup Change
polyfills-HASH.js gzip 39.4 kB 39.4 kB
Overall change 39.4 kB 39.4 kB
Client Pages
vercel/next.js canary gnoff/next.js use-cache-exclude-warmup Change
_app-HASH.js gzip 193 B 193 B
_error-HASH.js gzip 193 B 193 B
amp-HASH.js gzip 512 B 510 B N/A
css-HASH.js gzip 343 B 342 B N/A
dynamic-HASH.js gzip 1.84 kB 1.84 kB
edge-ssr-HASH.js gzip 265 B 265 B
head-HASH.js gzip 363 B 362 B N/A
hooks-HASH.js gzip 393 B 392 B N/A
image-HASH.js gzip 4.59 kB 4.58 kB N/A
index-HASH.js gzip 268 B 268 B
link-HASH.js gzip 2.35 kB 2.35 kB N/A
routerDirect..HASH.js gzip 328 B 328 B
script-HASH.js gzip 397 B 397 B
withRouter-HASH.js gzip 323 B 326 B N/A
1afbb74e6ecf..834.css gzip 106 B 106 B
Overall change 3.59 kB 3.59 kB
Client Build Manifests
vercel/next.js canary gnoff/next.js use-cache-exclude-warmup Change
_buildManifest.js gzip 748 B 747 B N/A
Overall change 0 B 0 B
Rendered Page Sizes
vercel/next.js canary gnoff/next.js use-cache-exclude-warmup Change
index.html gzip 523 B 524 B N/A
link.html gzip 538 B 538 B
withRouter.html gzip 519 B 520 B N/A
Overall change 538 B 538 B
Edge SSR bundle Size
vercel/next.js canary gnoff/next.js use-cache-exclude-warmup Change
edge-ssr.js gzip 129 kB 129 kB N/A
page.js gzip 210 kB 210 kB N/A
Overall change 0 B 0 B
Middleware size
vercel/next.js canary gnoff/next.js use-cache-exclude-warmup Change
middleware-b..fest.js gzip 669 B 667 B N/A
middleware-r..fest.js gzip 155 B 156 B N/A
middleware.js gzip 31.3 kB 31.3 kB N/A
edge-runtime..pack.js gzip 844 B 844 B
Overall change 844 B 844 B
Next Runtimes
vercel/next.js canary gnoff/next.js use-cache-exclude-warmup Change
app-page-exp...dev.js gzip 386 kB 386 kB
app-page-exp..prod.js gzip 132 kB 132 kB
app-page-tur..prod.js gzip 145 kB 145 kB
app-page-tur..prod.js gzip 141 kB 141 kB
app-page.run...dev.js gzip 373 kB 373 kB N/A
app-page.run..prod.js gzip 128 kB 128 kB
app-route-ex...dev.js gzip 39.2 kB 39.2 kB
app-route-ex..prod.js gzip 25.6 kB 25.6 kB
app-route-tu..prod.js gzip 25.6 kB 25.6 kB
app-route-tu..prod.js gzip 25.4 kB 25.4 kB
app-route.ru...dev.js gzip 40.8 kB 40.8 kB
app-route.ru..prod.js gzip 25.4 kB 25.4 kB
dist_client_...dev.js gzip 356 B 356 B
dist_client_...dev.js gzip 349 B 349 B
pages-api-tu..prod.js gzip 9.69 kB 9.69 kB
pages-api.ru...dev.js gzip 11.8 kB 11.8 kB
pages-api.ru..prod.js gzip 9.68 kB 9.68 kB
pages-turbo...prod.js gzip 21.9 kB 21.9 kB
pages.runtim...dev.js gzip 31.5 kB 31.5 kB
pages.runtim..prod.js gzip 21.9 kB 21.9 kB
server.runti..prod.js gzip 60.2 kB 60.2 kB N/A
Overall change 1.22 MB 1.22 MB
build cache Overall increase ⚠️
vercel/next.js canary gnoff/next.js use-cache-exclude-warmup Change
0.pack gzip 2.1 MB 2.1 MB ⚠️ +1.8 kB
index.pack gzip 75.3 kB 75.4 kB ⚠️ +120 B
Overall change 2.18 MB 2.18 MB ⚠️ +1.92 kB
Diff details
Diff for edge-ssr.js

Diff too large to display

Diff for main-HASH.js

Diff too large to display

Diff for server.runtime.prod.js

Diff too large to display

Commit: 29a0485

@gnoff gnoff merged commit d6e7183 into vercel:canary Feb 3, 2025
109 checks passed
@gnoff gnoff deleted the use-cache-exclude-warmup branch February 3, 2025 20:14
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 18, 2025
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.

3 participants