-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 & rehydration support, suspense foundations #1277
Conversation
✔️ Deploy Preview for redux-starter-kit-docs ready! 🔨 Explore the source changes: 262094e 🔍 Inspect the deploy log: https://app.netlify.com/sites/redux-starter-kit-docs/deploys/6144da47d393230008fd6248 😎 Browse the preview: https://deploy-preview-1277--redux-starter-kit-docs.netlify.app |
size-limit report 📦
|
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 ea86a3f:
|
{ | ||
arg, | ||
requestId, | ||
const queryAction: StartQueryActionCreator<any> = |
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.
@@ -502,7 +523,7 @@ export function buildHooks<Definitions extends EndpointDefinitions>({ | |||
|
|||
const promiseRef = useRef<QueryActionCreatorResult<any>>() | |||
|
|||
useEffect(() => { | |||
usePossiblyImmediateEffect((): void | undefined => { |
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 I'm not that familiar with RTK Query I'm just skimming this PR from my (SSR) perspective. I'm not sure how subscriptions work here (and don't have time to dig) so I'll go ahead and ask some questions. 😄
I'm not sure about converting this entire effect to be immediate. Specifically I notice that there is an effect cleanup calling promiseRef.current?.unsubscribe()
on line 559. This is never called on the server, will that cause problems? (You mentioned hydration issues related to this in a Twitter-thread, is there anything else that might be problematic like memory leaks on the server?) I'm guessing dispatch(initiate(...))
is what sets up that subscription? Is there a way to start a fetch and get a promise back without setting up a subscription and can that be used instead with sideEffectsInRender
?
About the Suspense parts, who knows what that will look like when Suspense for data fetching is finished, but at a glance it looks like this could work in the current versions.
(Aside: I also noticed this effect will be called in every render in Suspense mode, but looks like it's basically a noop if nothing has changed so maybe that is fine? Might be unexpected and brittle if adding things to usePossiblyImmediateEffect
s in the future though?)
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.
Having those subscriptions added in SSR might actually be helpful.
I'm thinking of a "hydrate" action (or other helper) that first restores the state from the server and then unsubscribes all subscriptions that were part of that "initial data".
That would start all those "the last component unsubscribed" timers for cache cleanup while components could "re-claim" those cache entries themselves by subscribing to them.
In reality though, that might be quite a lot of actions flying around at once, so I'll have to experiment around with this a bit.
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.
As for the usePossiblyImmediateEffects
being potentially brittle: I think we'll need to do something during render in every possible suspense scenario I can think of, so there is probably no way around that. I've just stuck this to the useEffects
where it's necessary (also those without a cleanup) and hope that we can get away with it, at least until other patterns surface. It's also a marker that we have to handle these effects with extra care.
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.
Memory leaks on the server should probably not be a problem, if the Redux store is disposed of correctly. (If nothing unsubscribes, there will not even be any timers around for cache collection.)
It might be a good practice to clean up thoroughly in a SSR scenario by doing a store.dispatch(api.util.resetApiState())
after the render is complete though, to make double sure really no timeouts etc. are sticking around.
We should definitely add that in a SSR documentation.
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 would start all those "the last component unsubscribed" timers for cache cleanup
Since I'm not familiar with what composes "a subscription" in RTK Query terms I'm just asking naïve questions here, but couldn't this be done for all queries in the store at hydration anyway? Does there need to be a previous subscription for this to work? (I get why that might make implementation easier though)
The reason I'm asking is that since components only render once in the same render on the server, subscriptions are usually a funky mental model conceptually there and most libraries avoid them for that reason to avoid having to deal with cleaning them up (something that often leaks into userland). To me it would seem unexpected if I inspected the store state after rendering on the server and I saw component subscriptions still lingering around. Doesn't mean avoiding subscriptions on the server is necessarily the right call here of course, just trying to provide context. 😄
It's also a marker that we have to handle these effects with extra care
I think the naming is great and doing things in render is just how Suspense works so that part is great, the potentially unexpected part for me was just that the dependency array had no meaning in some cases. One should always code so effects works without the dep array anyway, so that's no big deal.
Memory leaks on the server should probably not be a problem
Great! Just wanted to mention it as something to look out for.
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.
A "subscription" in this case is more like a "reference counter". It's the information "hey, there is a component out there, displaying that data, please don't remove it from the cache". (There can be more information attached like "this component requests for the data to be polled", but I feel that's not of major importance here). Unsubscribed data can be thrown away in certain circumstances or simply not re-queried if something would otherwise trigger invalidation/re-query of said data. So, it is useful to have that subscription.
Other than that, you might be reading more into the word that it actually means from a component perspective 🙂
I agree that we could do stuff like "never subscribe in the first place", but if we go this route of restoring a "live cache snapshot", it would also enable users of local storage persistence to rehydrate.
While I personally think that most times, cache data shouldn't be persisted and if you want to have that kind of cache, just set the right HTTP headers and let the browser handle it, I'm sure there are some edge cases around where data has to be persisted - and having a rehydration tool around for that would also be handy.
It might also come down to removing subscriptions before restoring data, if it is contained in the snapshot. I guess I'll explore many options there. That is all a bit muddy before I get to it 😅
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.
Ah, I was starting to suspect that a "subscription" in RTK Query was a way to track subscriptions/dependents rather than actually subscribing since that seems like something you would do through regular Redux means. In React Query there's the concept of "observers" which seems very similar. This does make things fall into place, thanks!
In fact, I'm pretty sure we've at some point had a bug in React Query where data fetched by prefetch was in some cases prematurely thrown away because it had no observers or something like that, so providing a way of fetching data without counting that as an "observer" also has tradeoffs.
I guess a different way to put it is, when you support imperative fetching with cache eviction, you either:
- Register that as an "dependent" and require the developer to later "unregister" that manually (the hydration tool you talk about)
- Don't register a "dependent", but now the data is immediately stale/scheduled for cache eviction, or you place it in the cache for infinite time, or you need to find some other way to deal with it
I guess it boils down to supporting imperative fetching in libraries where "query lifetime" is a central concept is kind of a mismatch that requires working around one way or the other. 😄
effect: () => void | undefined, | ||
deps?: DependencyList | ||
) => void = | ||
unstable__sideEffectsInRender || unstable__suspense |
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 the separate unstable__sideEffectsInRender
needed here, maybe it could just be "if you want to fetch inside of components on the server, you use Suspense"?
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.
I wanted to enable a "fetch in render, but do not throw" use case here, so I'd prefer to keep them separated.
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.
Ah, I get it, I missed that this wasn't throwing! As I mentioned in the issue I'm wary of that use case personally and don't see any benefit over using the Suspense paradigm, but that's up to you of course and this is clearly marked as unstable. 😄
So there is kind of a demo over at https://github.com/phryneas/ssr-experiments/blob/main/nextjs-blog/pages/pokemon/ssg/%5Bname%5D.tsx Quite hacky still (especially the hydration), but it works for SSG with next. |
And we have some hydration. It's still wonky, because the HYDRATE action does not seem to trigger the middleware on the client side, but I'm gonna look into that at a later time. Code in question: https://github.com/phryneas/ssr-experiments/blob/126262fcce7f8b788df3670dbc061362c65b6b9e/nextjs-blog/lib/pokemonApi.ts#L8-L12 Seems to get triggered on the server, but not on the client? |
@phryneas I'm sorry about I have no answer for your question 🙇♂️ Just put I thought about SSR/Next.js/ Next.js don't run SSR except visitor's first time access on the website. It's Next.js specific thing but seems tricky that |
Correction: it works perfectly, I just had a stale next cache and after deleting the So I guess I'm waiting for feedback on this one, if anyone wants to do some experimentation. Also, I would tacke a suspense example next, but don't have a lot of time at the moment. If anyone want to spin up a suspense example to try it out - or a SSR example with another framework than next, I'd be very grateful. |
@phryneas Congratulations! I think current SSR hydration flow has wasted process too much, so I wish Suspense fetch support publish soon. |
@ryota-murakami I probably won't merge this until I am sure the api here will work well with
essentially, we have only one shot at doing this right. So for experimentation, please just grab youself a CodeSandbox build and get going with that :) |
@phryneas sure, thanks for that information! |
return [ | ||
...Object.values(runningQueries), | ||
...Object.values(runningMutations), | ||
].filter(<T>(t: T | undefined): t is T => !!t) |
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.
💬 🍵 🍵 🍵 :)
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.
💯
// do not rehydrate entries that were currently in flight. | ||
entry?.status === QueryStatus.fulfilled || | ||
entry?.status === QueryStatus.rejected || | ||
// only rehydrate endpoints that were persisted using a `fixedCacheKey` | ||
key !== entry?.requestId |
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.
Umh, can it be that this should have been
// do not rehydrate entries that were currently in flight. | |
entry?.status === QueryStatus.fulfilled || | |
entry?.status === QueryStatus.rejected || | |
// only rehydrate endpoints that were persisted using a `fixedCacheKey` | |
key !== entry?.requestId | |
// do not rehydrate entries that were currently in flight. | |
(entry?.status === QueryStatus.fulfilled || entry?.status === QueryStatus.rejected) | |
// only rehydrate endpoints that were persisted using a `fixedCacheKey` | |
&& key !== entry?.requestId |
@markerikson @Shrugsy @msutkowski can you take a second look?
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.
Yeah, your updated version looks correct to me
getRunningOperationPromise: <EndpointName extends QueryKeys<Definitions>>( | ||
endpointName: EndpointName, | ||
args: QueryArgFrom<Definitions[EndpointName]> | ||
) => | ||
| QueryActionCreatorResult<Definitions[EndpointName]> | ||
| MutationActionCreatorResult<Definitions[EndpointName]> | ||
| undefined |
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.
getRunningOperationPromise: <EndpointName extends QueryKeys<Definitions>>( | |
endpointName: EndpointName, | |
args: QueryArgFrom<Definitions[EndpointName]> | |
) => | |
| QueryActionCreatorResult<Definitions[EndpointName]> | |
| MutationActionCreatorResult<Definitions[EndpointName]> | |
| undefined | |
getRunningOperationPromise: <EndpointName extends QueryKeys<Definitions>>( | |
endpointName: EndpointName, | |
args: QueryArgFrom<Definitions[EndpointName]> | |
) => | |
| QueryActionCreatorResult<Definitions[EndpointName]> | |
| undefined | |
getRunningOperationPromise: <EndpointName extends MutationKeys<Definitions>>( | |
endpointName: EndpointName, | |
fixedCacheKeyOrRequestId: string | |
) => | |
| MutationActionCreatorResult<Definitions[EndpointName]> | |
| undefined |
Just noticed this over in the docs 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.
@phryneas How would you actually do this? AFAIK TS doesn't support overloads with different types/params.
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.
Uhh... just exactly like that?
Co-authored-by: Lenz Weber <mail@lenzw.de>
Have been scrambling to get one of my projects in a good place to add this. Hope I am able to give it a try before 1.7 milestone complete. Haven't tried this yet but it should in theory work fine with getInitialProps as well, right? I am aware that using getInitialProps is generally advised against, but I have a situation where I want a page to be fully client side unless it is the first page load (strictly so SEO isn't broken.) |
@agusterodin I don't see any reason why it would not work in |
Congratulations! |
Woot! 🎉 |
Just tried it out on Nextjs with getInitialProps and it works beautifully. I am able to wait for a query to finish only if initial page load for SEO, otherwise behave as a normal client side application and show loading skeletons to keep things snappy 😀
|
I did notice that a duplicate request that gets sent out via client once the page is hydrated (getMetadata endpoint as shown above). I have a long keepUnusedDataFor value set for all endpoints. The application works as intended, the extra request isn't a showstopper by any means. I am not doing anything fancy with the useGetImageMetadataQuery auto-generated hook inside my component, not even providing an options object: SSR Extra Request Capture.json.zip I am on this version of RTK: https://pkg.csb.dev/reduxjs/redux-toolkit/commit/ea86a3fc/@reduxjs/toolkit |
Hmm. Can you create a small reproduction for that? I did not notice that behavior. |
Oh wait, I have refetchOnMountOrArgChange set to true for my whole API. Would it be correct to assume that with that option set to true, a refetch is supposed to happen on client side despite already manually initiating the same query in getInitialProps (as shown in code above)? |
Well, technically it is a mount and I don't believe we have any way of discerning a SSR hydration mount from a component mount, so yeah, that would be the cause. You could set it to something like |
Cool, then that is completely expected behavior. Really hyped on this making its way to RTK 1.7. Great work! |
This PR enables a few different use cases:
Pointers for docs
SSR with Next
An example repo can be found at https://github.com/phryneas/ssr-experiments/
getStaticProps
orgetServerSideProps
,store.dispatch(api.endpoints.getPokemonByName.initiate(name));
await Promise.all(api.getRunningOperationPromises());
createApi
call, configure rehydration in yourcreateApi
options:store.dispatch(api.util.resetApiState())
to make sure no rogue timers are running anywhere and clogging up memorySSR elsewhere
If it can not be done in a similar way as in the nextjs method above, there is also an
unstable__
marked approach to support SSR scenarios that render the full tree multiple times until nothing changes any more. (You know if you are doing this). Doing so might require async code to be triggered during render and not safely in an effect. If that is the case:createApi
that just does async stuff during render usingawait Promise.all(api.util.getRunningOperationPromises());
before you do your next render cycleSuspense
While this does not add suspense support, it adds a building block for suspense:
This enables writing custom hooks that look up if RTK-Q has already got a running promise for a certain endpoint/argument combination and retrieving that promise to
throw
it. We might in the future release an experimental companion package with a modifiedreactHooksModule
that builds upon this and creates suspense-ready hooks.Experimentation is very welcome, let us know what you come up with!
redux-persist
It should also be possible to use
with redux-persist's HYDRATE action.
This should work out-of-the-box with the
autoMergeLevel1
orautoMergeLevel2
(this one might still have some quirks with thesubscriptions
andconfig
sub-slices) state reconcilers, but not with thehardSet
reconciler.Changes
api.util.getRunningOperationPromises()
function that returns all promises for running queries and mutations (useful for SSR to await everything triggered in any way, via hook or initiate)api.util.getRunningOperationPromise(endpoint, arg)
function that returns all promises for running queries and mutations (useful for SSR to await everything triggered in any way, via hook or initiate)initiate
, so that a call that is made while a query is already running waits for the running query to resolveextractRehydrationInfo(action)
api config option that is passed every dispatched action. if this returns something other thanundefined
, that return value will be used to rehydrate fulfilled & errored queries (see https://github.dev/phryneas/ssr-experiments/blob/126262fcce7f8b788df3670dbc061362c65b6b9e/nextjs-blog/lib/pokemonApi.ts#L8-L12)