-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Feature/use queries suspense #2109
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
Changes from all commits
cbec5d7
d7b3162
ed20430
b7dea53
fc094cf
84e5eb9
9a7f633
3c581e7
2b75f2e
9053f5d
e278a7c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,23 @@ | ||
import { waitFor } from '@testing-library/react' | ||
import { fireEvent, waitFor } from '@testing-library/react' | ||
import React from 'react' | ||
import { ErrorBoundary } from 'react-error-boundary' | ||
|
||
import { queryKey, renderWithClient, setActTimeout, sleep } from './utils' | ||
import { useQueries, QueryClient, UseQueryResult, QueryCache } from '../..' | ||
import { | ||
mockConsoleError, | ||
queryKey, | ||
renderWithClient, | ||
setActTimeout, | ||
sleep, | ||
} from './utils' | ||
import { | ||
useQueries, | ||
QueryClient, | ||
UseQueryResult, | ||
QueryCache, | ||
useQueryErrorResetBoundary, | ||
useQueryClient, | ||
} from '../..' | ||
import { QueryState } from '../../core/query' | ||
|
||
describe('useQueries', () => { | ||
const queryCache = new QueryCache() | ||
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. is this just to make the tests faster? |
||
return 2 | ||
}, | ||
}, | ||
|
@@ -36,14 +51,145 @@ describe('useQueries', () => { | |
|
||
renderWithClient(queryClient, <Page />) | ||
|
||
await sleep(30) | ||
await sleep(10) | ||
|
||
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 commentThe reason will be displayed to describe this comment to others. Learn more. can you maybe add a separate test for only |
||
const key1 = queryKey() | ||
const key2 = queryKey() | ||
const results: (QueryState<unknown, undefined>[] | UseQueryResult[])[] = [] | ||
|
||
function Fallback() { | ||
const qc = useQueryClient() | ||
const queryStates = [qc.getQueryState(key1)!, qc.getQueryState(key2)!] | ||
|
||
results.push(queryStates) | ||
|
||
return null | ||
} | ||
|
||
function Page() { | ||
const result = useQueries([ | ||
{ | ||
queryKey: key1, | ||
queryFn: async () => { | ||
await sleep(5) | ||
return 1 | ||
}, | ||
suspense: true, | ||
}, | ||
{ | ||
queryKey: key2, | ||
queryFn: async () => { | ||
await sleep(7) | ||
return 2 | ||
}, | ||
}, | ||
]) | ||
results.push(result) | ||
return null | ||
} | ||
|
||
renderWithClient( | ||
queryClient, | ||
<React.Suspense fallback={<Fallback />}> | ||
<Page /> | ||
</React.Suspense> | ||
) | ||
|
||
await waitFor(() => { | ||
expect(results[0]).toMatchObject([ | ||
{ data: undefined }, | ||
{ data: undefined }, | ||
]) | ||
expect(results[1]).toMatchObject([{ data: 1 }, { data: 2 }]) | ||
expect(results.length).toBe(2) | ||
}) | ||
}) | ||
|
||
it('should retry fetch if the reset error boundary has been reset with global hook', async () => { | ||
const key1 = queryKey() | ||
const key2 = queryKey() | ||
|
||
let succeed = false | ||
const consoleMock = mockConsoleError() | ||
|
||
function Page() { | ||
const results = useQueries([ | ||
{ | ||
queryKey: key1, | ||
queryFn: async () => { | ||
await sleep(5) | ||
if (!succeed) { | ||
throw new Error('Suspense Error Bingo') | ||
} else { | ||
return 'data1' | ||
} | ||
}, | ||
retry: false, | ||
suspense: true, | ||
}, | ||
{ | ||
queryKey: key2, | ||
queryFn: () => 'data2', | ||
staleTime: Infinity, | ||
}, | ||
]) | ||
|
||
return ( | ||
<div> | ||
<div>data1: {results[0]?.data}</div> | ||
<div>data2: {results[1]?.data}</div> | ||
</div> | ||
) | ||
} | ||
|
||
function App() { | ||
const { reset } = useQueryErrorResetBoundary() | ||
return ( | ||
<ErrorBoundary | ||
onReset={reset} | ||
fallbackRender={({ resetErrorBoundary }) => ( | ||
<div> | ||
<div>error boundary</div> | ||
<button | ||
onClick={() => { | ||
resetErrorBoundary() | ||
}} | ||
> | ||
retry | ||
</button> | ||
</div> | ||
)} | ||
> | ||
<React.Suspense fallback="Loading..."> | ||
<Page /> | ||
</React.Suspense> | ||
</ErrorBoundary> | ||
) | ||
} | ||
|
||
const rendered = renderWithClient(queryClient, <App />) | ||
|
||
await waitFor(() => rendered.getByText('Loading...')) | ||
await waitFor(() => rendered.getByText('error boundary')) | ||
await waitFor(() => rendered.getByText('retry')) | ||
fireEvent.click(rendered.getByText('retry')) | ||
await waitFor(() => rendered.getByText('error boundary')) | ||
await waitFor(() => rendered.getByText('retry')) | ||
succeed = true | ||
fireEvent.click(rendered.getByText('retry')) | ||
await waitFor(() => rendered.getByText('data1: data1')) | ||
await waitFor(() => rendered.getByText('data2: data2')) | ||
|
||
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 commentThe 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 🤔 |
||
const key1 = queryKey() | ||
const key2 = queryKey() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,13 +3,15 @@ import React from 'react' | |
import { notifyManager } from '../core/notifyManager' | ||
import { QueriesObserver } from '../core/queriesObserver' | ||
import { useQueryClient } from './QueryClientProvider' | ||
import { useQueryErrorResetBoundary } from './QueryErrorResetBoundary' | ||
import { UseQueryOptions, UseQueryResult } from './types' | ||
|
||
export function useQueries(queries: UseQueryOptions[]): UseQueryResult[] { | ||
const mountedRef = React.useRef(false) | ||
const [, forceUpdate] = React.useState(0) | ||
|
||
const queryClient = useQueryClient() | ||
const errorResetBoundary = useQueryErrorResetBoundary() | ||
|
||
const defaultedQueries = queries.map(options => { | ||
const defaultedOptions = queryClient.defaultQueryObserverOptions(options) | ||
|
@@ -20,18 +22,30 @@ export function useQueries(queries: UseQueryOptions[]): UseQueryResult[] { | |
return defaultedOptions | ||
}) | ||
|
||
const [observer] = React.useState( | ||
() => new QueriesObserver(queryClient, defaultedQueries) | ||
const observerRef = React.useRef( | ||
new QueriesObserver(queryClient, defaultedQueries) | ||
Comment on lines
-23
to
+26
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
) | ||
|
||
const result = observer.getOptimisticResult(defaultedQueries) | ||
const result = observerRef.current.getOptimisticResult(defaultedQueries) | ||
|
||
React.useEffect(() => { | ||
// Do not notify on updates because of changes in the options because | ||
// these changes should already be reflected in the optimistic result. | ||
observerRef.current.setQueries(defaultedQueries, { listeners: false }) | ||
}, [defaultedQueries]) | ||
|
||
const someSuspense = defaultedQueries.some(q => q.suspense) | ||
const someUseErrorBoundary = defaultedQueries.some(q => q.useErrorBoundary) | ||
const firstResultWithError = result.find(r => r.error) | ||
const someError = firstResultWithError?.error | ||
const someIsLoading = result.some(r => r.isLoading) | ||
|
||
React.useEffect(() => { | ||
mountedRef.current = true | ||
|
||
const unsubscribe = observer.subscribe( | ||
const unsubscribe = observerRef.current.subscribe( | ||
notifyManager.batchCalls(() => { | ||
if (mountedRef.current) { | ||
if (mountedRef.current && someIsLoading) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we need this check for |
||
forceUpdate(x => x + 1) | ||
} | ||
}) | ||
|
@@ -41,13 +55,29 @@ export function useQueries(queries: UseQueryOptions[]): UseQueryResult[] { | |
mountedRef.current = false | ||
unsubscribe() | ||
} | ||
}, [observer]) | ||
}, [someIsLoading]) | ||
|
||
React.useEffect(() => { | ||
// Do not notify on updates because of changes in the options because | ||
// these changes should already be reflected in the optimistic result. | ||
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 commentThe 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 |
||
const unsubscribe = observerRef.current.subscribe() | ||
throw observerRef.current | ||
.refetch({ filter: x => x.getCurrentResult().isLoading }) | ||
.finally(unsubscribe) | ||
Comment on lines
+63
to
+65
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we not do something like |
||
}, [errorResetBoundary]) | ||
|
||
// Handle suspense and error boundaries | ||
if (someSuspense || someUseErrorBoundary) { | ||
if (someError) { | ||
if (errorResetBoundary.isReset()) { | ||
handleReset() | ||
} | ||
throw someError | ||
} | ||
|
||
if (someSuspense && someIsLoading) { | ||
handleReset() | ||
} | ||
} | ||
|
||
return result | ||
} |
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: