From: Eduardo San Martin Morote Date: Tue, 11 Jun 2019 12:20:05 +0000 (+0200) Subject: fix(router): avoid in-guard redirect in new navigation happened X-Git-Tag: v4.0.0-alpha.0~342 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=ae0649620cd2b5871b133e5463bf8d100d88fc2e;p=thirdparty%2Fvuejs%2Frouter.git fix(router): avoid in-guard redirect in new navigation happened --- diff --git a/__tests__/router.spec.js b/__tests__/router.spec.js index 8c8ae1eb..224748f6 100644 --- a/__tests__/router.spec.js +++ b/__tests__/router.spec.js @@ -3,8 +3,9 @@ require('./helper') const expect = require('expect') const fakePromise = require('faked-promise') const { HTML5History } = require('../src/history/html5') +const { AbstractHistory } = require('../src/history/abstract') const { Router } = require('../src/router') -const { createDom, components } = require('./utils') +const { createDom, components, tick } = require('./utils') function mockHistory() { // TODO: actually do a mock @@ -76,7 +77,7 @@ describe('Router', () => { }) describe('navigation', () => { - it('waits before navigating in an array of beforeEnter', async () => { + it('cancels pending navigations if a newer one is finished on push', async () => { const [p1, r1] = fakePromise() const [p2, r2] = fakePromise() const history = mockHistory() @@ -87,6 +88,7 @@ describe('Router', () => { path: '/a', component: components.Home, async beforeEnter(to, from, next) { + expect(from.fullPath).toBe('/') await p1 next() }, @@ -96,6 +98,7 @@ describe('Router', () => { component: components.Foo, name: 'Foo', async beforeEnter(to, from, next) { + expect(from.fullPath).toBe('/') await p2 next() }, @@ -117,6 +120,87 @@ describe('Router', () => { } expect(router.currentRoute.fullPath).toBe('/b') }) + + it('cancels pending navigations if a newer one is finished on user navigation (from history)', async () => { + const [p1, r1] = fakePromise() + const [p2, r2] = fakePromise() + const history = new AbstractHistory() + const router = new Router({ history, routes }) + // navigate first to add entries to the history stack + await router.push('/p/initial') + await router.push('/p/a') + await router.push('/p/b') + + router.beforeEach(async (to, from, next) => { + if (to.fullPath === '/p/initial') { + // because we delay navigation, we are coming from /p/b + expect(from.fullPath).toBe('/p/b') + await p1 + } else { + expect(from.fullPath).toBe('/p/b') + await p2 + } + next() + }) + + // trigger to history.back() + history.back() + history.back() + + expect(router.currentRoute.fullPath).toBe('/p/b') + // resolves the last call to history.back() first + // so we end up on /p/initial + r1() + await tick() + expect(router.currentRoute.fullPath).toBe('/p/initial') + // resolves the pending navigation, this should be cancelled + r2() + await tick() + expect(router.currentRoute.fullPath).toBe('/p/initial') + }) + + it('cancels pending in-guard navigations if a newer one is finished on user navigation (from history)', async () => { + const [p1, r1] = fakePromise() + const [p2, r2] = fakePromise() + const history = new AbstractHistory() + const router = new Router({ history, routes }) + // navigate first to add entries to the history stack + await router.push('/p/initial') + await router.push('/p/a') + await router.push('/p/b') + + router.beforeEach(async (to, from, next) => { + console.log('going to', to.fullPath, 'from', from.fullPath) + if (to.fullPath === '/p/initial') { + console.log('waiting for p1') + await p1 + console.log('done with p1') + next() + } else if (from.fullPath === '/p/b') { + console.log('waiting for p2') + await p2 + console.log('done with p2') + next('/p/other-place') + } else { + next() + } + }) + + // trigger to history.back() + history.back() + history.back() + + expect(router.currentRoute.fullPath).toBe('/p/b') + // resolves the last call to history.back() first + // so we end up on /p/initial + r1() + await tick() + expect(router.currentRoute.fullPath).toBe('/p/initial') + // resolves the pending navigation, this should be cancelled + r2() + await tick() + expect(router.currentRoute.fullPath).toBe('/p/initial') + }) }) describe('matcher', () => { diff --git a/src/router.ts b/src/router.ts index a9ee23d5..dd7c8b74 100644 --- a/src/router.ts +++ b/src/router.ts @@ -30,6 +30,8 @@ export interface RouterOptions { routes: RouteRecord[] } +type ErrorHandler = (error: any) => any + export class Router { protected history: BaseHistory private matcher: RouterMatcher @@ -38,6 +40,7 @@ export class Router { currentRoute: Readonly = START_LOCATION_NORMALIZED pendingLocation: Readonly = START_LOCATION_NORMALIZED private app: any + private errorHandlers: ErrorHandler[] = [] constructor(options: RouterOptions) { this.history = options.history @@ -50,10 +53,16 @@ export class Router { // console.log({ to, matchedRoute }) const toLocation: RouteLocationNormalized = { ...to, ...matchedRoute } + this.pendingLocation = toLocation try { await this.navigate(toLocation, this.currentRoute) + // a more recent navigation took place + if (this.pendingLocation !== toLocation) { + throw new NavigationCancelled(toLocation, this.currentRoute) + } + // accept current navigation this.currentRoute = { ...to, @@ -64,6 +73,16 @@ export class Router { // TODO: use the push/replace technique with any navigation to // preserve history when moving forward if (error instanceof NavigationGuardRedirect) { + // TODO: refactor the duplication of new NavigationCancelled by + // checking instanceof NavigationError (it's another TODO) + // a more recent navigation took place + if (this.pendingLocation !== toLocation) { + return this.triggerError( + new NavigationCancelled(toLocation, this.currentRoute) + ) + } + + // TODO: handle errors this.push(error.to) } else if (error instanceof NavigationAborted) { // TODO: test on different browsers ensure consistent behavior @@ -355,6 +374,25 @@ export class Router { } } + /** + * Add an error handler to catch errors during navigation + * TODO: return a remover like beforeEach + * @param handler error handler + */ + onError(handler: ErrorHandler): void { + this.errorHandlers.push(handler) + } + + /** + * Trigger all registered error handlers + * @param error thrown error + */ + private triggerError(error: any): void { + for (const handler of this.errorHandlers) { + handler(error) + } + } + private updateReactiveRoute() { if (!this.app) return // TODO: matched should be non enumerable and the defineProperty here shouldn't be necessary