-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
SSR docs rewrite #5966
SSR docs rewrite #5966
Conversation
Also includes initial scaffold for new prefetch/SSR docs structure
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 0c846b9. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this branch ⌛ The following target is in progress Sent 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 0c846b9:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome work so far @Ephem 👏
docs/react/guides/prefetching.md
Outdated
|
||
Before jumping into the different specific prefetch patterns, let's look at the `prefetchQuery` and `prefetchInfiniteQuery` functions. First a few basics: | ||
|
||
- If **fresh** data for this query is already in the cache, the data will not be fetched |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's technically true, but can be quite confusing. For this to work, React Query needs to know about "freshness" / "staleness", but that is a property of Observers, not the Query itself. If you call queryClient.prefetchQuery({ queryKey, queryFn })
without explicitly passing staleTime
, the default one will be taken. If you haven't set a global default staleTime
, the default of zero
will always trigger a new fetch with prefetchQuery
, even if there is data in the cache.
When we prefetch, we usually don't have an active Observer via useQuery
. So even if we set staleTime
on our useQuery
call, by the time we call prefetchQuery
, the default is still used. I think this is worth pointing out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent point! I copied this from the old docs and didn't think much about it, should be fixed now.
docs/react/guides/prefetching.md
Outdated
- If a `staleTime` is passed eg. `prefetchQuery({ queryKey: ['todos'], queryFn: fn, staleTime: 5000 })` and the data is older than the specified `staleTime`, the query will be fetched | ||
- (If you want to ignore `staleTime` and instead always return data if it's available in the cache, you can use the `ensureQueryData` function.) | ||
- If no instances of `useQuery` appear for a prefetched query, it will be deleted and garbage collected after the time specified in `gcTime` | ||
- These functions does not return the query data. If that's something you need, use `fetchQuery`/`fetchInfiniteQuery` instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- These functions does not return the query data. If that's something you need, use `fetchQuery`/`fetchInfiniteQuery` instead. | |
- These functions do not return the query data. If that's something you need, use `fetchQuery`/`fetchInfiniteQuery` instead. |
I would also point out that a Promise<void>
is returned so that it can be awaited, but it will never throw errors (so you don't need try/catch).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah! The error one has bit even me in the past, glad you pointed it out. Fixed.
docs/react/guides/prefetching.md
Outdated
const prefetch = useCallback(() => { | ||
queryClient.prefetchQuery({ | ||
queryKey: ['details'], | ||
queryFn: getDetailsData, | ||
// Prefetch only fires when data is older than the staleTime, | ||
// so in a case like this you definitely want to set one | ||
staleTime: 60000, | ||
}) | ||
}, [queryClient]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we skip the wrapping in useCallback
? Having it in the docs feels like its required to do it, but it's not. We pass that event handler to a built-in <button>
component, so it doesn't matter if it's a new function on every render.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, fixed!
docs/react/guides/prefetching.md
Outdated
// Prefetch | ||
useQuery({ | ||
queryKey: ['article-comments', id], | ||
queryFn: getArticleCommentsById, | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another optimization we could point out that even if we ignore the result of useQuery
, we still have an active observer that is subscribed to all changes of that query, so we will get a bunch of re-renders of this component even though it's not really necessary.
This is a technical limitation of how property tracking works: If you "use" no property during rendering, you "use" all of them because you could potentially access them in effects or event handlers.
To avoid this, we can pass notifyOnChangeProps: []
to this useQuery
:
// Prefetch
useQuery({
queryKey: ['article-comments', id],
queryFn: getArticleCommentsById,
notifyOnChangeProps: []
})
This might even be a valid enough use-case to have a usePrefetchQuery
hook that returns nothing 🤔 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one, added! Didn't know about that myself actually.
Yeah, I think there is a strong argument for adding that, it also encourages prefetching which is nice. If we add it, let's make sure it matches any fetch/read API for the suspense side of things. We could probably have just one common prefetch hook for both.
Phew, I need to re-read this during the weekend, but it should be ready for review now. It might not be perfect, but I hope it's better than the current docs at least. I'd love high level feedback as well. Are the pages too long/verbose/tries to cover too much? Are there sections we could skip or are there sections we should add? Also, I've made a few "official recommendations" in there, do you agree with them? I am biased towards prefetching, does that shine through too much? What about the naming of the guides, specifically, should we call the "Advanced Server Rendering" guide "Server Components & Streaming" instead? Finally, this messes up the Vue docs for prefetching since those are just copied from the React docs, and there are now React specific things in there. I wont have time to fix this properly right now, do you want me to just copy the old doc over into the Vue docs folder in this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wonderfully written ❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super valuable feedback @TkDodo, thanks so much! I'll tackle it right away.
I think I managed to fix all your feedback @TkDodo, I resolved the minor stuff directly but left the comments about clarifications/reformulations open for you to look at. 😄 |
Most of this content is applicable in v4 as well, except for naming of
useSuspenseQuery
etc, but I've written and targeted this to the v5 branch.There are four guides that progressively builds knowledge:
I'm open to suggestions on naming for the pages.
There are two existing doc pages that are related to these:
I'm not sure what to do with these. While there is some duplicate content there, they are also short and practical so I think I'm leaning towards keeping them around for discoverability. It's also good that these are mentioned early/high up in the guide pages navigation, while these 4 guides are more in depth and makes sense to have further down in the navigation.
I removed some content related to setting up React Query with custom SSR solutions. I don't think that's relevant to a majority of people, and the step by step instructions are still there so people can figure it out. If we want this I think it would fit better as an example, but I wont include that in this PR. Other than that, I don't think I removed any information, just added, rewrote or restructured.