From ae37a8ec218ba62f0bac20039d6e8942f63f0d96 Mon Sep 17 00:00:00 2001 From: Eduardo San Martin Morote Date: Fri, 9 Feb 2024 15:47:13 +0100 Subject: [PATCH] fix: allow errors outside of navigation --- src/data-fetching_new/createDataLoader.ts | 2 + src/data-fetching_new/defineColadaLoader.ts | 91 ++++++++++++++++----- src/data-fetching_new/defineLoader.ts | 16 +++- src/data-fetching_new/navigation-guard.ts | 11 ++- tests/data-loaders/tester.ts | 69 ++++++++++------ 5 files changed, 139 insertions(+), 50 deletions(-) diff --git a/src/data-fetching_new/createDataLoader.ts b/src/data-fetching_new/createDataLoader.ts index 99fda9e49..4f640cb58 100644 --- a/src/data-fetching_new/createDataLoader.ts +++ b/src/data-fetching_new/createDataLoader.ts @@ -191,6 +191,8 @@ export interface DefineDataLoader { ): UseDataLoader } +// TODO: should be in each data loader. Refactor the base type to accept the needed generics + /** * Data Loader composable returned by `defineLoader()`. * @see {@link DefineDataLoader} diff --git a/src/data-fetching_new/defineColadaLoader.ts b/src/data-fetching_new/defineColadaLoader.ts index 1de9280f2..3da56be96 100644 --- a/src/data-fetching_new/defineColadaLoader.ts +++ b/src/data-fetching_new/defineColadaLoader.ts @@ -19,6 +19,7 @@ import { IS_USE_DATA_LOADER_KEY, LOADER_ENTRIES_KEY, NAVIGATION_RESULTS_KEY, + PENDING_LOCATION_KEY, STAGED_NO_VALUE, _DefineLoaderEntryMap, } from './meta-extensions' @@ -30,7 +31,7 @@ import { setCurrentContext, trackRoute, } from './utils' -import { Ref, ShallowRef, ref, shallowRef } from 'vue' +import { Ref, ShallowRef, ref, shallowRef, toRaw, watch } from 'vue' import { NavigationResult } from './navigation-guard' import { UseQueryKey, @@ -155,7 +156,7 @@ export function defineColadaLoader( }) } - const { isLoading, data, ext } = entry + const { isLoading, data, error, ext } = entry // we are rendering for the first time and we have initial data // we need to synchronously set the value so it's available in components @@ -219,7 +220,12 @@ export function defineColadaLoader( // e // ) // in this case, commit will never be called so we should just drop the error - if (options.lazy || options.commit !== 'after-load') { + if ( + options.lazy || + options.commit !== 'after-load' || + // if we are outside of a navigation guard, we should set the error + !router[PENDING_LOCATION_KEY] + ) { entry.stagedError = ext.error.value } // propagate error if non lazy or during SSR @@ -229,6 +235,11 @@ export function defineColadaLoader( } else { entry.staged = ext.data.value } + } else { + // TODO: add test that checks we don't set the load was discarded + console.log(`❌ Discarded old result for "${key}"`, d, ext.data.value) + ext.data.value = data.value + ext.error.value = error.value } }) .finally(() => { @@ -238,9 +249,15 @@ export function defineColadaLoader( // currentContext?.[2]?.fullPath // ) if (entry.pendingLoad === currentLoad) { + // TODO: should this be delayed with commit too? isLoading.value = false // we must run commit here so nested loaders are ready before used by their parents - if (options.lazy || options.commit === 'immediate') { + if ( + options.lazy || + options.commit === 'immediate' || + // outside of a navigation + !router[PENDING_LOCATION_KEY] + ) { // @ts-expect-error: FIXME: once pendingTo is removed from DataLoaderEntryBase entry.commit(to) } @@ -316,18 +333,6 @@ export function defineColadaLoader( > let entry = entries.get(loader) - // console.log(`-- useDataLoader called ${options.key} --`) - // console.log( - // 'router pending location', - // router[PENDING_LOCATION_KEY]?.fullPath - // ) - // console.log('target route', route.fullPath) - // console.log('has parent', !!parentEntry) - // console.log('has entry', !!entry) - // console.log('entryLatestLoad', entry?.pendingTo?.fullPath) - // console.log('is same route', entry?.pendingTo === route) - // console.log('-- END --') - if (process.env.NODE_ENV === 'development') { if (!parentEntry && !entry) { console.error( @@ -366,11 +371,34 @@ export function defineColadaLoader( const { data, error, isLoading, ext } = entry + watch(ext!.data, (newData) => { + console.log( + `👀 "${options.key}" data changed`, + newData, + entry!.pendingLoad + ) + // only if we are not in the middle of a navigation + if (!router[PENDING_LOCATION_KEY]) { + data.value = newData + } + }) + + watch(ext!.isFetching, (isFetching) => { + if (!entry!._pendingTo) { + isLoading.value = isFetching + } + }) + + watch(ext!.error, (newError) => { + if (!entry!._pendingTo) { + error.value = newError + } + }) + const useDataLoaderResult = { data, error, isLoading, - // TODO: add pinia colada stuff reload: ( // @ts-expect-error: FIXME: should be fixable to: _RouteLocationNormalizedLoaded = router.currentRoute.value @@ -381,7 +409,28 @@ export function defineColadaLoader( // @ts-expect-error: FIXME: entry!.commit(to) ), - } satisfies UseDataLoaderResult + // pinia colada + refetch: ( + // @ts-expect-error: FIXME: should be fixable + to: _RouteLocationNormalizedLoaded = router.currentRoute.value + ) => + router[APP_KEY].runWithContext(() => + load(to, router, undefined, true) + ).then(() => + // @ts-expect-error: FIXME: + entry!.commit(to) + ), + refresh: ( + // @ts-expect-error: FIXME: should be fixable + to: _RouteLocationNormalizedLoaded = router.currentRoute.value + ) => + router[APP_KEY].runWithContext(() => load(to, router)).then(() => + // @ts-expect-error: FIXME: + entry!.commit(to) + ), + isPending: ext!.isPending, + status: ext!.status, + } satisfies UseDataLoaderColadaResult // load ensures there is a pending load const promise = entry @@ -440,7 +489,11 @@ export interface DefineDataLoaderOptions< export interface DataLoaderContext extends DataLoaderContextBase {} export interface UseDataLoaderColadaResult - extends UseDataLoaderResult {} + extends UseDataLoaderResult, + Pick< + UseQueryReturn, + 'isPending' | 'refetch' | 'refresh' | 'status' + > {} export interface DataLoaderColadaEntry // TODO: remove Omit once pendingTo is removed from DataLoaderEntryBase diff --git a/src/data-fetching_new/defineLoader.ts b/src/data-fetching_new/defineLoader.ts index 7df88a6f1..e3384dc2a 100644 --- a/src/data-fetching_new/defineLoader.ts +++ b/src/data-fetching_new/defineLoader.ts @@ -19,6 +19,7 @@ import { IS_USE_DATA_LOADER_KEY, LOADER_ENTRIES_KEY, NAVIGATION_RESULTS_KEY, + PENDING_LOCATION_KEY, STAGED_NO_VALUE, } from './meta-extensions' import { IS_CLIENT, getCurrentContext, setCurrentContext } from './utils' @@ -160,7 +161,12 @@ export function defineBasicLoader( // ) if (entry.pendingLoad === currentLoad) { // in this case, commit will never be called so we should just drop the error - if (options.lazy || options.commit !== 'after-load') { + if ( + options.lazy || + options.commit !== 'after-load' || + !router[PENDING_LOCATION_KEY] + ) { + console.log(`🚨 error in "${options.key}"`, e) entry.stagedError = e } // propagate error if non lazy or during SSR @@ -175,10 +181,16 @@ export function defineBasicLoader( // `😩 restored context ${options.key}`, // currentContext?.[2]?.fullPath // ) + console.log(`🗄️ is same load: ${entry.pendingLoad === currentLoad}`) if (entry.pendingLoad === currentLoad) { isLoading.value = false // we must run commit here so nested loaders are ready before used by their parents - if (options.lazy || options.commit === 'immediate') { + if ( + options.lazy || + options.commit === 'immediate' || + // outside of navigation + !router[PENDING_LOCATION_KEY] + ) { entry.commit(to) } } diff --git a/src/data-fetching_new/navigation-guard.ts b/src/data-fetching_new/navigation-guard.ts index fc419af9e..919c125f5 100644 --- a/src/data-fetching_new/navigation-guard.ts +++ b/src/data-fetching_new/navigation-guard.ts @@ -222,10 +222,13 @@ export function setupLoaderGuard({ const entry = loader._.getEntry(router as _Router) entry.cancelPending() }) - // avoid this navigation being considered valid by the loaders - router[PENDING_LOCATION_KEY] = null } } + + if (router[PENDING_LOCATION_KEY] === to) { + // avoid this navigation being considered valid by the loaders + router[PENDING_LOCATION_KEY] = null + } }) // TODO: allow throwing a NavigationResult to skip the selectNavigationResult @@ -237,6 +240,10 @@ export function setupLoaderGuard({ if (to.meta[ABORT_CONTROLLER_KEY]) { to.meta[ABORT_CONTROLLER_KEY].abort(error) } + if (router[PENDING_LOCATION_KEY] === to) { + // avoid this navigation being considered valid by the loaders + router[PENDING_LOCATION_KEY] = null + } }) return () => { diff --git a/tests/data-loaders/tester.ts b/tests/data-loaders/tester.ts index 34ec0fea3..3b014f9bb 100644 --- a/tests/data-loaders/tester.ts +++ b/tests/data-loaders/tester.ts @@ -142,6 +142,24 @@ export function testDefineLoader( expect(data.value).toEqual(null) }) + it('can reject outside of a navigation', async () => { + const spy = vi + .fn>() + .mockResolvedValue('ko') + + const { wrapper, useData, router } = singleLoaderOneRoute( + loaderFactory({ lazy, commit, fn: spy }) + ) + // initial navigation + await router.push('/fetch') + const { error, reload } = useData() + spy.mockRejectedValueOnce(new Error('ok')) + await reload().catch(() => {}) + await vi.runAllTimersAsync() + expect(spy).toHaveBeenCalledTimes(2) + expect(error.value).toEqual(new Error('ok')) + }) + it('can return a NavigationResult without affecting initial data', async () => { let calls = 0 const spy = vi.fn(async (to: _RouteLocationNormalizedLoaded) => { @@ -265,32 +283,29 @@ export function testDefineLoader( expect(error.value).toBe(null) }) - it.todo( - 'keeps the existing error until the new data is resolved', - async () => { - let calls = 0 - const { useData, router } = singleLoaderOneRoute( - loaderFactory({ - fn: async (to) => { - if (calls++ === 0) { - throw new Error(to.query.p as string) - } else { - return to.query.p - } - }, - lazy, - commit, - }) - ) - await router.push('/fetch?p=ko').catch(() => {}) - const { data, error } = useData() - expect(error.value).toEqual(new Error('ko')) - // TODO: delay with the controlled promises to ensure the error is not null - await router.push('/fetch?p=ok').catch(() => {}) - expect(error.value).toBe(null) - expect(data.value).toBe('ok') - } - ) + it('keeps the existing error until the new data is resolved', async () => { + const l = mockedLoader({ lazy, commit }) + const { useData, router } = singleLoaderOneRoute(l.loader) + l.spy.mockResolvedValueOnce('initial') + // initiate the loader and then force an error + await router.push('/fetch?p=ko') + const { data, error, reload } = useData() + // force the error + l.spy.mockRejectedValueOnce(new Error('ok')) + await reload().catch(() => {}) + await vi.runAllTimersAsync() + expect(error.value).toEqual(new Error('ok')) + + // trigger a new navigation + router.push('/fetch?p=ok') + await vi.runAllTimersAsync() + // we still see the error + expect(error.value).toEqual(new Error('ok')) + l.resolve('resolved') + await vi.runAllTimersAsync() + // not anymore + expect(data.value).toBe('resolved') + }) }) it(`should abort the navigation if a non lazy loader throws, commit: ${commit}`, async () => { @@ -646,7 +661,7 @@ export function testDefineLoader( router.push('/fetch?p=ok') await vi.runAllTimersAsync() // it runs 3 times because in vue router, going from /fetch?p=ok to /fetch?p=ok fails right away, so the loader are never called - // We simply don't test it because it doesn't matter, what matters is what value is preserved at the end + // We simply don't test it because it doesn't matter, what matters is what value is preserved in the end // expect(spy).toHaveBeenCalledTimes(3) resolveCall1()