Error boundary handling for useQueries #4177
Conversation
|
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 e5ab278:
|
Codecov ReportBase: 96.36% // Head: 96.59% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #4177 +/- ##
==========================================
+ Coverage 96.36% 96.59% +0.23%
==========================================
Files 45 73 +28
Lines 2281 2969 +688
Branches 640 816 +176
==========================================
+ Hits 2198 2868 +670
- Misses 80 99 +19
+ Partials 3 2 -1 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
zorzysty
changed the title
| @@ -188,5 +190,35 @@ export function useQueries<T extends any[]>({ | |||
| observer.setQueries(defaultedQueries, { listeners: false }) | |||
| }, [defaultedQueries, observer]) | |||
|
|
|||
| const errorResetBoundary = useQueryErrorResetBoundary() | |||
|
|
|||
| defaultedQueries.forEach((query) => { | |||
There was a problem hiding this comment.
can we rename this from queries to options ? I think this is highly confusing right now 😅
There was a problem hiding this comment.
sorry, I meant more globally that defaultedQueries should also probably be defaultedOptions. It's not a query 😅
There was a problem hiding this comment.
okay nevermind, this is actually a bigger refactoring. we have queries everywhere and also setQueries...
There was a problem hiding this comment.
EDIT:
Just to make it clear, I'm responding to this comment:
sorry, I meant more globally that defaultedQueries should also probably be defaultedOptions. It's not a query
ORIGINAL COMMENT BELOW:
I was struggling to grasp the naming conventions here, and when I thought I finally got it, this comment got me confused again 😁
Let me explain how I understand it. First, here are 3 facts about the code in this PR its current state:
useQueriestakesqueriesarray as an argument
export function useQueries<T extends any[]>({
queries,
context,
}: {
queries: readonly [...QueriesOptions<T>]
context?: UseQueryOptions['context']
})- Each item of
queriesarray is a query, but query is basically anoptionsobject - These
queriesare then defaulted and stored asdefaultedQueries:
const defaultedQueries = React.useMemo(
() =>
queries.map((options) => {
const defaultedOptions = queryClient.defaultQueryOptions(options)
// Make sure the results are already in fetching state before subscribing or updating options
defaultedOptions._optimisticResults = isRestoring
? 'isRestoring'
: 'optimistic'
return defaultedOptions
}),
[queries, queryClient, isRestoring],
)It would be strange if mapping with default values would suddenly convert queries to options. Furthermore in the code below QueriesObserver constructor has second parameter named queries and not options (although queries is typed as QueryObserverOptions, because why not xD).
const [observer] = React.useState(
() => new QueriesObserver(queryClient, defaultedQueries),
)So here are my thoughts to sum it up:
If we rename defaultedQueries to defaultedOptions it would be confusing because options are of a single query and not of queries array (useQueries does not have options on its own). To make it more consistent and less confusing, I would instead suggest to take the example from how queries are typed in useQueries arguments (type is called QueriesOptions) and do the name change this way:
defaultedQueries -> defaultedQueriesOptions
WDYT?
If that makes sense, maybe we could also do this to make it even more consistent:
export function useQueries<T extends any[]>({
queries: queriesOptions,
context,
}There was a problem hiding this comment.
yeah the naming is weird, I agree. But we have a public method called setQueries that actually takes options in, but they are named queries :/
so for now, let's:
- revert the lats commit (sorry) and just keep it
queriesfor now. Being consistent is better I think.
there are also conflicts now, sorry. If those are solved we can go ahead and merge 🚀
zorzysty
force-pushed
the
error_boundary_for_usequeries
branch
from
5552172 to
7f81e88
Compare
…usequeries # Conflicts: # packages/react-query/src/useBaseQuery.ts # packages/react-query/src/useQueries.ts
closes #2395