Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
fix(useQueries): make sure keepPreviousData is respected #2340
Conversation
|
This pull request is being automatically deployed with Vercel (learn more).
|
|
Hmm why is codesandbox not running on this PR and how can i trigger it? |
|
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 4fe6ff4:
|
| expect(states[3]).toMatchObject([ | ||
| { status: 'success', data: 2, isPreviousData: true, isFetching: true }, | ||
| { status: 'success', data: 5, isPreviousData: true, isFetching: true }, | ||
| ]) | ||
| expect(states[4]).toMatchObject([ | ||
| { status: 'success', data: 2, isPreviousData: true, isFetching: true }, | ||
| { status: 'success', data: 5, isPreviousData: true, isFetching: true }, | ||
| ]) |
There was a problem hiding this comment.
not sure why I'm getting two re-renders here with the same results, I think it's because each of the two queries transitions to isPreviousData separately ... ?
| ) | ||
| let currentObserver = this.observersMap[defaultedOptions.queryHash!] | ||
| if (!currentObserver && defaultedOptions.keepPreviousData) { | ||
| currentObserver = this.observers[index] |
There was a problem hiding this comment.
please let me know if this is a bad idea. Generally, I was thinking that if we have keepPreviousData on, we would like to keep the data of the query that lies at the index. This likely won't work if the lengths are completely different, and we might "keep" wrong data then. But by just going by the queryHash like before, we're never able to make keepPreviousData work because we'll get a new Observer every time as the hash changes ...
There was a problem hiding this comment.
One way to limit this would be to only do the index access if the length hasn’t changed. That would make keepPreviousData only work on equal lengths though and we should likely document that
|
@tannerlinsley @boschni could I get your feedback on this PR? I left comments in a self-review on parts where I'm not sure, thanks :) |
| { status: 'success', data: 4, isPreviousData: false, isFetching: false }, | ||
| { status: 'success', data: 10, isPreviousData: false, isFetching: false }, | ||
| ]) | ||
| }) |
There was a problem hiding this comment.
I created two more test cases, tell me what you think:
it('should keep previous data if amount of queries is the same when has 3 elements', async () => {
const key1 = queryKey()
const key2 = queryKey()
const key3 = queryKey()
const states: UseQueryResult[][] = []
function Page() {
const [count, setCount] = React.useState(1)
const result = useQueries([
{
queryKey: [key1, count],
keepPreviousData: true,
queryFn: async () => {
await sleep(5)
return 1 * count
},
},
{
queryKey: [key2, count],
keepPreviousData: true,
queryFn: async () => {
await sleep(10)
return 2 * count
},
},
{
queryKey: [key3, count],
keepPreviousData: true,
queryFn: async () => {
await sleep(15)
return 3 * count
},
},
])
states.push(result)
React.useEffect(() => {
setActTimeout(() => {
setCount(prev => prev + 1)
}, 20)
}, [])
return null
}
renderWithClient(queryClient, <Page />)
await waitFor(() => expect(states.length).toBe(9))
expect(states[0]).toMatchObject([
{
status: 'loading',
data: undefined,
isPreviousData: false,
isFetching: true,
},
{
status: 'loading',
data: undefined,
isPreviousData: false,
isFetching: true,
},
{
status: 'loading',
data: undefined,
isPreviousData: false,
isFetching: true,
},
])
expect(states[1]).toMatchObject([
{ status: 'success', data: 1, isPreviousData: false, isFetching: false },
{
status: 'loading',
data: undefined,
isPreviousData: false,
isFetching: true,
},
{
status: 'loading',
data: undefined,
isPreviousData: false,
isFetching: true,
},
])
expect(states[2]).toMatchObject([
{ status: 'success', data: 1, isPreviousData: false, isFetching: false },
{ status: 'success', data: 2, isPreviousData: false, isFetching: false },
{
status: 'loading',
data: undefined,
isPreviousData: false,
isFetching: true,
},
])
expect(states[3]).toMatchObject([
{ status: 'success', data: 1, isPreviousData: false, isFetching: false },
{ status: 'success', data: 2, isPreviousData: false, isFetching: false },
{ status: 'success', data: 3, isPreviousData: false, isFetching: false },
])
expect(states[4]).toMatchObject([
{ status: 'success', data: 1, isPreviousData: true, isFetching: true },
{ status: 'success', data: 2, isPreviousData: true, isFetching: true },
{ status: 'success', data: 3, isPreviousData: true, isFetching: true },
])
expect(states[5]).toMatchObject([
{ status: 'success', data: 1, isPreviousData: true, isFetching: true },
{ status: 'success', data: 2, isPreviousData: true, isFetching: true },
{ status: 'success', data: 3, isPreviousData: true, isFetching: true },
])
expect(states[6]).toMatchObject([
{ status: 'success', data: 2, isPreviousData: false, isFetching: false },
{ status: 'success', data: 2, isPreviousData: true, isFetching: true },
{ status: 'success', data: 3, isPreviousData: true, isFetching: true },
])
expect(states[7]).toMatchObject([
{ status: 'success', data: 2, isPreviousData: false, isFetching: false },
{ status: 'success', data: 4, isPreviousData: false, isFetching: false },
{ status: 'success', data: 3, isPreviousData: true, isFetching: true },
])
expect(states[8]).toMatchObject([
{ status: 'success', data: 2, isPreviousData: false, isFetching: false },
{ status: 'success', data: 4, isPreviousData: false, isFetching: false },
{ status: 'success', data: 6, isPreviousData: false, isFetching: false },
])
})
it('should not keep previous data', async () => {
const key1 = queryKey()
const key2 = queryKey()
const key3 = queryKey()
const states: UseQueryResult[][] = []
function Page() {
const [count, setCount] = React.useState(1)
const result = useQueries([
{
queryKey: [key1, count],
queryFn: async () => {
await sleep(5)
return 1 * count
},
},
{
queryKey: [key2, count],
queryFn: async () => {
await sleep(10)
return 2 * count
},
},
{
queryKey: [key3, count],
queryFn: async () => {
await sleep(15)
return 3 * count
},
},
])
states.push(result)
React.useEffect(() => {
setActTimeout(() => {
setCount(prev => prev + 1)
}, 20)
}, [])
return null
}
renderWithClient(queryClient, <Page />)
await waitFor(() => expect(states.length).toBe(9))
expect(states[0]).toMatchObject([
{
status: 'loading',
data: undefined,
isPreviousData: false,
isFetching: true,
},
{
status: 'loading',
data: undefined,
isPreviousData: false,
isFetching: true,
},
{
status: 'loading',
data: undefined,
isPreviousData: false,
isFetching: true,
},
])
expect(states[1]).toMatchObject([
{ status: 'success', data: 1, isPreviousData: false, isFetching: false },
{
status: 'loading',
data: undefined,
isPreviousData: false,
isFetching: true,
},
{
status: 'loading',
data: undefined,
isPreviousData: false,
isFetching: true,
},
])
expect(states[2]).toMatchObject([
{ status: 'success', data: 1, isPreviousData: false, isFetching: false },
{ status: 'success', data: 2, isPreviousData: false, isFetching: false },
{
status: 'loading',
data: undefined,
isPreviousData: false,
isFetching: true,
},
])
expect(states[3]).toMatchObject([
{ status: 'success', data: 1, isPreviousData: false, isFetching: false },
{ status: 'success', data: 2, isPreviousData: false, isFetching: false },
{ status: 'success', data: 3, isPreviousData: false, isFetching: false },
])
expect(states[4]).toMatchObject([
{
status: 'loading',
data: undefined,
isPreviousData: false,
isFetching: true,
},
{
status: 'loading',
data: undefined,
isPreviousData: false,
isFetching: true,
},
{
status: 'loading',
data: undefined,
isPreviousData: false,
isFetching: true,
},
])
expect(states[5]).toMatchObject([
{
status: 'loading',
data: undefined,
isPreviousData: false,
isFetching: true,
},
{
status: 'loading',
data: undefined,
isPreviousData: false,
isFetching: true,
},
{
status: 'loading',
data: undefined,
isPreviousData: false,
isFetching: true,
},
])
expect(states[6]).toMatchObject([
{ status: 'success', data: 2, isPreviousData: false, isFetching: false },
{
status: 'loading',
data: undefined,
isPreviousData: false,
isFetching: true,
},
{
status: 'loading',
data: undefined,
isPreviousData: false,
isFetching: true,
},
])
expect(states[7]).toMatchObject([
{ status: 'success', data: 2, isPreviousData: false, isFetching: false },
{ status: 'success', data: 4, isPreviousData: false, isFetching: false },
{
status: 'loading',
data: undefined,
isPreviousData: false,
isFetching: true,
},
])
expect(states[8]).toMatchObject([
{ status: 'success', data: 2, isPreviousData: false, isFetching: false },
{ status: 'success', data: 4, isPreviousData: false, isFetching: false },
{ status: 'success', data: 6, isPreviousData: false, isFetching: false },
])
})
There was a problem hiding this comment.
when has 3 elements
thanks, but your test has 3 static elements. My claim is that it might not work with dynamic elements, so 2 elements on the first render, followed by 3 elements on the next render. I added a test for this here: 62ce4ff and it seems to work okay.
I'm still positive that if you remove one element in the middle of the list, you will keep the previous data from this item. not sure if this will be perceived as intended behaviour, which is why I've suggested a different solution here: #2340 (comment), which would limit keepPreviousData to arrays of the same length...
to make sure node10 resolves them in order
|
The release is available on: Your semantic-release bot |
fixes #2181