-
Notifications
You must be signed in to change notification settings - Fork 3
Fix hydration error, remove prefetch helper #2697
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
Fix hydration error, remove prefetch helper #2697
Conversation
frontends/api/src/ssr/prefetch.ts
Outdated
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.
The prefetch helper avoided a small amount of boilerplate—namely, creating the query client. However, removing the helper:
- doesn't add much boilerplate
- Makes types simpler (proper typechecking for prefetch was hard)
- Means we use vanilla tanstack/react-query methods, matching their docs more easily, and makes it easier to do things like
fetchQuery(instead of prefetch) which allows server-side usage of response data. - lets us co-locate the query client creation
| * The exact value here isn't important, as long as it's bigger than | ||
| * request duration. | ||
| */ | ||
| staleTime: 15 * 60 * 1000, |
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.
The root cause of this hydration error was a lack of staleTime.
Prior to #2596:
getPrefetchQueryClientgenerated query client used byprefetch(...)- this did not have staletime set
getQueryClientgenerated query client during server render or browser render- the configuration differed slightly between server and client, but not in terms of staletime. Both had staletime set.
After #2596, (1) was used exclusively on the sever. Since staleTime wasn't set, it defaulted to 0. This meant that isFetching would be true during the server's render pass.
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.
Since staleTime wasn't set, it defaulted to 0. This meant that isFetching would be true during the server's render pass.
Here's an excerpt from some logging I did locally while working on this. Below, during the server render, the data was about 20ms old, which caused it to be stale (on server, where staleTime was 0).
Prefetched query at 1762973815075
[ 'offerors', 'list' ]
Prefetched query at 1762973815077
[ 'learning_resources', 'search' ]
Rendering SearchDisplay at 1762973815097
Returning SearchDisplay at 1762973815098
{ isFetching: true, isLoading: false, dataUpdatedAt: 1762973815077 }
GET /search 200 in 418ms
My understanding is:
- prefetch finished at T1 = 1762973815077
- SearchDisplay starts rendering at T2 = 1762973815097 (= basically when the query data is accessed)
- This "access query" happens using whatever QueryClient is given to our providers
- data age = T2 - T1 = 20 (milliseconds)
- data age exceeds staleTime, so queryClient refetches even though it's first render.
- Aside: there is a separate refetchOnMount option... I think that if refetchOnMount is false, it will NOT refetch on first render, even if data age exceeds stale time
- the refetch is mostly irrelevant, since the server only renders once anyway, and all that stuff gets GC'd between requests. But... it does affect isFetching , which we used on the search page
gumaerc
left a comment
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.
This fixed the errors for me. I just have a few questions / suggestions:
| */ | ||
| retry: (failureCount, error) => { | ||
| const axiosError = error as AxiosError | ||
| console.info("Retrying failed request", { |
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.
Are we sure we want to log this?
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.
not entirely sure, but we are on main, so won't change it here.
| * The exact value here isn't important, as long as it's bigger than | ||
| * request duration. | ||
| */ | ||
| staleTime: 15 * 60 * 1000, |
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.
Is it worth maybe extracting staleTime to be configurable via env var? 15 minutes seems pretty long and we might want to tweak this in production.
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.
We could, I don't think we need to now.
IMO, 15 min is pretty short for us. The data changes on scale of 12-24 hours via ETL; other data change (like userlists) might change more quickly, but mutations invalidate things.
| defaultOptions: { | ||
| queries: { | ||
| /** | ||
| * We create a new query client per request, but still need a staletime |
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.
nit: staleTime
OpenAPI ChangesShow/hide No detectable change.Unexpected changes? Ensure your branch is up-to-date with |
0dd8419 to
b70db3a
Compare
What are the relevant tickets?
https://github.com/mitodl/hq/issues/9253
Description (What does it do?)
This PR:
prefetchhelper. This is a change @jonkafton had previously discussed, and doing it allowed me to colocate the query clients, so I did it here.How can this be tested?