fix: allow errors outside of navigation

src/data-fetching_new/createDataLoader.ts

@@ -191,6 +191,8 @@ export interface DefineDataLoader<Context extends DataLoaderContextBase> {
   ): UseDataLoader<isLazy, Data>
 }
 
+// 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}

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<Data, isLazy extends boolean>(
       })
     }
 
-    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<Data, isLazy extends boolean>(
             //   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<Data, isLazy extends boolean>(
           } 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<Data, isLazy extends boolean>(
         //   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<Data, isLazy extends boolean>(
     >
     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<Data, isLazy extends boolean>(
 
     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<Data, isLazy extends boolean>(
           // @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<boolean, unknown>
 
     // 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<isLazy extends boolean, Data>
-  extends UseDataLoaderResult<isLazy, Data> {}
+  extends UseDataLoaderResult<isLazy, Data>,
+    Pick<
+      UseQueryReturn<Data, any>,
+      'isPending' | 'refetch' | 'refresh' | 'status'
+    > {}
 
 export interface DataLoaderColadaEntry<isLazy extends boolean, Data>
   // TODO: remove Omit once pendingTo is removed from DataLoaderEntryBase

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<Data, isLazy extends boolean>(
         // )
         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<Data, isLazy extends boolean>(
         //   `😩 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)
           }
         }

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 () => {

tests/data-loaders/tester.ts

@@ -142,6 +142,24 @@ export function testDefineLoader<Context = void>(
           expect(data.value).toEqual(null)
         })
 
+        it('can reject outside of a navigation', async () => {
+          const spy = vi
+            .fn<unknown[], Promise<unknown>>()
+            .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<Context = void>(
           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<Context = void>(
     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()