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

Missing Postpone Detection Fix #59891

Merged
merged 1 commit into from
Jan 9, 2024
Merged

Missing Postpone Detection Fix #59891

merged 1 commit into from
Jan 9, 2024

Conversation

wyattjoh
Copy link
Member

@wyattjoh wyattjoh commented Dec 23, 2023

What?

Previously, if an error such as the client side rendering bailout (not to be confused with the static rendering bailout which PPR supercedes) occurs during render, and the postpone function was invoked during the original render, then the staticGenerationStore would incorrectly report that the render did call postpone, because the value is not reset on the render for the error page.

How?

Returning no error when the standard render is used and returning the error when the error render was used ensures that we don't warn about missing postpone data when a client side rendering bailout occurs.

Looking Ahead

A refactor of the AsyncLocalStorage should be done such that the stores are:

  1. Returned by the calling function so we aren't reaching into store properties at different parts
  2. Reorganizing the stores so that they're tied to the invocation lifetime, not the entire request lifetime, so that operations (like postpone) should only be available during renders that support postpone, not all renders during a request.

Closes NEXT-1927

@ijjk ijjk added created-by: Next.js team PRs by the Next.js team. type: next labels Dec 23, 2023
@wyattjoh
Copy link
Member Author

wyattjoh commented Dec 23, 2023

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 Dec 23, 2023

Tests Passed

@ijjk
Copy link
Member

ijjk commented Dec 23, 2023

Stats from current PR

Default Build
General Overall increase ⚠️
vercel/next.js canary vercel/next.js fix/error-postpone-bug Change
buildDuration 12.9s 12.9s N/A
buildDurationCached 7s 6s N/A
nodeModulesSize 199 MB 199 MB ⚠️ +16.7 kB
nextStartRea..uration (ms) 431ms 431ms
Client Bundles (main, webpack)
vercel/next.js canary vercel/next.js fix/error-postpone-bug Change
193.HASH.js gzip 181 B 182 B N/A
3f784ff6-HASH.js gzip 53.3 kB 53.3 kB N/A
433-HASH.js gzip 28.6 kB 28.6 kB N/A
framework-HASH.js gzip 45.2 kB 45.2 kB
main-app-HASH.js gzip 239 B 242 B N/A
main-HASH.js gzip 31.7 kB 31.8 kB N/A
webpack-HASH.js gzip 1.7 kB 1.7 kB N/A
Overall change 45.2 kB 45.2 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary vercel/next.js fix/error-postpone-bug 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/error-postpone-bug Change
_app-HASH.js gzip 194 B 195 B N/A
_error-HASH.js gzip 183 B 181 B N/A
amp-HASH.js gzip 504 B 502 B N/A
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 253 B N/A
head-HASH.js gzip 350 B 349 B N/A
hooks-HASH.js gzip 369 B 369 B
image-HASH.js gzip 4.28 kB 4.28 kB N/A
index-HASH.js gzip 255 B 256 B N/A
link-HASH.js gzip 2.61 kB 2.61 kB
routerDirect..HASH.js gzip 312 B 311 B N/A
script-HASH.js gzip 385 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.4 kB 3.4 kB
Client Build Manifests
vercel/next.js canary vercel/next.js fix/error-postpone-bug 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/error-postpone-bug Change
index.html gzip 527 B 527 B
link.html gzip 541 B 539 B N/A
withRouter.html gzip 523 B 522 B N/A
Overall change 527 B 527 B
Edge SSR bundle Size
vercel/next.js canary vercel/next.js fix/error-postpone-bug Change
edge-ssr.js gzip 93.8 kB 93.8 kB N/A
page.js gzip 148 kB 148 kB N/A
Overall change 0 B 0 B
Middleware size
vercel/next.js canary vercel/next.js fix/error-postpone-bug Change
middleware-b..fest.js gzip 623 B 625 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 vercel/next.js fix/error-postpone-bug Change
app-page-exp...dev.js gzip 169 kB 169 kB N/A
app-page-exp..prod.js gzip 95.1 kB 95.2 kB N/A
app-page-tur..prod.js gzip 95.8 kB 95.8 kB N/A
app-page-tur..prod.js gzip 90.4 kB 90.4 kB N/A
app-page.run...dev.js gzip 142 kB 142 kB N/A
app-page.run..prod.js gzip 89.7 kB 89.8 kB N/A
app-route-ex...dev.js gzip 24.1 kB 24.1 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.5 kB 23.5 kB
app-route.ru..prod.js gzip 16.3 kB 16.3 kB
pages-api-tu..prod.js gzip 9.38 kB 9.38 kB
pages-api.ru...dev.js gzip 9.65 kB 9.65 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.5 kB 22.5 kB
pages.runtim..prod.js gzip 21.9 kB 21.9 kB
server.runti..prod.js gzip 49.5 kB 49.5 kB
Overall change 258 kB 258 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

Commit: fe54872

@wyattjoh wyattjoh force-pushed the fix/error-postpone-bug branch 2 times, most recently from 24e4b3f to 119cf4d Compare December 23, 2023 01:05
Comment on lines 69 to 86

