feat: allow lazy loaders with commit after-load

.vscode/launch.json

@@ -9,7 +9,7 @@
       "autoAttachChildProcesses": true,
       "skipFiles": [
         "<node_internals>/**",
-        // "**/node_modules/**"
+        "**/node_modules/**"
       ],
       "program": "${workspaceRoot}/node_modules/vitest/vitest.mjs",
       "args": [

src/data-fetching/defineColadaLoader.ts

@@ -298,9 +298,9 @@ export function defineColadaLoader<Data, isLazy extends boolean>(
       this.pendingTo = null
 
       // children entries cannot be committed from the navigation guard, so the parent must tell them
-      this.children.forEach((childEntry) => {
+      for (const childEntry of this.children) {
         childEntry.commit(to)
-      })
+      }
     } else {
       // console.log(` -> skipped`)
     }

src/data-fetching/defineLoader.spec.ts

@@ -28,6 +28,9 @@ import { RouteLocationNormalizedLoaded } from 'vue-router'
 
 describe(
   'defineBasicLoader',
+  // change it during dev while working on features
+  // CI might need higher timeout
+  { timeout: process.env.CI ? 1000 : 100 },
   () => {
     enableAutoUnmount(afterEach)
 
@@ -241,8 +244,5 @@ describe(
         expect(nestedPending.value).toEqual(false)
       })
     })
-  },
-  // change it during dev while working on features
-  // CI might need higher timeout
-  { timeout: process.env.CI ? 1000 : 100 }
+  }
 )

src/data-fetching/defineLoader.ts

@@ -154,6 +154,9 @@ export function defineBasicLoader<Data, isLazy extends boolean>(
     }
     // set the current context before loading so nested loaders can use it
     setCurrentContext([entry, router, to])
+    entry.staged = STAGED_NO_VALUE
+    // preserve error until data is committed
+    entry.stagedError = error.value
 
     // Promise.resolve() allows loaders to also be sync
     const currentLoad = Promise.resolve(
@@ -166,7 +169,13 @@ export function defineBasicLoader<Data, isLazy extends boolean>(
         //   `accepted: ${entry.pendingLoad === currentLoad}; data: ${d}`
         // )
         if (entry.pendingLoad === currentLoad) {
-          entry.staged = d
+          // let the navigation guard collect the result
+          if (d instanceof NavigationResult) {
+            to.meta[NAVIGATION_RESULTS_KEY]!.push(d)
+          } else {
+            entry.staged = d
+            entry.stagedError = null
+          }
         }
       })
       .catch((e) => {
@@ -178,15 +187,10 @@ 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' ||
-            !router[PENDING_LOCATION_KEY]
-          ) {
-            // console.log(`🚨 error in "${options.key}"`, e)
-            entry.stagedError = e
-          }
+          // console.log(`🚨 error in "${options.key}"`, e)
+          entry.stagedError = e
           // propagate error if non lazy or during SSR
+          // NOTE: Cannot be handled at the guard level because of nested loaders
           if (!options.lazy || !IS_CLIENT) {
             return Promise.reject(e)
           }
@@ -198,17 +202,22 @@ export function defineBasicLoader<Data, isLazy extends boolean>(
         //   `😩 restored context ${options.key}`,
         //   currentContext?.[2]?.fullPath
         // )
+        // TODO: could we replace with signal.aborted?
         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' ||
             // outside of navigation
             !router[PENDING_LOCATION_KEY]
           ) {
             entry.commit(to)
           }
+        } else {
+          // For debugging purposes and refactoring the code
+          // console.log(
+          //   to.meta[ABORT_CONTROLLER_KEY]!.signal.aborted ? '✅' : '❌'
+          // )
         }
       })
 
@@ -235,28 +244,25 @@ export function defineBasicLoader<Data, isLazy extends boolean>(
           )
         }
       }
+
       // if the entry is null, it means the loader never resolved, maybe there was an error
       if (this.staged !== STAGED_NO_VALUE) {
-        // collect navigation results instead of setting the data
-        if (this.staged instanceof NavigationResult) {
-          to.meta[NAVIGATION_RESULTS_KEY]!.push(this.staged)
-        } else {
-          this.data.value = this.staged
-        }
-      }
-      // The navigation was changed so avoid resetting the error
-      if (!(this.staged instanceof NavigationResult)) {
-        this.error.value = this.stagedError
+        this.data.value = this.staged
       }
+      // we always commit the error unless the navigation was cancelled
+      this.error.value = this.stagedError
+
+      // reset the staged values so they can't be commit
       this.staged = STAGED_NO_VALUE
-      this.stagedError = null
+      // preserve error until data is committed
+      this.stagedError = this.error.value
       this.pendingTo = null
       // we intentionally keep pendingLoad so it can be reused until the navigation is finished
 
       // children entries cannot be committed from the navigation guard, so the parent must tell them
