Skip to content

Commit

Permalink
fix(query): avoid creating entries after unmounting
Browse files Browse the repository at this point in the history
Fix #155
  • Loading branch information
posva committed Jan 14, 2025
1 parent 2f880bb commit e2ff278
Show file tree
Hide file tree
Showing 3 changed files with 84 additions and 16 deletions.
8 changes: 8 additions & 0 deletions src/query-store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -433,6 +433,7 @@ export const useQueryCache = /* @__PURE__ */ defineStore(QUERY_STORE_ID, ({ acti
TDataInitial extends TResult | undefined = undefined,
>(
opts: UseQueryOptions<TResult, TError, TDataInitial>,
previousEntry?: UseQueryEntry<TResult, TError, TDataInitial>,
): UseQueryEntry<TResult, TError, TDataInitial> => {
const options: UseQueryOptionsWithDefaults<TResult, TError, TDataInitial> = {
...optionDefaults,
Expand All @@ -450,6 +451,13 @@ export const useQueryCache = /* @__PURE__ */ defineStore(QUERY_STORE_ID, ({ acti
let entry = cachesRaw.get(key) as UseQueryEntry<TResult, TError, TDataInitial> | undefined
if (!entry) {
cachesRaw.set(key, (entry = create(key, options, options.initialData?.())))
// the placeholderData is only used if the entry is initially loading
if (options.placeholderData && entry.state.value.status === 'pending') {
entry.placeholderData = toValueWithArgs(
options.placeholderData,
previousEntry?.state.value.data,
)
}
}

// warn against using the same key for different functions
Expand Down
62 changes: 61 additions & 1 deletion src/use-query.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import type { UseQueryOptions } from './query-options'
import { enableAutoUnmount, flushPromises, mount } from '@vue/test-utils'
import { createPinia } from 'pinia'
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
import { createApp, defineComponent, nextTick, ref } from 'vue'
import { computed, createApp, defineComponent, nextTick, ref, type PropType } from 'vue'
import { mockWarn } from '../test/mock-warn'
import { createSerializedTreeNodeEntry, delay, isSpy, promiseWithResolvers } from '../test/utils'
import { PiniaColada } from './pinia-colada'
Expand Down Expand Up @@ -997,6 +997,66 @@ describe('useQuery', () => {
expect(query).toHaveBeenCalledTimes(2)
})

it('should not create entries while unmounting', async () => {
const NestedComp = defineComponent({
props: {
queryKey: {
type: Object as PropType<{ key: string }>,
required: true,
},
},
render: () => 'child',
setup(props) {
const key = computed(() => [props.queryKey.key])
const useQueryResult = useQuery({
key,
query: async () => 'ok',
})
return {
...useQueryResult,
}
},
})

const wrapper = mount(
defineComponent({
components: { NestedComp },
template: `
<div>
<NestedComp v-if="queryKey.key" :queryKey="queryKey" />
</div>
`,
setup() {
const queryKey = ref<{ key: string | undefined }>({ key: 'key' })
return {
queryKey,
}
},
}),
{
global: {
plugins: [createPinia(), PiniaColada],
},
},
)
const queryCache = useQueryCache()

await flushPromises()
expect(wrapper.text()).toBe('child')
let entries = queryCache.getEntries()
expect(entries.length).toBe(1)

// changing the key unmounts the nested component
// but it should not create a new entry
wrapper.vm.queryKey.key = undefined
entries = queryCache.getEntries()
expect(entries.length).toBe(1)
await flushPromises()
expect(wrapper.text()).toBe('')
entries = queryCache.getEntries()
expect(entries.length).toBe(1)
})

describe('shared state', () => {
it('reuses the same state for the same key', async () => {
const pinia = createPinia()
Expand Down
30 changes: 15 additions & 15 deletions src/use-query.ts
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,15 @@ export function useQuery<
}
}

const entry = computed(() => queryCache.ensure<TResult, TError, TDataInitial>(options))
// This plain variable is not reactive and allows us to use the currentEntry
// without triggering watchers and creating entries. It is used during
// unmounting and mounting
let lastEntry: UseQueryEntry<TResult, TError, TDataInitial>
const entry = computed(
() => (lastEntry = queryCache.ensure<TResult, TError, TDataInitial>(options, lastEntry)),
)
// we compute the entry here and reuse this across
lastEntry = entry.value

// adapter that returns the entry state
const errorCatcher = () => entry.value.state.value
Expand Down Expand Up @@ -183,7 +191,7 @@ export function useQuery<

// TODO: find a way to allow a custom implementation for the returned value
const extensions = {} as Record<string, any>
for (const key in entry.value.ext) {
for (const key in lastEntry.ext) {
extensions[key] = computed({
get: () => toValue(entry.value.ext[key as keyof UseQueryEntryExtensions<TResult, TError>]),
set(value) {
Expand Down Expand Up @@ -231,32 +239,25 @@ export function useQuery<
if (hasCurrentInstance) {
onMounted(() => {
isActive = true
queryCache.track(entry.value, hasCurrentInstance)
queryCache.track(lastEntry, hasCurrentInstance)
})
onUnmounted(() => {
// remove instance from Set of refs
queryCache.untrack(entry.value, hasCurrentInstance, queryCache)
queryCache.untrack(lastEntry, hasCurrentInstance, queryCache)
})
} else {
isActive = true
if (currentEffect) {
queryCache.track(entry.value, currentEffect)
queryCache.track(lastEntry, currentEffect)
onScopeDispose(() => {
queryCache.untrack(entry.value, currentEffect, queryCache)
queryCache.untrack(lastEntry, currentEffect, queryCache)
})
}
}

watch(
entry,
(entry, previousEntry) => {
// the placeholderData is only used if the entry is initially loading
if (options.placeholderData && entry.state.value.status === 'pending') {
entry.placeholderData = toValueWithArgs(
options.placeholderData,
previousEntry?.state.value.data,
)
}
if (!isActive) return
if (previousEntry) {
queryCache.untrack(previousEntry, hasCurrentInstance, queryCache)
Expand All @@ -266,12 +267,11 @@ export function useQuery<
queryCache.track(entry, hasCurrentInstance)
queryCache.track(entry, currentEffect)

// TODO: does this trigger after unmount?
if (toValue(enabled)) refresh()
},
{
immediate: true,
// We need to set the placeholderData immediately to avoid an undefined value
flush: 'sync',
},
)

Expand Down

0 comments on commit e2ff278

Please sign in to comment.