Feature/hydrate promises #7481
Feature/hydrate promises #7481
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 70f7f71. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 70f7f71:
|
this could use some reconciliation
github-actions
bot
added
the
documentation
packages/query-core/src/hydration.ts
Outdated
| @@ -65,6 +66,7 @@ function dehydrateQuery(query: Query): DehydratedQuery { | |||
| state: query.state, | |||
| queryKey: query.queryKey, | |||
| queryHash: query.queryHash, | |||
| ...(query.state.status === 'pending' && { promise: query.promise }), | |||
There was a problem hiding this comment.
I'd recommend to "hide" the error here to prevent leaking server secrets.
| ...(query.state.status === 'pending' && { promise: query.promise }), | |
| ...(query.state.status === 'pending' && { promise: query.promise.catch(() => { throw new Error("redacted") }) }), |
You might also want to consider different behaviour on rehydration of errors, depending of environment. I'd probably throw an error in SSR so the suspense boundary would just retry in the browser - and in the browser, I wouldn't throw at all, but immediately retry the query if it failed in RSC.
There was a problem hiding this comment.
thanks. How does this look?
There was a problem hiding this comment.
I think in Next.js (or maybe React does that), the error message and stack are already hidden when crossing the RSC boundary. This is also the case for Server Actions.
However, there will be a special digest field attached to the error which is a random ID. So developers can log the actual error on the server and use that ID from client logs to match it, without leaking anything.
There was a problem hiding this comment.
if react does this, it means we could remove this error handling. if it's next specific, I don't think we can rely on it because other frameworks like waku might implement this differently.
So can we figure out if this is next or react who does this ?
There was a problem hiding this comment.
I thought that was only for errors thrown during render, not for transported values, but I might be wrong - double-checking might definitely be a good idea here.
There was a problem hiding this comment.
Either way, this part is in the query core, so might make sense to redact it by default if entirely other frameworks handle it differently? Would be nice to support the digest too though. 🤔
Easier to go from redacted -> more details in the future than the other way around so I think current version is fine to ship.
There was a problem hiding this comment.
true, I haven't even thought about other frameworks 👍
There was a problem hiding this comment.
@TkDodo @Ephem I confirmed that it's a React behavior (which makes sense, as it's done on the RSC encoding/decoding layer that frameworks can't inject into): https://github.com/facebook/react/blob/ea6e05912aa43a0bbfbee381752caa1817a41a86/packages/react-client/src/ReactFlightClient.js#L1477-L1484
If we redact it here, the client code (e.g. error boundaries, alert boxes, trackers) can't receive that digest value and get it displayed/logged.
because we test against multiple versions of next
There was a problem hiding this comment.
I think this looks great! I left mostly NITs.
The one thing I'm a bit unsure of, but don't know if it affects this PR, is exactly under which circumstances NextJs makes the shell static. In the current example, I think the shell is supposed to be static with or without await because the fetch is cached by default?
When I add { cache: "no-store" } to that fetch, it turns the page dynamic both with and without await. This seems to happen in the swr example too though, so not sure what the expected behaviour is.
| @@ -1,4 +1,5 @@ | |||
| import { describe, expect, test, vi } from 'vitest' | |||
| import { waitFor } from '@testing-library/react' | |||
…ady have a cached entry.
Co-authored-by: Fredrik Höglund <fredrik.hoglund@gmail.com>
Co-authored-by: Fredrik Höglund <fredrik.hoglund@gmail.com>
|
This is awesome, thank you! |
No description provided.