it('should fail the build & surface any errors that were thrown by user code', async () => {
// in the case of catching a postpone and throwing a new error, we log the error that the user threw to help with debugging
expect(stderr).toContain(
'Prerendering /no-suspense-boundary-re-throwing-error needs to partially bail out because something dynamic was used. '
)
expect(stderr).toContain(
'The following error was thrown during build, and may help identify the source of the issue:'
)
expect(stderr).toContain(
"Error: Throwing a new error from 'no-suspense-boundary-re-throwing-error'"
)

// the regular pre-render error should not be thrown as well, as we've already logged a more specific error
expect(stderr).not.toContain(
'Error occurred prerendering page "/no-suspense-boundary-re-throwing-error"'
)
})
Copy link
Member Author

Choose a reason for hiding this comment

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

This is technically incorrect, as this is race dependent. The error that reaches the top level first should be the one that the user has to manage, not the deeper error. Once the user had fixed this specific error, they would instead see the error about the missing postpone data which could be handled.

Copy link
Member

@ztanner ztanner Jan 3, 2024

Choose a reason for hiding this comment

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

The only bummer here is that given this code:

try {
  cookies();
} catch (err) {
  throw new Error(
    "Throwing a new error from 'no-suspense-boundary-re-throwing-error'",
  );
}

The error will now be:

Error occurred prerendering page "/no-suspense-boundary-re-throwing-error". Read more: https://nextjs.org/docs/messages/prerender-error
Error: Throwing a new error from 'no-suspense-boundary-re-throwing-error'

Which seems confusing, since as a user I wouldn't expect cookies() to throw and might lead me down the wrong path looking at other unrelated parts of my code. This is why previously it would say:

⨯ Prerendering /no-suspense-boundary-re-throwing-error needs to partially bail out because something dynamic was used. React throws a special object to indicate where we need to bail out but it was caught by a try/catch or a Promise was not awaited. These special objects should not be caught by your own try/catch. Learn more: https://nextjs.org/docs/messages/ppr-caught-error
 ⚠ The following error was thrown during build, and may help identify the source of the issue:
 ⨯ Error: Throwing a new error from 'no-suspense-boundary-re-throwing-error'

It seems like we still should handle this case somehow, but perhaps out of scope for this PR. 🤔

@wyattjoh wyattjoh marked this pull request as ready for review December 23, 2023 02:48
@wyattjoh wyattjoh force-pushed the fix/error-postpone-bug branch 2 times, most recently from 469c4ab to 2fe805a Compare January 3, 2024 20:16
@bernatfortet
Copy link

Hi! I'm commenting here because I don't know where else to find information about this issue.

I'm getting one of the errors above on next@canary 41. Below the error, and the page in question.

 ⚠ Entire page /lexile/[lexile_range] deopted into client-side rendering. https://nextjs.org/docs/messages/deopted-into-client-rendering /lexile/[lexile_range]

 ⨯ Prerendering /lexile/201-300 needs to partially bail out because something dynamic was used. React throws a special object to indicate where we need to bail out but it was caught by a try/catch or a Promise was not awaited. These special objects should not be caught by your own try/catch. Learn more: https://nextjs.org/docs/messages/ppr-caught-error
 ⨯ rD [Error]: Missing Postpone Data Error: An unexpected error occurred while prerendering /lexile/201-300. Please check the logs above for more details.
    at rz (/var/task/node_modules/next/dist/compiled/next-server/app-page-experimental.runtime.prod.js:16:25973)
    at runNextTicks (node:internal/process/task_queues:60:5)
    at listOnTimeout (node:internal/timers:538:9)
    at process.processTimers (node:internal/timers:512:7)
    at async Y (/var/task/node_modules/next/dist/compiled/next-server/server.runtime.prod.js:16:25400)
    at async Q.responseCache.get.routeKind (/var/task/node_modules/next/dist/compiled/next-server/server.runtime.prod.js:17:1025)
    at async /var/task/node_modules/next/dist/compiled/next-server/server.runtime.prod.js:17:13670
    at async /var/task/node_modules/next/dist/compiled/next-server/server.runtime.prod.js:17:12271 {
  digest: 'MISSING_POSTPONE_DATA_ERROR',
  page: '/lexile/201-300'
}
export default async function Page(props: Props) {
  const {} = props

  let lexileRange = props.params.lexile_range
  const olClient = new OLClient()
  const res = await olClient.searchBooks(`subject:lexile_range:${lexileRange}`)
  const books = res.data.books

  return (
    <PageContent>
      <PageHeader>
        <PageTitle>Lexile: {lexileRange}</PageTitle>
        <MoreButton lexileRange={lexileRange} />
      </PageHeader>

      <BooksList books={books} />
    </PageContent>
  )
}

@wyattjoh wyattjoh force-pushed the fix/error-postpone-bug branch 2 times, most recently from 9f7c091 to 2c94d55 Compare January 8, 2024 19:52
@wyattjoh wyattjoh merged commit 7018a65 into canary Jan 9, 2024
70 checks passed
@wyattjoh wyattjoh deleted the fix/error-postpone-bug branch January 9, 2024 20:03
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 24, 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.

4 participants