-      this.children.forEach((childEntry) => {
+      for (const childEntry of this.children) {
         childEntry.commit(to)
-      })
+      }
     }
   }
 

src/data-fetching/navigation-guard.spec.ts

@@ -320,6 +320,10 @@ describe('navigation-guard', () => {
     expect(router.currentRoute.value.path).not.toBe('/fetch')
   })
 
+  it.todo(
+    'does not call commit for a loader if the navigation is canceled by another loader'
+  )
+
   describe('signal', () => {
     it('aborts the signal if the navigation throws', async () => {
       const router = getRouter()

src/data-fetching/navigation-guard.ts

@@ -1,5 +1,5 @@
 import type { NavigationGuard } from 'vue-router'
-import { isNavigationFailure } from 'vue-router'
+import { isNavigationFailure, NavigationFailure } from 'vue-router'
 import { effectScope, type App, type EffectScope } from 'vue'
 import {
   ABORT_CONTROLLER_KEY,
@@ -57,12 +57,12 @@ export function setupLoaderGuard({
 
   // guard to add the loaders to the meta property
   const removeLoaderGuard = router.beforeEach((to) => {
-    // Here we could check if there is a pending navigation and call abort:
+    // Abort any pending navigation. For cancelled navigations, this will happen before the `router.afterEach()`
     // if (router[PENDING_LOCATION_KEY]) {
-    //   router[PENDING_LOCATION_KEY].meta[ABORT_CONTROLLER_KEY]!.abort()
+    //   router[PENDING_LOCATION_KEY].meta[ABORT_CONTROLLER_KEY]?.abort()
     // }
-    // but we don't need it because we already abort in afterEach and onError
-    // and both are called if a new navigation happens
+    // TODO: test out if worth adding here since the afterEach will also abort the signal and with a reason parameter
+    // NOTE: in tests, it does allow to have an aborted signal faster but not in all cases
 
     // global pending location, used by nested loaders to know if they should load or not
     router[PENDING_LOCATION_KEY] = to
@@ -128,7 +128,7 @@ export function setupLoaderGuard({
 
     // TODO: could we benefit anywhere here from verifying the signal is aborted and not call the loaders at all
     // if (to.meta[ABORT_CONTROLLER_KEY]!.signal.aborted) {
-    //   return
+    //   return to.meta[ABORT_CONTROLLER_KEY]!.signal.reason ?? false
     // }
 
     // unset the context so all loaders are executed as root loaders
@@ -171,14 +171,6 @@ export function setupLoaderGuard({
       })
     ) // let the navigation go through by returning true or void
       .then((loaders) => {
-        for (const loader of loaders) {
-          if (loader) {
-            // console.log(`⬇️ Committing ${loader.name}`)
-            loader._.getEntry(router as _Router).commit(
-              to as _RouteLocationNormalizedLoaded
-            )
-          }
-        }
         // console.log(
         //   `✨ Navigation results "${to.fullPath}": [${to.meta[
         //     NAVIGATION_RESULTS_KEY
@@ -204,29 +196,39 @@ export function setupLoaderGuard({
     // console.log(
     //   `🔚 afterEach "${_from.fullPath}" -> "${to.fullPath}": ${failure?.message}`
     // )
-    // abort the signal of a failed navigation
-    // we need to check if it exists because the navigation guard that creates
-    // the abort controller could not be triggered depending on the failure
-    if (failure && to.meta[ABORT_CONTROLLER_KEY]) {
-      to.meta[ABORT_CONTROLLER_KEY].abort(failure)
-    }
-
-    if (
-      // NOTE: using a smaller version to cutoff some bytes
-      isNavigationFailure(failure, 16 /* NavigationFailureType.duplicated */)
-    ) {
-      if (router[PENDING_LOCATION_KEY]) {
-        // the PENDING_LOCATION_KEY is set at the same time the LOADER_SET_KEY is set
-        // so we know it exists
-        router[PENDING_LOCATION_KEY].meta[LOADER_SET_KEY]!.forEach((loader) => {
+    if (failure) {
+      // abort the signal of a failed navigation
+      // we need to check if it exists because the navigation guard that creates
+      // the abort controller could not be triggered depending on the failure
+      to.meta[ABORT_CONTROLLER_KEY]?.abort(failure)
+
+      if (
+        // NOTE: using a smaller version to cutoff some bytes
+        isNavigationFailure(failure, 16 /* NavigationFailureType.duplicated */)
+      ) {
+        for (const loader of to.meta[LOADER_SET_KEY]!) {
           const entry = loader._.getEntry(router as _Router)
           entry.resetPending()
-        })
+        }
+      }
+    } else {
+      for (const loader of to.meta[LOADER_SET_KEY]!) {
+        const { commit, lazy } = loader._.options
+        if (commit === 'after-load') {
+          const entry = loader._.getEntry(router as _Router)
+          // lazy loaders do not block the navigation so the navigation guard
+          // might call commit before the loader is ready
+          if (!lazy || !entry.isLoading.value) {
+            loader._.getEntry(router as _Router).commit(
+              to as _RouteLocationNormalizedLoaded
+            )
+          }
+        }
       }
     }
 
+    // avoid this navigation being considered valid by the loaders
     if (router[PENDING_LOCATION_KEY] === to) {
-      // avoid this navigation being considered valid by the loaders
       router[PENDING_LOCATION_KEY] = null
     }
   })
@@ -237,11 +239,9 @@ export function setupLoaderGuard({
   const removeOnError = router.onError((error, to) => {
     // same as with afterEach, we check if it exists because the navigation guard
     // that creates the abort controller could not be triggered depending on the error
-    if (to.meta[ABORT_CONTROLLER_KEY]) {
-      to.meta[ABORT_CONTROLLER_KEY].abort(error)
-    }
+    to.meta[ABORT_CONTROLLER_KEY]?.abort(error)
+    // avoid this navigation being considered valid by the loaders
     if (router[PENDING_LOCATION_KEY] === to) {
-      // avoid this navigation being considered valid by the loaders
       router[PENDING_LOCATION_KEY] = null
     }
   })
@@ -302,7 +302,8 @@ export type _DataLoaderRedirectResult = Exclude<
 
 /**
  * Possible values to change the result of a navigation within a loader. Can be returned from a data loader and will
- * appear in `selectNavigationResult`. If thrown, it will immediately cancel the navigation.
+ * appear in `selectNavigationResult`. If thrown, it will immediately cancel the navigation. It can only contain values
+ * that cancel the navigation.
  *
  * @example
  * ```ts

tests/data-loaders/tester.ts

@@ -21,6 +21,7 @@ import RouterViewMock from '../data-loaders/RouterViewMock.vue'
 import ComponentWithNestedLoader from '../data-loaders/ComponentWithNestedLoader.vue'
 import { dataOneSpy, dataTwoSpy } from '../data-loaders/loaders'
 import type { _RouteLocationNormalizedLoaded } from '../../src/type-extensions/routeLocation'
+import { mockWarn } from '../vitest-mock-warn'
 
 export function testDefineLoader<Context = void>(
   loaderFactory: (
@@ -60,6 +61,9 @@ export function testDefineLoader<Context = void>(
     }
   }
 
+  // TODO:
+  // mockWarn()
+
   beforeEach(async () => {
     dataOneSpy.mockClear()
     dataTwoSpy.mockClear()
@@ -874,6 +878,55 @@ export function testDefineLoader<Context = void>(
     expect(two.value).toEqual('two')
   })
 
+  it.each([new NavigationResult(false), new Error('ko')] as const)(
+    'does not commit new data if loader returns %s',
+    async (resolvedValue) => {
+      const l1 = mockedLoader({ lazy: false, commit: 'after-load', key: 'l1' })
+      const l2 = mockedLoader({ lazy: false, commit: 'after-load', key: 'l2' })
+      const router = getRouter()
+      router.addRoute({
+        name: '_test',
+        path: '/fetch',
+        component: defineComponent({
+          template: `<p></p>`,
+        }),
+        meta: {
+          loaders: [l1.loader, l2.loader],
+        },
+      })
+
+      const wrapper = mount(RouterViewMock, {
+        global: {
+          plugins: [
+            [DataLoaderPlugin, { router }],
+            ...(plugins?.(customContext!) || []),
+          ],
+        },
+      })
+      const app: App = wrapper.vm.$.appContext.app
+
+      const p = router.push('/fetch').catch(() => {})
+      await vi.runOnlyPendingTimersAsync()
+      l1.resolve('ko')
+      await vi.runOnlyPendingTimersAsync()
+      expect(l1.spy).toHaveBeenCalledTimes(1)
+      expect(l2.spy).toHaveBeenCalledTimes(1)
+      if (resolvedValue instanceof NavigationResult) {
+        l2.resolve(resolvedValue)
+      } else {
+        l2.reject(resolvedValue)
+      }
+      await vi.runOnlyPendingTimersAsync()
+      await p
+      const { data: one, error: e1 } = app.runWithContext(() => l1.loader())
+      const { data: two, error: e2 } = app.runWithContext(() => l2.loader())
+      expect(one.value).toBeUndefined()
+      expect(e1.value).toBeUndefined()
+      expect(two.value).toBeUndefined()
+      expect(e2.value).toBeUndefined()
+    }
+  )
+
   it('awaits for a lazy loader if used as a nested loader', async () => {
     const l1 = mockedLoader({ lazy: true, key: 'nested' })
     const { wrapper, app, router, useData } = singleLoaderOneRoute(