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

Fix revalidateTag() behaviour when invoked in server components (#70446) #70642

Merged
merged 4 commits into from
Sep 30, 2024

Conversation

ijjk
Copy link
Member

@ijjk ijjk commented Sep 30, 2024

This backports #70446 to 14-2-1

Fixes #70403

What?

When revalidateTag() is called directly in server components, the cache is not purged for the corresponding tags. Reproduction steps available in #70403

How?

Check
app-render.tsx This issue was introduced in #65296 in this file:
revalidate.ts. The lines removed from the file resulted in the revalidation checks to be skipped when there is an RSC request.

Also fixed checks on pendingRevalidates to also check for revalidatedTags.

…rcel#70446)

Fixes vercel#70403

### What?
When `revalidateTag()` is called directly in server components, the
cache is not purged for the corresponding tags. Reproduction steps
available in vercel#70403

### How?
Check
[app-render.tsx](https://github.com/vercel/next.js/compare/canary...abhi12299:fix-revalidatetag-rsc?expand=1#diff-a3e2e024db1faa1b501e0dd6040eaaf0d931cb9878ae0fb0f4c3658daa982768)
This issue was introduced in vercel#65296 in this file:
[revalidate.ts](https://github.com/vercel/next.js/pull/65296/files#diff-7f0cb5bb30d44b9153d724e31c25859b9aab6cc258b35563a1d9464cd0688283).
The lines removed from the file resulted in the revalidation checks to
be skipped when there is an RSC request.

Also fixed checks on `pendingRevalidates` to also check for
`revalidatedTags`.

---------

Co-authored-by: JJ Kasper <jj@jjsweb.site>
# Conflicts:
#	packages/next/src/server/app-render/app-render.tsx
@ijjk
Copy link
Member Author

ijjk commented Sep 30, 2024

Stats from current PR

Default Build (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js 14-2-1 ijjk/next.js ijjk/backport-revalidate-tag Change
buildDuration 18.7s 17.3s N/A
buildDurationCached 9.3s 8.3s N/A
nodeModulesSize 200 MB 200 MB ⚠️ +27.2 kB
nextStartRea..uration (ms) 438ms 446ms N/A
Client Bundles (main, webpack)
vercel/next.js 14-2-1 ijjk/next.js ijjk/backport-revalidate-tag Change
1103-HASH.js gzip 31.6 kB 31.6 kB N/A
1a9f679d-HASH.js gzip 53.7 kB 53.7 kB N/A
335-HASH.js gzip 5.05 kB 5.05 kB
7953.HASH.js gzip 181 B 181 B
framework-HASH.js gzip 44.9 kB 44.9 kB
main-app-HASH.js gzip 243 B 243 B
main-HASH.js gzip 32.3 kB 32.3 kB N/A
webpack-HASH.js gzip 1.68 kB 1.68 kB N/A
Overall change 50.3 kB 50.3 kB
Legacy Client Bundles (polyfills)
vercel/next.js 14-2-1 ijjk/next.js ijjk/backport-revalidate-tag Change
polyfills-HASH.js gzip 39.4 kB 39.4 kB
Overall change 39.4 kB 39.4 kB
Client Pages
vercel/next.js 14-2-1 ijjk/next.js ijjk/backport-revalidate-tag Change
_app-HASH.js gzip 195 B 197 B N/A
_error-HASH.js gzip 184 B 184 B
amp-HASH.js gzip 501 B 506 B N/A
css-HASH.js gzip 323 B 324 B N/A
dynamic-HASH.js gzip 1.82 kB 1.82 kB N/A
edge-ssr-HASH.js gzip 259 B 258 B N/A
head-HASH.js gzip 350 B 352 B N/A
hooks-HASH.js gzip 372 B 371 B N/A
image-HASH.js gzip 4.23 kB 4.23 kB
index-HASH.js gzip 259 B 258 B N/A
link-HASH.js gzip 2.68 kB 2.68 kB N/A
routerDirect..HASH.js gzip 316 B 315 B N/A
script-HASH.js gzip 387 B 387 B
withRouter-HASH.js gzip 311 B 310 B N/A
1afbb74e6ecf..834.css gzip 106 B 106 B
Overall change 4.9 kB 4.9 kB
Client Build Manifests
vercel/next.js 14-2-1 ijjk/next.js ijjk/backport-revalidate-tag Change
_buildManifest.js gzip 483 B 483 B
Overall change 483 B 483 B
Rendered Page Sizes
vercel/next.js 14-2-1 ijjk/next.js ijjk/backport-revalidate-tag Change
index.html gzip 528 B 529 B N/A
link.html gzip 542 B 541 B N/A
withRouter.html gzip 525 B 525 B
Overall change 525 B 525 B
Edge SSR bundle Size
vercel/next.js 14-2-1 ijjk/next.js ijjk/backport-revalidate-tag Change
edge-ssr.js gzip 95.4 kB 95.4 kB N/A
page.js gzip 3.06 kB 3.06 kB N/A
Overall change 0 B 0 B
Middleware size
vercel/next.js 14-2-1 ijjk/next.js ijjk/backport-revalidate-tag Change
middleware-b..fest.js gzip 659 B 659 B
middleware-r..fest.js gzip 156 B 156 B
middleware.js gzip 25.5 kB 25.5 kB N/A
edge-runtime..pack.js gzip 839 B 839 B
Overall change 1.65 kB 1.65 kB
Next Runtimes
vercel/next.js 14-2-1 ijjk/next.js ijjk/backport-revalidate-tag Change
app-page-exp...dev.js gzip 171 kB 171 kB N/A
app-page-exp..prod.js gzip 98.1 kB 98.2 kB N/A
app-page-tur..prod.js gzip 99.8 kB 99.9 kB N/A
app-page-tur..prod.js gzip 94.1 kB 94.2 kB N/A
app-page.run...dev.js gzip 145 kB 145 kB N/A
app-page.run..prod.js gzip 92.6 kB 92.7 kB N/A
app-route-ex...dev.js gzip 22 kB 22 kB
app-route-ex..prod.js gzip 15.5 kB 15.5 kB
app-route-tu..prod.js gzip 15.5 kB 15.5 kB
app-route-tu..prod.js gzip 15.2 kB 15.2 kB
app-route.ru...dev.js gzip 21.7 kB 21.7 kB
app-route.ru..prod.js gzip 15.2 kB 15.2 kB
pages-api-tu..prod.js gzip 9.58 kB 9.58 kB
pages-api.ru...dev.js gzip 9.85 kB 9.85 kB
pages-api.ru..prod.js gzip 9.57 kB 9.57 kB
pages-turbo...prod.js gzip 22.5 kB 22.5 kB
pages.runtim...dev.js gzip 23.2 kB 23.2 kB
pages.runtim..prod.js gzip 22.5 kB 22.5 kB
server.runti..prod.js gzip 51.5 kB 51.5 kB
Overall change 254 kB 254 kB
build cache Overall increase ⚠️
vercel/next.js 14-2-1 ijjk/next.js ijjk/backport-revalidate-tag Change
0.pack gzip 1.6 MB 1.6 MB ⚠️ +118 B
index.pack gzip 113 kB 113 kB N/A
Overall change 1.6 MB 1.6 MB ⚠️ +118 B
Diff details
Diff for edge-ssr.js

Diff too large to display

Diff for app-page-exp..ntime.dev.js
failed to diff
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

Commit: 6047f26

@ijjk ijjk mentioned this pull request Sep 30, 2024
@ijjk ijjk marked this pull request as ready for review September 30, 2024 21:35
@ijjk ijjk requested review from huozhi and ztanner September 30, 2024 21:45
@ijjk
Copy link
Member Author

ijjk commented Sep 30, 2024

Failing test suites

Commit: 6047f26

TURBOPACK=1 pnpm test-dev test/e2e/prerender.test.ts (turbopack)

  • Prerender > should log error in console and browser in development mode
Expand output

● Prerender › should log error in console and browser in development mode

page.waitForSelector: Timeout 60000ms exceeded.
Call log:
  - waiting for locator('#after-change')

  425 |     return this.chain(() => {
  426 |       return page
> 427 |         .waitForSelector(selector, { timeout, state: 'attached' })
      |          ^
  428 |         .then(async (el) => {
  429 |           // it seems selenium waits longer and tests rely on this behavior
  430 |           // so we wait for the load event fire before returning

  at waitForSelector (lib/browsers/playwright.ts:427:10)

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

@ijjk ijjk merged commit 0ffea65 into vercel:14-2-1 Sep 30, 2024
49 of 51 checks passed
@ijjk ijjk deleted the ijjk/backport-revalidate-tag branch September 30, 2024 23:56
zemnmez-renovate-bot added a commit to zemn-me/monorepo that referenced this pull request Oct 1, 2024
##### [`v14.2.14](https://github.com/vercel/next.js/releases/tag/v14.2.14)

> \[!NOTE]\
> This release is backporting bug fixes. It does **not** include all pending features/changes on canary.

##### Core Changes

-   Fix: clone response in first handler to prevent race ([#70082](vercel/next.js#70082)) ([#70649](vercel/next.js#70649))
-   Respect reexports from metadata API routes ([#70508](vercel/next.js#70508)) ([#70647](vercel/next.js#70647))
-   Externalize node binary modules for app router ([#70646](vercel/next.js#70646))
-   Fix revalidateTag() behaviour when invoked in server components ([#70446](vercel/next.js#70446)) ([#70642](vercel/next.js#70642))
-   Fix prefetch bailout detection for nested loading segments ([#70618](vercel/next.js#70618))
-   Add missing node modules to externals ([#70382](vercel/next.js#70382))
-   Feature: next/image: add support for images.remotePatterns.search ([#70302](vercel/next.js#70302))

##### Credits

Huge thanks to [@styfle](https://github.com/styfle), [@ztanner](https://github.com/ztanner), [@ijjk](https://github.com/ijjk), [@huozhi](https://github.com/huozhi) and [@wyattjoh](https://github.com/wyattjoh) for helping!
github-merge-queue bot pushed a commit to zemn-me/monorepo that referenced this pull request Oct 1, 2024
##### [`v14.2.14](https://github.com/vercel/next.js/releases/tag/v14.2.14)

> \[!NOTE]\
> This release is backporting bug fixes. It does **not** include all pending features/changes on canary.

##### Core Changes

-   Fix: clone response in first handler to prevent race ([#70082](vercel/next.js#70082)) ([#70649](vercel/next.js#70649))
-   Respect reexports from metadata API routes ([#70508](vercel/next.js#70508)) ([#70647](vercel/next.js#70647))
-   Externalize node binary modules for app router ([#70646](vercel/next.js#70646))
-   Fix revalidateTag() behaviour when invoked in server components ([#70446](vercel/next.js#70446)) ([#70642](vercel/next.js#70642))
-   Fix prefetch bailout detection for nested loading segments ([#70618](vercel/next.js#70618))
-   Add missing node modules to externals ([#70382](vercel/next.js#70382))
-   Feature: next/image: add support for images.remotePatterns.search ([#70302](vercel/next.js#70302))

##### Credits

Huge thanks to [@styfle](https://github.com/styfle), [@ztanner](https://github.com/ztanner), [@ijjk](https://github.com/ijjk), [@huozhi](https://github.com/huozhi) and [@wyattjoh](https://github.com/wyattjoh) for helping!
renovate bot added a commit to mmkal/eslint-plugin-mmkal that referenced this pull request Oct 1, 2024
##### [v14.2.14](https://github.com/vercel/next.js/releases/tag/v14.2.14)

> \[!NOTE]\
> This release is backporting bug fixes. It does **not** include all pending features/changes on canary.

##### Core Changes

-   Fix: clone response in first handler to prevent race ([#70082](vercel/next.js#70082)) ([#70649](vercel/next.js#70649))
-   Respect reexports from metadata API routes ([#70508](vercel/next.js#70508)) ([#70647](vercel/next.js#70647))
-   Externalize node binary modules for app router ([#70646](vercel/next.js#70646))
-   Fix revalidateTag() behaviour when invoked in server components ([#70446](vercel/next.js#70446)) ([#70642](vercel/next.js#70642))
-   Fix prefetch bailout detection for nested loading segments ([#70618](vercel/next.js#70618))
-   Add missing node modules to externals ([#70382](vercel/next.js#70382))
-   Feature: next/image: add support for images.remotePatterns.search ([#70302](vercel/next.js#70302))

##### Credits

Huge thanks to [@styfle](https://github.com/styfle), [@ztanner](https://github.com/ztanner), [@ijjk](https://github.com/ijjk), [@huozhi](https://github.com/huozhi) and [@wyattjoh](https://github.com/wyattjoh) for helping!
renovate bot added a commit to mmkal/eslint-plugin-mmkal that referenced this pull request Oct 1, 2024
##### [v14.2.14](https://github.com/vercel/next.js/releases/tag/v14.2.14)

> \[!NOTE]\
> This release is backporting bug fixes. It does **not** include all pending features/changes on canary.

##### Core Changes

-   Fix: clone response in first handler to prevent race ([#70082](vercel/next.js#70082)) ([#70649](vercel/next.js#70649))
-   Respect reexports from metadata API routes ([#70508](vercel/next.js#70508)) ([#70647](vercel/next.js#70647))
-   Externalize node binary modules for app router ([#70646](vercel/next.js#70646))
-   Fix revalidateTag() behaviour when invoked in server components ([#70446](vercel/next.js#70446)) ([#70642](vercel/next.js#70642))
-   Fix prefetch bailout detection for nested loading segments ([#70618](vercel/next.js#70618))
-   Add missing node modules to externals ([#70382](vercel/next.js#70382))
-   Feature: next/image: add support for images.remotePatterns.search ([#70302](vercel/next.js#70302))

##### Credits

Huge thanks to [@styfle](https://github.com/styfle), [@ztanner](https://github.com/ztanner), [@ijjk](https://github.com/ijjk), [@huozhi](https://github.com/huozhi) and [@wyattjoh](https://github.com/wyattjoh) for helping!
@sebmarkbage
Copy link
Contributor

This should actually throw and not revalidate because it's a side-effect in render. cookies().set() too.

Other than just breaking The Rules of React in general, and being bad practice since there's no guarantee when it's called - if it's called - how many times it is called. It's also not possible to send invalidations to the client at this point so if you do it for example after render a Server Action, the client won't be invalidated as a result.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants