-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
WIP: feat: add suspense to useQueries #1754
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
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/tannerlinsley/react-query/kw4qlv2l7 |
Anything I can do to help out here? Any questions? |
import { UseQueryOptions, UseQueryResult } from './types' | ||
|
||
export function useQueries(queries: UseQueryOptions[]): UseQueryResult[] { | ||
export function useQueries<TData = unknown, TError = unknown>( |
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 work!
I have a question. This adds the ability to specify generics for useQueries
. Reading the code here, I think that this may fall into the "generic that is only used in the return position" anti-pattern category.
It's possible I'm reading it wrong so I wanted to ask a clarifying question: Does the approach here allow a user to specify types using the generic parameters that would differ from the return type of the query or of the select
function?
If that's possible then the generics would not be adding a great deal of value as incorrectly typed data could still flow through.
Is that possible in this case?
As discussed, I'm generally keen on making use of type inference. But in the absence of that, avoiding type definitions that can possibly let incorrect types through is still worth aiming for.
@boschni do you need any help with this PR? I'm really looking forward to this feature and ready to help you finish it. |
I would like to give a hand as well if there is a need. |
@boschni We would also benefit from this fix, and I'd be happy to contribute if that'd be helpful. @johnnyreilly It appears the original review just raised a question about the new type annotation. Is that the only outstanding change requested on this PR (besides conflict resolutions at this point : ))? If no other changes are requested, I'd be happy to resolve the conflicts and revert the generics type change. Then @boschni can open a future change to re-add generics pending that discussion. Would that be ok? |
Go for it! |
Thanks for confirming. This merge turns out to be a bit of a doozy, but I should be able to get through it over this weekend. |
@boschni @johnnyreilly Quick question as I work through what's amounting more to refactor of the fix than conflict resolution (given the amount of change to the target logic that's occurred since this was opened): The fix here assumes |
Talking about errors, my thought is that it should be configured with |
I've opened the new PR as #2109 due to the significant amount of change from this original PR. The commit in this PR is included in that PR. Please let me know if you'd prefer this be handled differently git-wise. Requesting a very thorough review of this one, as it was significantly more complicated than I expected and my context as a contributor going in was basically nil. Please be brutal : ) I did confirm it working with a basic CRA and yarn link. |
Fixes #1523