feat: commit option

Close #201

src/data-fetching_new/createDataLoader.ts

@@ -62,11 +62,14 @@ export interface DataLoaderEntryBase<
    * Data stored in the entry.
    */
   data: Ref<_DataMaybeLazy<UnwrapRef<Data>, isLazy>>
-}
 
-export interface _UseLoaderState {
-  id: symbol
-  promise: Promise<void>
+  /**
+   * Data that was staged by a loader. This is used to avoid showing the old data while the new data is loading. Calling
+   * the internal `commit()` function will replace the data with the staged data.
+   */
+  staged: Data | null
+
+  commit(to: RouteLocationNormalizedLoaded): void
 }
 
 export function createDataLoader<Context extends DataLoaderContextBase>({
@@ -140,8 +143,24 @@ export interface DefineDataLoaderOptionsBase<isLazy extends boolean> {
    * @defaultValue `true`
    */
   server?: boolean
+
+  /**
+   * When the data should be committed to the entry. This only applies to non-lazy loaders.
+   *
+   * @see {@link DefineDataLoaderCommit}
+   * @defaultValue `'immediate'`
+   */
+  commit?: DefineDataLoaderCommit
 }
 
+/**
+ * When the data should be committed to the entry.
+ * - `immediate`: the data is committed as soon as it is loaded.
+ * - `after-load`: the data is committed after all non-lazy loaders have finished loading.
+ */
+export type DefineDataLoaderCommit = 'immediate' | 'after-load'
+// TODO: is after-load fine or is it better to have an after-navigation instead
+
 export interface DataLoaderContextBase {}
 // export interface DataLoaderContext {}
 
@@ -192,6 +211,14 @@ export interface UseDataLoaderInternals<
    * Resolved options for the loader.
    */
   options: Required<DefineDataLoaderOptionsBase<isLazy>>
+
+  /**
+   * Commits the pending data to the entry. This is called by the navigation guard when all non-lazy loaders have
+   * finished loading. It should be implemented by the loader.
+   */
+  commit(to: RouteLocationNormalizedLoaded): void
+
+  entry: DataLoaderEntryBase<isLazy, Data>
 }
 
 export type _DataMaybeLazy<Data, isLazy extends boolean = boolean> =
@@ -219,9 +246,11 @@ export interface UseDataLoaderResult<
   error: ShallowRef<Err | null> // any is simply more convenient for errors
 
   /**
-   * Refresh the data. Returns a promise that resolves when the data is refreshed.
+   * Refresh the data using the current route location. Returns a promise that resolves when the data is refreshed. You
+   * can pass a route location to refresh the data for a specific location. Note that **if an ongoing navigation is
+   * happening**, refresh will still refresh the data for the current location, which could lead to inconsistencies.
    */
-  refresh: () => Promise<void>
+  refresh: (route?: RouteLocationNormalizedLoaded) => Promise<void>
 
   /**
    * Get the promise of the current loader if there is one, returns a falsy value otherwise.

src/data-fetching_new/defineLoader.spec.ts

@@ -18,15 +18,17 @@ import { mount } from '@vue/test-utils'
 import { getRouter } from 'vue-router-mock'
 import { setupRouter } from './navigation-guard'
 import { UseDataLoader } from './createDataLoader'
-import { mockPromise } from '~/tests/utils'
+import { mockPromise, mockedLoader } from '~/tests/utils'
 import RouterViewMock from '~/tests/data-loaders/RouterViewMock.vue'
 import ComponentWithNestedLoader from '~/tests/data-loaders/ComponentWithNestedLoader.vue'
 import {
   dataOneSpy,
   dataTwoSpy,
   useDataOne,
+  useDataTwo,
 } from '~/tests/data-loaders/loaders'
 import type { RouteLocationNormalizedLoaded } from 'vue-router'
+import ComponentWithLoader from '~/tests/data-loaders/ComponentWithLoader.vue'
 
 describe('defineLoader', () => {
   let removeGuards = () => {}
@@ -134,7 +136,7 @@ describe('defineLoader', () => {
     it('does not block navigation when lazy loaded', async () => {
       const [spy, resolve, reject] = mockPromise('resolved')
       const { wrapper, useData, router } = singleLoaderOneRoute(
-        defineLoader(async () => spy(), { lazy: true })
+        defineLoader(async () => spy(), { lazy: true, key: 'lazy-test' })
       )
       expect(spy).not.toHaveBeenCalled()
       await router.push('/fetch')
@@ -321,14 +323,15 @@ describe('defineLoader', () => {
       await firstNavigation
       await secondNavigation
 
+      expect(rootCalls).toEqual(2)
+      expect(nestedCalls).toEqual(2)
+
       // only the data from the second navigation should be preserved
       const { data } = useData()
       const { data: nestedData } = app.runWithContext(() => useNestedLoader())
+
       expect(nestedData.value).toEqual('two')
       expect(data.value).toEqual('two,two')
-
-      expect(rootCalls).toEqual(2)
-      expect(nestedCalls).toEqual(2)
     })
 
     it('discards a pending load from a previous navigation that resolved later', async () => {
@@ -453,6 +456,42 @@ describe('defineLoader', () => {
       expect(dataOneSpy).toHaveBeenCalledTimes(1)
       expect(wrapper.text()).toMatchInlineSnapshot('"resolved 1resolved 1"')
     })
+
+    it('keeps the old data until all loaders are resolved', async () => {
+      const router = getRouter()
+      const l1 = mockedLoader({ commit: 'after-load' })
+      const l2 = mockedLoader({ commit: 'after-load' })
+      router.addRoute({
+        name: '_test',
+        path: '/fetch',
+        component: defineComponent({
+          template: `<p></p>`,
+        }),
+        meta: {
+          loaders: [l1.loader, l2.loader],
+        },
+      })
+      const wrapper = mount(RouterViewMock, {})
+      const app: App = wrapper.vm.$.appContext.app
+
+      const p = router.push('/fetch')
+      await vi.runOnlyPendingTimersAsync()
+      l1.resolve('one')
+      await vi.runOnlyPendingTimersAsync()
+
+      const { data: one } = app.runWithContext(() => l1.loader())
+      const { data: two } = app.runWithContext(() => l2.loader())
+      expect(l1.spy).toHaveBeenCalledTimes(1)
+      expect(l2.spy).toHaveBeenCalledTimes(1)
+
+      // it waits for both to be resolved
+      expect(one.value).toEqual(undefined)
+      l2.resolve('two')
+      await vi.runOnlyPendingTimersAsync()
+      await p
+      expect(one.value).toEqual('one')
+      expect(two.value).toEqual('two')
+    })
   })
 })
 

src/data-fetching_new/defineLoader.ts

@@ -56,6 +56,8 @@ export function defineLoader<
     ...opts,
   } as any // because of the isLazy generic
 
+  let _entry: DataLoaderEntryBase<isLazy, Awaited<P>> | undefined
+
   function load(
     to: RouteLocationNormalizedLoaded,
     router: Router,
@@ -64,16 +66,28 @@ export function defineLoader<
   ): Promise<void> {
     const entries = router[LOADER_ENTRIES_KEY]!
     if (!entries.has(loader)) {
-      entries.set(loader, createDefineLoaderEntry<boolean>(options))
+      entries.set(loader, createDefineLoaderEntry<boolean>(options, commit))
     }
-    const entry = entries.get(loader)!
+    const entry =
+      // @ts-expect-error: isLazy again
+      (_entry =
+        // ...
+        entries.get(loader)!)
 
     const { data, error, isReady, pending } = entry
 
     error.value = null
     pending.value = true
     // save the current context to restore it later
     const currentContext = getCurrentContext()
+
+    if (process.env.NODE_ENV === 'development') {
+      if (parent !== currentContext[0]) {
+        console.warn(
+          `❌👶 "${options.key}" has a different parent than the current context.`
+        )
+      }
+    }
     // set the current context before loading so nested loaders can use it
     setCurrentContext([entry, router, to])
     // console.log(
@@ -91,7 +105,7 @@ export function defineLoader<
         //   d
         // )
         if (entry.pendingLoad === currentLoad) {
-          data.value = d
+          entry.staged = d
         }
       })
       .catch((e) => {
@@ -113,16 +127,60 @@ export function defineLoader<
         // )
         if (entry.pendingLoad === currentLoad) {
           pending.value = false
+          // we must run commit here so nested loaders are ready before used by their parents
+          if (options.lazy || options.commit === 'immediate') {
+            commit(to)
+          }
         }
       })
 
+    // restore the context after the first tick to avoid lazy loaders to use their own context as parent
+    setCurrentContext(currentContext)
+
     // this still runs before the promise resolves even if loader is sync
     entry.pendingLoad = currentLoad
     // console.log(`🔶 Promise set to pendingLoad "${options.key}"`)
 
     return currentLoad
   }
 
+  function commit(to: RouteLocationNormalizedLoaded) {
+    if (!_entry) {
+      if (process.env.NODE_ENV === 'development') {
+        throw new Error(
+          `Loader "${options.key}"'s "commit()" was called before it was loaded once. This will fail in production.`
+        )
+      }
+      return
+    }
+
+    if (_entry.pendingTo === to) {
+      // console.log('👉 commit', _entry.staged)
+      if (process.env.NODE_ENV === 'development') {
+        if (_entry.staged === null) {
+          console.warn(
+            `Loader "${options.key}"'s "commit()" was called but there is no staged data.`
+          )
+        }
+      }
+      // if the entry is null, it means the loader never resolved, maybe there was an error
+      if (_entry.staged !== null) {
+        // @ts-expect-error: staged starts as null but should always be set at this point
+        _entry.data.value = _entry.staged
+      }
+      _entry.staged = null
+      _entry.pendingTo = null
+
+      // children entries cannot be committed from the navigation guard, so the parent must tell them
+      _entry.children.forEach((childEntry) => {
+        childEntry.commit(to)
+      })
+    }
+  }
+
+  // should only be called after load
+  const pendingLoad = () => _entry!.pendingLoad
+
   // @ts-expect-error: requires the internals and symbol
   const useDataLoader: // for ts
   UseDataLoader<isLazy, Awaited<P>> = () => {
@@ -154,6 +212,13 @@ export function defineLoader<
           `Some "useDataLoader()" was called outside of a component's setup or a data loader.`
         )
       }
+
+      // TODO: we can probably get around this by returning the staged data
+      if (parentEntry && options.commit === 'after-load') {
+        console.warn(
+          `🚨 "${options.key}" is used used as a nested loader and its commit option is set to "after-load" but nested loaders are always immediate to be able to give a value to their parent loader.`
+        )
+      }
     }
 
     // TODO: skip if route is not the router pending location
@@ -172,20 +237,30 @@ export function defineLoader<
 
     entry = entries.get(loader)!
 
+    if (parentEntry) {
+      if (parentEntry === entry) {
+        console.warn(`👶❌ "${options.key}" has itself as parent`)
+      }
+      console.log(`👶 "${options.key}" has parent ${parentEntry}`)
+      parentEntry.children.add(entry!)
+    }
+
     const { data, error, pending } = entry
 
     const useDataLoaderResult = {
       data,
       error,
       pending,
-      refresh: async () => {
-        return load(router.currentRoute.value, router)
-      },
-      pendingLoad: () => entry!.pendingLoad,
+      refresh: (
+        to: RouteLocationNormalizedLoaded = router.currentRoute.value
+      ) => load(to, router).then(() => commit(to)),
+      pendingLoad,
     } satisfies UseDataLoaderResult
 
     // load ensures there is a pending load
-    const promise = entry.pendingLoad!.then(() => useDataLoaderResult)
+    const promise = entry.pendingLoad!.then(() => {
+      return useDataLoaderResult
+    })
 
     return Object.assign(promise, useDataLoaderResult)
   }
@@ -197,6 +272,10 @@ export function defineLoader<
   useDataLoader._ = {
     load,
     options,
+    commit,
+    get entry() {
+      return _entry!
+    },
   }
 
   return useDataLoader
@@ -225,6 +304,7 @@ const DEFAULT_DEFINE_LOADER_OPTIONS: Required<
   lazy: false,
   key: '',
   server: true,
+  commit: 'immediate',
 }
 
 // TODO: move to a different file
@@ -233,8 +313,10 @@ export function createDefineLoaderEntry<
   Data = unknown
 >(
   options: Required<DefineDataLoaderOptions<isLazy>>,
+  commit: (to: RouteLocationNormalizedLoaded) => void,
   initialData?: Data
 ): DataLoaderEntryBase<isLazy, Data> {
+  // TODO: the scope should be passed somehow and be unique per application
   return withinScope<DataLoaderEntryBase<isLazy, Data>>(
     () =>
       ({
@@ -249,6 +331,8 @@ export function createDefineLoaderEntry<
         isReady: false,
         pendingLoad: null,
         pendingTo: null,
+        staged: null,
+        commit,
       } satisfies DataLoaderEntryBase<isLazy, Data>)
   )
 }

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

@@ -15,7 +15,7 @@ import {
 import { setCurrentContext } from './utils'
 import { getRouter } from 'vue-router-mock'
 import { setupRouter } from './navigation-guard'
-import { mockPromise } from '~/tests/utils'
+import { mockPromise, mockedLoader } from '~/tests/utils'
 import { LOADER_SET_KEY } from './symbols'
 import {
   useDataOne,
@@ -68,19 +68,6 @@ describe('navigation-guard', () => {
   const loader4 = defineLoader(async () => {})
   const loader5 = defineLoader(async () => {})
 
-  function mockedLoader(
-    // boolean is easier to handle for router mock
-    options?: DefineDataLoaderOptions<boolean>
-  ) {
-    const [spy, resolve, reject] = mockPromise('ok', 'ko')
-    return {
-      spy,
-      resolve,
-      reject,
-      loader: defineLoader(async () => await spy(), options),
-    }
-  }
-
   it('creates a set of loaders during navigation', async () => {
     const router = getRouter()
     router.addRoute({