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

Navigation Signals in PPR #60450

Merged
merged 6 commits into from
Feb 6, 2024
Merged

Navigation Signals in PPR #60450

merged 6 commits into from
Feb 6, 2024

Conversation

wyattjoh
Copy link
Member

@wyattjoh wyattjoh commented Jan 10, 2024

What

This adds support for navigation signals like notFound() and redirect(url) when Partial Prerendering has been enabled.

Why

Navigation API's like notFound() and redirect(url) throw errors in order to interrupt the rendering of components. When a page both invokes API's that cause the render to be marked as dynamic (like unstable_noStore()) and also a navigation API, these errors may race to the end. In the case where the navigation error does not beat out the error emitted by dynamic API's will still trigger the detection warning that's present to warn you about situations where you may have accidentally caught the error.

How

This resolves this issue by explicitly checking for navigation signals (errors) thrown during the render, and not displaying the "caught dynamic API" error and console warning.

Closes NEXT-2037

@wyattjoh
Copy link
Member Author

wyattjoh commented Jan 10, 2024

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @wyattjoh and the rest of your teammates on Graphite Graphite

@ijjk
Copy link
Member

ijjk commented Jan 10, 2024

Stats from current PR

Default Build
General Overall increase ⚠️
vercel/next.js canary vercel/next.js fix/navigation-signals-ppr Change
buildDuration 11.9s 11.9s N/A
buildDurationCached 6.4s 5.2s N/A
nodeModulesSize 196 MB 196 MB ⚠️ +21 kB
nextStartRea..uration (ms) 406ms 411ms N/A
Client Bundles (main, webpack)
vercel/next.js canary vercel/next.js fix/navigation-signals-ppr Change
3f784ff6-HASH.js gzip 53.4 kB 53.4 kB
423.HASH.js gzip 185 B 181 B N/A
68-HASH.js gzip 29.7 kB 29.7 kB N/A
framework-HASH.js gzip 45.2 kB 45.2 kB
main-app-HASH.js gzip 238 B 242 B N/A
main-HASH.js gzip 31.8 kB 31.8 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 fix/navigation-signals-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 fix/navigation-signals-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 502 B 501 B N/A
css-HASH.js gzip 320 B 322 B N/A
dynamic-HASH.js gzip 2.5 kB 2.5 kB N/A
edge-ssr-HASH.js gzip 255 B 256 B N/A
head-HASH.js gzip 350 B 349 B N/A
hooks-HASH.js gzip 368 B 369 B N/A
image-HASH.js gzip 4.19 kB 4.18 kB N/A
index-HASH.js gzip 257 B 256 B N/A
link-HASH.js gzip 2.61 kB 2.61 kB N/A
routerDirect..HASH.js gzip 310 B 311 B N/A
script-HASH.js gzip 384 B 383 B N/A
withRouter-HASH.js gzip 306 B 308 B N/A
1afbb74e6ecf..834.css gzip 106 B 106 B
Overall change 106 B 106 B
Client Build Manifests
vercel/next.js canary vercel/next.js fix/navigation-signals-ppr Change
_buildManifest.js gzip 483 B 484 B N/A
Overall change 0 B 0 B
Rendered Page Sizes
vercel/next.js canary vercel/next.js fix/navigation-signals-ppr Change
index.html gzip 529 B 526 B N/A
link.html gzip 540 B 537 B N/A
withRouter.html gzip 524 B 521 B N/A
Overall change 0 B 0 B
Edge SSR bundle Size
vercel/next.js canary vercel/next.js fix/navigation-signals-ppr Change
edge-ssr.js gzip 94 kB 94 kB N/A
page.js gzip 149 kB 149 kB N/A
Overall change 0 B 0 B
Middleware size
vercel/next.js canary vercel/next.js fix/navigation-signals-ppr Change
middleware-b..fest.js gzip 620 B 624 B N/A
middleware-r..fest.js gzip 151 B 149 B N/A
middleware.js gzip 47.4 kB 47.4 kB N/A
edge-runtime..pack.js gzip 1.94 kB 1.94 kB
Overall change 1.94 kB 1.94 kB
Next Runtimes
vercel/next.js canary vercel/next.js fix/navigation-signals-ppr Change
app-page-exp...dev.js gzip 166 kB 166 kB N/A
app-page-exp..prod.js gzip 95.1 kB 95.1 kB N/A
app-page-tur..prod.js gzip 96.9 kB 96.9 kB N/A
app-page-tur..prod.js gzip 91.4 kB 91.5 kB N/A
app-page.run...dev.js gzip 135 kB 135 kB N/A
app-page.run..prod.js gzip 90 kB 90 kB N/A
app-route-ex...dev.js gzip 22 kB 22 kB
app-route-ex..prod.js gzip 14.8 kB 14.8 kB
app-route-tu..prod.js gzip 14.8 kB 14.8 kB
app-route-tu..prod.js gzip 14.6 kB 14.6 kB
app-route.ru...dev.js gzip 21.7 kB 21.7 kB
app-route.ru..prod.js gzip 14.6 kB 14.6 kB
pages-api-tu..prod.js gzip 9.43 kB 9.43 kB
pages-api.ru...dev.js gzip 9.7 kB 9.7 kB
pages-api.ru..prod.js gzip 9.43 kB 9.43 kB
pages-turbo...prod.js gzip 22 kB 22 kB
pages.runtim...dev.js gzip 22.7 kB 22.7 kB
pages.runtim..prod.js gzip 22 kB 22 kB
server.runti..prod.js gzip 49.7 kB 49.7 kB N/A
Overall change 198 kB 198 kB
Diff details
Diff for page.js

Diff too large to display

