-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Feature/use queries suspense #2109
Feature/use queries suspense #2109
Conversation
…-suspense * Conflicts: - src/core/queriesObserver.ts - src/react/useQueries.ts
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/tannerlinsley/react-query/6YQJRibhQzpvvqMgiJ5akVseSkS9 |
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. |
src/react/tests/useQueries.test.tsx
Outdated
await sleep(10) | ||
|
||
expect(results.length).toBe(1) | ||
expect(results[0]).toMatchObject([{ data: 1 }, { data: 2 }]) |
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'd expect more from this test. If you are playing with suspense: true/false(undefined)
queries why not check intermediate values like [{ data: 1}, { loading: true }]
in this case?
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.
And actually expecting suspension on first query :)
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.
Thank you for the review!
What I think is happening here is that it is suspending, so fallback is rendering until the hook is no longer throwing, at which point it does its first render with actual data. Are there intermediate states I would expect to see on first render? Would a refetch to show new states after original suspense render help to make this case more robust?
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.
What i mean is: the component suspends on first render, but 5s later it render again and push [{ data: 1}, { loading: true }]
in results. This should happen because first query suspends since it has suspense: true
but the second query behaves like regular useQuery(...{ suspense: false })
doesn't 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 see what you mean. The initial PR suspended when any of the queries was configured for suspense and any of the queries was loading. In other words, if anything had suspense, everything had suspense.
This could be revised so that it only suspends when any of the queries that are configured for suspense are loading--is that the expected functionality? I'm honestly not sure what a client would expect as behavior here. I feel the same about the current summary error handling.
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, the test should go something like:
- Suspends on first load
- Data comes in
- query becomes stale
- query gets refetched
isFetching === true
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.
Revisiting this, apologies for the delay. I do feel a bit out of context on this one : )
Is the desired behavior that all queries have aggregate suspense and error state until the first settlement of all queries, and then queries behave independently?
My concern with this approach is that the individual queries still accept a fully query config, including options like suspense, but they aren't guaranteed to honor that config (and won't individually honor certain configs like suspense
).
I wonder if this is a case where useQueries
and useQueriesSuspense
should exist as distinct hooks that can explicitly define their option subsets and be opinionated for suspense vs no 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 wonder if this is a case where useQueries and useQueriesSuspense should exist as distinct hooks that can explicitly define their option subsets and be opinionated for suspense vs no suspense.
Hmm, I wouldn't be a big fan of this api wise. For example, because we just got #2395 reported: useErrorBoundary
alone also doesn't work with useQueries
, and I think the reasons are similar. I tried the reproduction out with this PR and it seems to work.
Now we have to think about if we even want different behaviour for multiple queries in the same useQueries
batch. E.g. one of them having suspense, while others don't. Or one has useErrorBoundary and another one doesn't.
I would say this shouldn't be allowed, kind of like an invalid state. The current api makes this possible because it's just an array of useQuery configs. If the structure were a bit different, it would be more obvious, something like:
useQueries({
queries: [{ queryKey1, queryFn1, options1 }, { queryKey2, queryFn2, options2 }],
suspense: boolean,
useErrorBoundary: boolean
})
with that, we would lift "global" options out of the array, so you can only set them once for all queries. The options on each query would then not allow what we have at the top level, e.g. suspense or useErrorBoundary.
I don't to propose a breaking change to the useQueries api though, just some food for thought. I am personally fine with doing what we do now, which is:
const someSuspense = defaultedQueries.some((q) => q.suspense);
const someUseErrorBoundary = defaultedQueries.some((q) => q.useErrorBoundary);
But we'd need to document that setting suspense
or useErrorBoundary
to true on any of the queries will affect all queries! It's not that intuitive, but we can revisit it for v4 maybe.
Any news on this pr? |
Apologies for letting this drag @RoyGrosz. The problem turned out to be more complex than expected, and I question the implementation proposed by this PR. Just left another comment for direction going forward. Not confident I should be the one owning this issue. For our org we've implemented some more opinionated workarounds, but we would still benefit from built-in support of suspense for a dynamic array of queries. |
…-suspense # Conflicts: # src/react/useBaseQuery.ts
@escaton Based on continued interest in the comments, I went back through and updated this and improved the test as suggested. GH is not allowing me to re-request your review, but this is ready for another look if there's desire to move forward. Note: Looks like workflows need approval to run remaining checks. |
* Run yarn format * Fix bad dependencies in useQueries * Fix explicit any in useQueries.test
Hey all, I dont want to be a nag, but am also checking daily to see if this feature has been included! We are very excited about the possibilities it opens with our app architecture. If there is something that needs to be worked on, I would be glad to pull this repo down and contribute, however it looks like perhaps its just the testing and review left? |
From my perspective this is just awaiting review, approval. @escaton you'd done the previous review, are you available for another look? |
Has this been addressed (document which settings will be "on" if they are on for one of the queries): #2109 (comment)? also, please solve the conflicts |
# Conflicts: # src/react/tests/useQueries.test.tsx
… useErrorBoundary in useQueries
@TkDodo I've resolved the conflict and updated the docs. I'm reproducing inconsistent test behavior locally for |
hmm, let's see. I've also had some failing tests on node10, they were timing related and I think I fixed them (they were also showing up on CI). Which node version are you using locally? |
@TkDodo Thanks for running the remaining tests, I'm on 14.17.1 locally. Appears the failures do repro in CI. Looks from blame like you might have context on the |
yeah I wrote those tests, and it was merged about a week ago. I'll have to investigate 🤔 |
let observers = this.observers | ||
|
||
if (options?.filter) { | ||
observers = this.observers.filter(options.filter) | ||
} |
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.
could this just be:
const observers = options?.filter ? this.observers.filter(options.filter) : this.observers
@@ -25,7 +40,7 @@ describe('useQueries', () => { | |||
{ | |||
queryKey: key2, | |||
queryFn: async () => { | |||
await sleep(10) | |||
await sleep(7) |
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 this just to make the tests faster?
const [observer] = React.useState( | ||
() => new QueriesObserver(queryClient, defaultedQueries) | ||
const observerRef = React.useRef( | ||
new QueriesObserver(queryClient, defaultedQueries) |
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.
why are we moving away from state to refs please? I explicitly made that change not too long ago :) It's the preferred way of doing one-time initializations - I don't see us writing to observerRef.current
anyhwere?
notifyManager.batchCalls(() => { | ||
if (mountedRef.current) { | ||
if (mountedRef.current && someIsLoading) { |
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.
why do we need this check for someIsLoading
here please? This looks like we're never force-updating if nothing is loading, which doesn't seem right 🤔
|
||
expect(results.length).toBe(3) | ||
expect(results[0]).toMatchObject([{ data: undefined }, { data: undefined }]) | ||
expect(results[1]).toMatchObject([{ data: 1 }, { data: undefined }]) | ||
expect(results[2]).toMatchObject([{ data: 1 }, { data: 2 }]) | ||
}) | ||
|
||
it('should return the correct states with suspense', async () => { |
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 you maybe add a separate test for only useErrorBoundary
without suspense
?
|
||
consoleMock.mockRestore() | ||
}) | ||
|
||
it('should keep previous data if amount of queries is the same', async () => { |
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.
regarding the test failures: could it be that the tests somehow influence each other? I see no failures on master, neither locally nor in CI 🤔
observer.setQueries(defaultedQueries, { listeners: false }) | ||
}, [defaultedQueries, observer]) | ||
const handleReset = React.useCallback(() => { | ||
errorResetBoundary.clearReset() |
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 don't think we can just call this during render, as this has a side-effect. In useBaseQuery.ts
, we call this in the catch
of the throw observer.fetchOptimistic
.
throw observerRef.current | ||
.refetch({ filter: x => x.getCurrentResult().isLoading }) | ||
.finally(unsubscribe) |
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.
should we not do something like fetchOptimistic
does on the QueryObserver, just for multiple queries? We're also not calling onSuccess / onError / onSettled
like we do in useBaseQuery
....
Hi! it's been a while since this PR saw any activity, and I'd like to offer some help in getting this over the finish line -- i really want to use Also, a separate question for @TkDodo - assuming I could get this PR to a state everyone's happy with, do you see this as mergeable anytime soon? I've been keeping up with the React 18 stuff over at reactwg and saw your comments in the |
@lubieowoce I appreciate the offer, I would really appreciate it if you could take this the rest of the way. |
@lubieowoce thank you for offering to take this over.
from what I've seen so far, the uSES changes will not interfere with what also, I suspect that it will take quite some time until React 18 will be released, so yes, I think I would definitely merge this before 🚀 |
Alright, I'll get on it! :) @TkDodo I've got one question re: intended behavior. Based on previous discussions i've seen:
It seems as though, for a static array of queries (no adding new queries, no reordering), this should work as a "reference implementation" to test the actual one against:
code for the above (seems to behave as intended after some limited testing)function useQueriesSuspense(queries) {
// ==========================
// THIS IS A BARELY-TESTED REFERENCE IMPLEMENTATION
// NOT INTENDED FOR PRODUCTION USAGE. YOU HAVE BEEN WARNED
// ==========================
// Execute the queries "in parallel", see if any of them suspend or throw an error
const queryAttempts = queries.map((query) => {
try {
const value = useQuery({ ...query, suspense: true, useErrorBoundary: true });
return { type: 'ready', value }
} catch (thrown) {
if (thrown instanceof Promise) {
return { type: 'suspended', promise: thrown }
} else {
return { type: 'error', error: thrown }
}
}
});
const didError = (a) => a.type === 'error';
const didSuspend = (a) => a.type === 'suspended';
if (queryAttempts.some(didError)) {
// Rethrow the first error
throw queryAttempts.find(didError).error;
} else if (queryAttempts.some(didSuspend)) {
// Suspend until all the queries that suspended are ready
throw Promise.all(
queryAttempts.filter(didSuspend).map((a) => a.promise)
);
} else {
// Everything's ready, return!
return queryAttempts.map((a) => a.value);
}
} Note about concurrent renderingBased on my (limited 😅) understanding this *may* not be concurrent-rendering safe:
so my thinking is, if a render got interrupted and this hook was called again in a concurrent render, Obviously this only works if the queries array has a static shape/order -- otherwise we'll start getting nonsense results or a "mismatched number of hook calls" error. It'd be great to hear if this makes sense for you -- having a reference implementation to compare against would be great for testing/development. |
I don't think this is how useQueries is implemented. There is a separate What you have stated about the intended behaviour is correct though. We can't really have one query suspend while others should not suspend. This is why we've agreed that we'll just check if any of the queries uses suspense to see if all of them should suspend. |
oh yeah, i know it's not implemented that way! :D i've already started playing around with a real implementation using QueriesObserver (based on this PR), i just wanted to make sure that's behaviorally equivalent to what we want (for |
@lubieowoce we have a proposal for a potential re-design of the |
This is very stale, and afaik this feature has been fixed in latest versions. Closing : ) |
@steevsachs fyi, v4 still does not support suspense for useQueries. We have a new api (top level object instead of arrays), which allows for some new ideas, but nothing has been done yet. |
@TkDodo Hmm, thank you for the call out. I agree, this support would definitely be useful (we've at this point worked around it in a variety of purpose-specific ways). My sense is the effort might benefit from a fresh approach rather than being bogged down by this PR given how much change has occurred--would you agree? |
fixes #1523
Attempting to resolve merge conflicts and carry forward the fix from #1754
This is my first contribution to the repo and I have low context (both on the repo and the PR, since I didn't open it originally), so I would appreciate someone taking a close look at this change to ensure it does not introduce new issues.
Notes:
queryClient.test.tsx