From b14b020cc13659cafd1dc1c77cfd325a572eb34e Mon Sep 17 00:00:00 2001 From: Eduardo San Martin Morote Date: Tue, 20 Feb 2024 18:28:41 +0100 Subject: [PATCH] feat: allow lazy loaders with commit after-load --- .vscode/launch.json | 2 +- src/data-fetching/defineColadaLoader.ts | 4 +- src/data-fetching/defineLoader.spec.ts | 8 +-- src/data-fetching/defineLoader.ts | 52 ++++++++------- src/data-fetching/navigation-guard.spec.ts | 4 ++ src/data-fetching/navigation-guard.ts | 73 +++++++++++----------- tests/data-loaders/tester.ts | 53 ++++++++++++++++ 7 files changed, 130 insertions(+), 66 deletions(-) diff --git a/.vscode/launch.json b/.vscode/launch.json index d3294e47d..d8eb240da 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -9,7 +9,7 @@ "autoAttachChildProcesses": true, "skipFiles": [ "/**", - // "**/node_modules/**" + "**/node_modules/**" ], "program": "${workspaceRoot}/node_modules/vitest/vitest.mjs", "args": [ diff --git a/src/data-fetching/defineColadaLoader.ts b/src/data-fetching/defineColadaLoader.ts index eada1af19..2af77c2b6 100644 --- a/src/data-fetching/defineColadaLoader.ts +++ b/src/data-fetching/defineColadaLoader.ts @@ -298,9 +298,9 @@ export function defineColadaLoader( 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`) } diff --git a/src/data-fetching/defineLoader.spec.ts b/src/data-fetching/defineLoader.spec.ts index c4ac5d11a..113b38664 100644 --- a/src/data-fetching/defineLoader.spec.ts +++ b/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 } + } ) diff --git a/src/data-fetching/defineLoader.ts b/src/data-fetching/defineLoader.ts index a993d20ba..f2886b5dc 100644 --- a/src/data-fetching/defineLoader.ts +++ b/src/data-fetching/defineLoader.ts @@ -154,6 +154,9 @@ export function defineBasicLoader( } // 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( // `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( // ) 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( // `😩 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( ) } } + // 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) - }) + } } } diff --git a/src/data-fetching/navigation-guard.spec.ts b/src/data-fetching/navigation-guard.spec.ts index 11583d712..b20c82e3e 100644 --- a/src/data-fetching/navigation-guard.spec.ts +++ b/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() diff --git a/src/data-fetching/navigation-guard.ts b/src/data-fetching/navigation-guard.ts index b6cfc8a15..731f937f7 100644 --- a/src/data-fetching/navigation-guard.ts +++ b/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 diff --git a/tests/data-loaders/tester.ts b/tests/data-loaders/tester.ts index c38d78d86..52132e119 100644 --- a/tests/data-loaders/tester.ts +++ b/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( loaderFactory: ( @@ -60,6 +61,9 @@ export function testDefineLoader( } } + // TODO: + // mockWarn() + beforeEach(async () => { dataOneSpy.mockClear() dataTwoSpy.mockClear() @@ -874,6 +878,55 @@ export function testDefineLoader( 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: `

`, + }), + 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(