Diff for edge-ssr.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 server.runtime.prod.js

Diff too large to display

Commit: 893f564

@ijjk
Copy link
Member

ijjk commented Jan 10, 2024

Failing test suites

Commit: fda552c

pnpm test-dev test/e2e/app-dir/ppr-full/ppr-full.test.ts

  • ppr-full > Navigation Signals > notFound() > for /navigation/not-found > should have correct headers
  • ppr-full > Navigation Signals > notFound() > for /navigation/not-found/dynamic > should have correct headers
  • ppr-full > Navigation Signals > notFound() > for /navigation/not-found/dynamic > should cache the static part
  • ppr-full > Navigation Signals > redirect() > for /navigation/redirect > should have correct headers
  • ppr-full > Navigation Signals > redirect() > for /navigation/redirect/dynamic > should have correct headers
  • ppr-full > Navigation Signals > redirect() > for /navigation/redirect/dynamic > should cache the static part
Expand output

● ppr-full › Navigation Signals › notFound() › for /navigation/not-found › should have correct headers

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

Expected: "s-maxage=31536000, stale-while-revalidate"
Received: "no-store, must-revalidate"

  279 |               'text/html; charset=utf-8'
  280 |             )
> 281 |             expect(res.headers.get('cache-control')).toEqual(
      |                                                      ^
  282 |               's-maxage=31536000, stale-while-revalidate'
  283 |             )
  284 |

  at Object.toEqual (e2e/app-dir/ppr-full/ppr-full.test.ts:281:54)

● ppr-full › Navigation Signals › notFound() › for /navigation/not-found/dynamic › should have correct headers

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

Expected: "s-maxage=31536000, stale-while-revalidate"
Received: "no-store, must-revalidate"

  279 |               'text/html; charset=utf-8'
  280 |             )
> 281 |             expect(res.headers.get('cache-control')).toEqual(
      |                                                      ^
  282 |               's-maxage=31536000, stale-while-revalidate'
  283 |             )
  284 |

  at Object.toEqual (e2e/app-dir/ppr-full/ppr-full.test.ts:281:54)

● ppr-full › Navigation Signals › notFound() › for /navigation/not-found/dynamic › should cache the static part

expect(received).toBeLessThan(expected)

Expected: < 500
Received:   519

  309 |               // This is because the signal should throw and interrupt the
  310 |               // delay timer.
> 311 |               expect(result.streamEnd - start).toBeLessThan(delay)
      |                                                ^
  312 |             })
  313 |           }
  314 |         })

  at Object.toBeLessThan (e2e/app-dir/ppr-full/ppr-full.test.ts:311:48)

● ppr-full › Navigation Signals › redirect() › for /navigation/redirect › should have correct headers

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

Expected: "s-maxage=31536000, stale-while-revalidate"
Received: "no-store, must-revalidate"

  279 |               'text/html; charset=utf-8'
  280 |             )
> 281 |             expect(res.headers.get('cache-control')).toEqual(
      |                                                      ^
  282 |               's-maxage=31536000, stale-while-revalidate'
  283 |             )
  284 |

  at Object.toEqual (e2e/app-dir/ppr-full/ppr-full.test.ts:281:54)

● ppr-full › Navigation Signals › redirect() › for /navigation/redirect/dynamic › should have correct headers

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

Expected: "s-maxage=31536000, stale-while-revalidate"
Received: "no-store, must-revalidate"

  279 |               'text/html; charset=utf-8'
  280 |             )
> 281 |             expect(res.headers.get('cache-control')).toEqual(
      |                                                      ^
  282 |               's-maxage=31536000, stale-while-revalidate'
  283 |             )
  284 |

  at Object.toEqual (e2e/app-dir/ppr-full/ppr-full.test.ts:281:54)

● ppr-full › Navigation Signals › redirect() › for /navigation/redirect/dynamic › should cache the static part

expect(received).toBeLessThan(expected)

Expected: < 500
Received:   522

  309 |               // This is because the signal should throw and interrupt the
  310 |               // delay timer.
> 311 |               expect(result.streamEnd - start).toBeLessThan(delay)
      |                                                ^
  312 |             })
  313 |           }
  314 |         })

  at Object.toBeLessThan (e2e/app-dir/ppr-full/ppr-full.test.ts:311:48)

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

@ijjk
Copy link
Member

ijjk commented Jan 10, 2024

Tests Passed

@wyattjoh wyattjoh force-pushed the fix/navigation-signals-ppr branch 3 times, most recently from 7eba58a to 051d19c Compare January 10, 2024 21:26
@wyattjoh wyattjoh force-pushed the fix/navigation-signals-ppr branch 9 times, most recently from 4da8ee7 to 02297f0 Compare January 23, 2024 16:22
@wyattjoh wyattjoh force-pushed the fix/navigation-signals-ppr branch 5 times, most recently from b1a53eb to 0c8912c Compare February 1, 2024 20:26
@wyattjoh wyattjoh marked this pull request as ready for review February 1, 2024 22:13
test/ppr-tests-manifest.json Outdated Show resolved Hide resolved
packages/next/src/export/routes/app-page.ts Outdated Show resolved Hide resolved
packages/next/src/server/app-render/app-render.tsx Outdated Show resolved Hide resolved
…ht router state

When handling an interception route prefetch, the responses are always dynamic. To ensure
that we didn't polute the cache, we added a test to prevent the flight router state from
being used during static generation.
@wyattjoh wyattjoh merged commit 252d9cf into canary Feb 6, 2024
68 checks passed
@wyattjoh wyattjoh deleted the fix/navigation-signals-ppr branch February 6, 2024 19:25
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 21, 2024
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.

None yet

3 participants