vercel / next.js

The React Framework
https://nextjs.org
MIT License
124.7k stars 26.62k forks source link

Pending Promises sent from server to client component crash if they reject without a reason #67863

Open edspencer opened 1 month ago

edspencer commented 1 month ago

Link to the code that reproduces this issue

https://codesandbox.io/p/devbox/still-cherry-8wks5w

To Reproduce

  1. Load the codesandbox (enable VS Code web)
  2. Load the preview pane
  3. See the "Loading..." Suspense fallback never disappears
  4. See the TypeError: Cannot read properties of undefined (reading 'digest') error in the terminal

It boils down to a code block like this:

<ErrorBoundary fallback={<ErrorFallback />}>
  <Suspense fallback={<p>Loading...</p>}>
    <Table dataPromise={promiseThatWillReject} />
  </Suspense>
</ErrorBoundary>

If that promiseThatWillReject calls its reject() function without any argument, the bug is encountered.

Current vs. Expected behavior

Rejecting a promise without a rejection reason causes the Next response to crash with an error inside create-error-handler.tsx. This causes the page response to never finish. The error does not bubble up to the ErrorBoundary, and the page just breaks.

I expect the rejected promise to trigger my ErrorBoundary, or at least give me some kind of message telling me I need to provide an argument to my reject() call.

Provide environment information

Operating System:
  Platform: linux
  Arch: x64
  Version: #1 SMP PREEMPT_DYNAMIC Sun Aug  6 20:05:33 UTC 2023
  Available memory (MB): 4102
  Available CPU cores: 2
Binaries:
  Node: 20.9.0
  npm: 9.8.1
  Yarn: 1.22.19
  pnpm: 8.10.2
Relevant Packages:
  next: 15.0.0-canary.70 // Latest available version is detected (15.0.0-canary.70).
  eslint-config-next: N/A
  react: 19.0.0-rc.0
  react-dom: 19.0.0-rc.0
  typescript: 5.3.3
Next.js Config:
  output: N/A

Which area(s) are affected? (Select all that apply)

Not sure, Developer Experience

Which stage(s) are affected? (Select all that apply)

next dev (local), next build (local)

Additional context

This appears to be because create-error-handler.tsx expects its err: any argument to be present, but when you reject() a promise without an argument, this does not happen (err is undefined).

To debug, I cloned nextjs, copied my app inside of it as per the debugging instructions and debugged with VS Code. The issue starts inside of react-server-dom-webpack-server.edge.development.js, which passes the (in this case undefined) reason to logRecoverableError. I guess this part is React code, rather than Next code. That part seems reasonable, as does the next call which calls requestStorage.run(undefined, onError, error) (the error here is undefined).

The error is caused when create-error-handler.tsx attempts to read err.digest when err is undefined. This issue is occurring at the intersection of React and Next, but it looks like it should be addressed on the Next side, specifically by not assuming that err is defined. Perhaps just defaulting err to a new Error() inside create-error-handler.tsx would be a reasonable solution.

The terminal error looks like this:

TypeError: Cannot read properties of undefined (reading 'digest')
    at /Users/ed/Code/rsc-examples/node_modules/next/dist/compiled/next-server/app-page.runtime.dev.js:36:17750
    at AsyncLocalStorage.run (node:async_hooks:346:14)
    at eL (/Users/ed/Code/rsc-examples/node_modules/next/dist/compiled/next-server/app-page.runtime.dev.js:35:273717)
    at /Users/ed/Code/rsc-examples/node_modules/next/dist/compiled/next-server/app-page.runtime.dev.js:35:269545
edspencer commented 1 month ago

Slightly more context: it looks like calling reject() without a reason is supported when the promise is on the client side - see https://react.dev/reference/react/use#dealing-with-rejected-promises

I'm guessing that this may be something to do with how Flight deals with promise resolution.

little077 commented 1 month ago

.catch(e=>{ return "no new message found."; });Maybe you need to throw a catch to catch him

edspencer commented 1 month ago

.catch(e=>{ return "no new message found."; });Maybe you need to throw a catch to catch him

Yes, you can write that code, but it's non-obvious that you need to, and if the Promise that rejects is not in your own codebase then it may be difficult/impossible to do that. Also, it's not going to trigger your error boundary, so now you have 2 code paths to handle errors.

I think this is a bug as it doesn't seem reasonable that rejecting a promise without an argument (which is a valid use of reject()) should crash the process and halt the response. There are workarounds - the catch you mention and also passing an Error instance to reject() (you can even just call reject({digest: 'something'}) as it's the .digest property the code is looking for), but I lost of few hours of my life figuring this out and I can see it affecting others too.