From 56b2a4d6e35aa8ef5d45f4ee914ab86a7577f8e5 Mon Sep 17 00:00:00 2001 From: Eduardo San Martin Morote Date: Tue, 8 Aug 2023 14:42:01 +0200 Subject: [PATCH] feat: commit option Close posva/unplugin-vue-router#201 --- src/data-fetching_new/createDataLoader.ts | 41 +++++-- src/data-fetching_new/defineLoader.spec.ts | 49 ++++++++- src/data-fetching_new/defineLoader.ts | 100 ++++++++++++++++-- .../navigation-guard.spec.ts | 15 +-- src/data-fetching_new/navigation-guard.ts | 28 ++++- src/data-fetching_new/utils.ts | 3 +- 6 files changed, 198 insertions(+), 38 deletions(-) diff --git a/src/data-fetching_new/createDataLoader.ts b/src/data-fetching_new/createDataLoader.ts index 8568471e4..5a48ace92 100644 --- a/src/data-fetching_new/createDataLoader.ts +++ b/src/data-fetching_new/createDataLoader.ts @@ -62,11 +62,14 @@ export interface DataLoaderEntryBase< * Data stored in the entry. */ data: Ref<_DataMaybeLazy, isLazy>> -} -export interface _UseLoaderState { - id: symbol - promise: Promise + /** + * 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({ @@ -140,8 +143,24 @@ export interface DefineDataLoaderOptionsBase { * @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> + + /** + * 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 } export type _DataMaybeLazy = @@ -219,9 +246,11 @@ export interface UseDataLoaderResult< error: ShallowRef // 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 + refresh: (route?: RouteLocationNormalizedLoaded) => Promise /** * Get the promise of the current loader if there is one, returns a falsy value otherwise. diff --git a/src/data-fetching_new/defineLoader.spec.ts b/src/data-fetching_new/defineLoader.spec.ts index df5e9a19f..25d5eaf65 100644 --- a/src/data-fetching_new/defineLoader.spec.ts +++ b/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: `

`, + }), + 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') + }) }) }) diff --git a/src/data-fetching_new/defineLoader.ts b/src/data-fetching_new/defineLoader.ts index e65681316..809c1f8cd 100644 --- a/src/data-fetching_new/defineLoader.ts +++ b/src/data-fetching_new/defineLoader.ts @@ -56,6 +56,8 @@ export function defineLoader< ...opts, } as any // because of the isLazy generic + let _entry: DataLoaderEntryBase> | undefined + function load( to: RouteLocationNormalizedLoaded, router: Router, @@ -64,9 +66,13 @@ export function defineLoader< ): Promise { const entries = router[LOADER_ENTRIES_KEY]! if (!entries.has(loader)) { - entries.set(loader, createDefineLoaderEntry(options)) + entries.set(loader, createDefineLoaderEntry(options, commit)) } - const entry = entries.get(loader)! + const entry = + // @ts-expect-error: isLazy again + (_entry = + // ... + entries.get(loader)!) const { data, error, isReady, pending } = entry @@ -74,6 +80,14 @@ export function defineLoader< 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,9 +127,16 @@ 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}"`) @@ -123,6 +144,43 @@ export function defineLoader< 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> = () => { @@ -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>, + commit: (to: RouteLocationNormalizedLoaded) => void, initialData?: Data ): DataLoaderEntryBase { + // TODO: the scope should be passed somehow and be unique per application return withinScope>( () => ({ @@ -249,6 +331,8 @@ export function createDefineLoaderEntry< isReady: false, pendingLoad: null, pendingTo: null, + staged: null, + commit, } satisfies DataLoaderEntryBase) ) } diff --git a/src/data-fetching_new/navigation-guard.spec.ts b/src/data-fetching_new/navigation-guard.spec.ts index 11563bbdd..b8caafa82 100644 --- a/src/data-fetching_new/navigation-guard.spec.ts +++ b/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 - ) { - 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({ diff --git a/src/data-fetching_new/navigation-guard.ts b/src/data-fetching_new/navigation-guard.ts index 57a8c1aa7..e4f368ca4 100644 --- a/src/data-fetching_new/navigation-guard.ts +++ b/src/data-fetching_new/navigation-guard.ts @@ -5,6 +5,7 @@ import { PENDING_LOCATION_KEY, } from './symbols' import { IS_CLIENT, isDataLoader, setCurrentContext } from './utils' +import { UseDataLoader } from './createDataLoader' /** * Setups the different Navigation Guards to collect the data loaders from the route records and then to execute them. @@ -98,21 +99,40 @@ export function setupRouter(router: Router) { setCurrentContext([]) return Promise.all( loaders.map((loader) => { - if (!loader._.options.server && !IS_CLIENT) { + const { commit, server, lazy } = loader._.options + // do not run on the server if specified + if (!server && !IS_CLIENT) { return } - const ret = loader._.load(to, router) + const ret = loader._.load(to, router).then(() => { + if (lazy || commit === 'immediate') { + // TODO: refactor, it should be done here, it's better + // loader._.entry.commit(to) + } else if (commit === 'after-load') { + return loader + } + }) // on client-side, lazy loaders are not awaited, but on server they are - return IS_CLIENT && loader._.options.lazy ? undefined : ret + return IS_CLIENT && lazy + ? undefined + : // return the non-lazy loader to commit changes after all loaders are done + ret }) ) // let the navigation go through by returning true or void - .then(() => { + .then((loaders) => { + for (const loader of loaders) { + if (loader) { + // console.log(`⬇️ Committing ${loader.name}`) + loader._.entry.commit(to) + } + } // TODO: // reset the initial state as it can only be used once // initialData = undefined // NOTE: could this be dev only? // isFetched = true }) + // TODO: handle errors and navigation failures? }) return () => { diff --git a/src/data-fetching_new/utils.ts b/src/data-fetching_new/utils.ts index 7e036fa4d..6172de19b 100644 --- a/src/data-fetching_new/utils.ts +++ b/src/data-fetching_new/utils.ts @@ -65,6 +65,7 @@ export function withLoaderContext

>(promise: P): P { * Object and promise of the object itself. Used when we can await some of the properties of an object to be loaded. * @internal */ -export type _PromiseMerged = T & Promise +export type _PromiseMerged = RawType & + Promise export const IS_CLIENT = typeof window !== 'undefined'