From 6f96915911d72b9cabdbd41c4e930f4efb25cd41 Mon Sep 17 00:00:00 2001 From: Eduardo San Martin Morote Date: Thu, 23 Jan 2020 14:10:50 +0100 Subject: [PATCH] refactor: remove doInitialNavigation --- __tests__/router.spec.ts | 39 +++++++++------- jest.config.js | 1 + src/history/html5.ts | 2 +- src/index.ts | 11 ++++- src/router.ts | 98 ++++++++-------------------------------- 5 files changed, 53 insertions(+), 98 deletions(-) diff --git a/__tests__/router.spec.ts b/__tests__/router.spec.ts index c448cbba..b5abfc04 100644 --- a/__tests__/router.spec.ts +++ b/__tests__/router.spec.ts @@ -2,7 +2,12 @@ import fakePromise from 'faked-promise' import { createRouter, createMemoryHistory, createHistory } from '../src' import { NavigationCancelled } from '../src/errors' import { createDom, components, tick } from './utils' -import { RouteRecord, RouteLocation } from '../src/types' +import { + RouteRecord, + RouteLocation, + START_LOCATION_NORMALIZED, +} from '../src/types' +import { RouterHistory } from '../src/history/common' const routes: RouteRecord[] = [ { path: '/', component: components.Home }, @@ -23,6 +28,14 @@ const routes: RouteRecord[] = [ }, ] +async function newRouter({ history }: { history?: RouterHistory } = {}) { + history = history || createMemoryHistory() + const router = createRouter({ history, routes }) + await router.push('/') + + return { history, router } +} + describe('Router', () => { beforeAll(() => { createDom() @@ -31,22 +44,15 @@ describe('Router', () => { it('can be instantiated', () => { const history = createMemoryHistory() const router = createRouter({ history, routes }) - expect(router.currentRoute.value).toEqual({ - name: undefined, - fullPath: '/', - hash: '', - params: {}, - path: '/', - query: {}, - meta: {}, - }) + expect(router.currentRoute.value).toEqual(START_LOCATION_NORMALIZED) }) // TODO: should do other checks not based on history implem - it.skip('takes browser location', () => { + it.skip('takes browser location', async () => { const history = createMemoryHistory() history.replace('/search?q=dog#footer') - const router = createRouter({ history, routes }) + const { router } = await newRouter({ history }) + await router.push(history.location) expect(router.currentRoute).toEqual({ fullPath: '/search?q=dog#footer', hash: '#footer', @@ -57,8 +63,7 @@ describe('Router', () => { }) it('calls history.push with router.push', async () => { - const history = createMemoryHistory() - const router = createRouter({ history, routes }) + const { router, history } = await newRouter() jest.spyOn(history, 'push') await router.push('/foo') expect(history.push).toHaveBeenCalledTimes(1) @@ -72,7 +77,7 @@ describe('Router', () => { it('calls history.replace with router.replace', async () => { const history = createMemoryHistory() - const router = createRouter({ history, routes }) + const { router } = await newRouter({ history }) jest.spyOn(history, 'replace') await router.replace('/foo') expect(history.replace).toHaveBeenCalledTimes(1) @@ -85,8 +90,7 @@ describe('Router', () => { }) it('can pass replace option to push', async () => { - const history = createMemoryHistory() - const router = createRouter({ history, routes }) + const { router, history } = await newRouter() jest.spyOn(history, 'replace') await router.push({ path: '/foo', replace: true }) expect(history.replace).toHaveBeenCalledTimes(1) @@ -153,6 +157,7 @@ describe('Router', () => { const [p2, r2] = fakePromise() const history = createMemoryHistory() const router = createRouter({ history, routes }) + // navigate first to add entries to the history stack await router.push('/foo') await router.push('/p/a') diff --git a/jest.config.js b/jest.config.js index 74f91374..95862540 100644 --- a/jest.config.js +++ b/jest.config.js @@ -13,5 +13,6 @@ module.exports = { 'src/consola.ts', ], testMatch: ['/__tests__/**/*.spec.ts?(x)'], + watchPathIgnorePatterns: ['/node_modules'], testEnvironment: 'node', } diff --git a/src/history/html5.ts b/src/history/html5.ts index e58b8a3f..8d98b167 100644 --- a/src/history/html5.ts +++ b/src/history/html5.ts @@ -259,7 +259,7 @@ function useHistoryStateNavigation(base: string) { } export default function createHistory(base: string = ''): RouterHistory { - if ('scrollRestoration' in history) { + if ('scrollRestoration' in window.history) { history.scrollRestoration = 'manual' } diff --git a/src/index.ts b/src/index.ts index f4dfc0ae..943a7a9d 100644 --- a/src/index.ts +++ b/src/index.ts @@ -5,7 +5,10 @@ import createMemoryHistory from './history/memory' import createHashHistory from './history/hash' import View from './components/View' import Link from './components/Link' -import { RouteLocationNormalized } from './types' +import { + RouteLocationNormalized, + START_LOCATION_NORMALIZED as START_LOCATION, +} from './types' declare module '@vue/runtime-core' { function inject(name: 'router'): Router @@ -30,12 +33,15 @@ export function RouterPlugin(app: App, router: Router) { app.component('RouterView', View as any) let started = false + // TODO: can we use something that isn't a mixin? app.mixin({ beforeCreate() { if (!started) { router.setActiveApp(this) - router.doInitialNavigation().catch(err => { + // TODO: this initial navigation is only necessary on client, on server it doesn't make sense + // because it will create an extra unecessary navigation and could lead to problems + router.push(router.history.location).catch(err => { console.error('Unhandled error', err) }) started = true @@ -56,4 +62,5 @@ export { createRouter, RouteLocationNormalized, Router, + START_LOCATION, } diff --git a/src/router.ts b/src/router.ts index 0d1d2c40..abbf7032 100644 --- a/src/router.ts +++ b/src/router.ts @@ -17,7 +17,6 @@ import { stringifyURL, normalizeQuery, HistoryLocationNormalized, - START, } from './history/common' import { ScrollToPosition, @@ -55,6 +54,7 @@ export interface RouterOptions { } export interface Router { + history: RouterHistory currentRoute: Ref resolve(to: RouteLocation): RouteLocationNormalized @@ -63,7 +63,6 @@ export interface Router { replace(to: RouteLocation): Promise // TODO: find a way to remove it - doInitialNavigation(): Promise setActiveApp(vm: TODO): void beforeEach(guard: NavigationGuard): ListenerRemover @@ -232,12 +231,17 @@ export function createRouter({ ) return currentRoute.value + const isFirstNavigation = + currentRoute.value === START_LOCATION_NORMALIZED && + to === history.location + const toLocation: RouteLocationNormalized = location pendingLocation = toLocation // trigger all guards, throw if navigation is rejected try { await navigate(toLocation, currentRoute.value) } catch (error) { + if (isFirstNavigation) markAsReady(error) if (NavigationGuardRedirect.is(error)) { // push was called while waiting in guards if (pendingLocation !== toLocation) { @@ -261,21 +265,28 @@ export function createRouter({ // push was called while waiting in guards if (pendingLocation !== toLocation) { - throw new NavigationCancelled(toLocation, currentRoute.value) + const error = new NavigationCancelled(toLocation, currentRoute.value) + // TODO: refactor errors to be more lightweight + if (isFirstNavigation) markAsReady(error) + throw error } // change URL - if (to.replace === true) history.replace(url) - else history.push(url) + if (!isFirstNavigation) { + if (to.replace === true) history.replace(url) + else history.push(url) + } const from = currentRoute.value currentRoute.value = markNonReactive(toLocation) - updateReactiveRoute() - handleScroll(toLocation, from).catch(err => triggerError(err, false)) + if (!isFirstNavigation) + handleScroll(toLocation, from).catch(err => triggerError(err, false)) // navigation is confirmed, call afterGuards for (const guard of afterGuards) guard(toLocation, from) + markAsReady() + return currentRoute.value } @@ -380,7 +391,6 @@ export function createRouter({ ...to, ...matchedRoute, }) - updateReactiveRoute() // TODO: refactor with a state getter // const { scroll } = history.state const { state } = window.history @@ -451,16 +461,6 @@ export function createRouter({ if (shouldThrow) throw error } - function updateReactiveRoute() { - if (!app) return - // TODO: matched should be non enumerable and the defineProperty here shouldn't be necessary - const route = { ...currentRoute.value } - Object.defineProperty(route, 'matched', { enumerable: false }) - // @ts-ignore - app._route = Object.freeze(route) - markAsReady() - } - function isReady(): Promise { if (ready && currentRoute.value !== START_LOCATION_NORMALIZED) return Promise.resolve() @@ -470,7 +470,7 @@ export function createRouter({ } function markAsReady(err?: any): void { - if (ready || currentRoute.value === START_LOCATION_NORMALIZED) return + if (ready) return ready = true for (const [resolve] of onReadyCbs) { // TODO: is this okay? @@ -484,63 +484,6 @@ export function createRouter({ onReadyCbs = [] } - async function doInitialNavigation(): Promise { - // let the user call replace or push on SSR - if (history.location === START) return - // TODO: refactor code that was duplicated from push method - const toLocation: RouteLocationNormalized = resolveLocation( - history.location, - currentRoute.value - ) - - pendingLocation = toLocation - // trigger all guards, throw if navigation is rejected - try { - await navigate(toLocation, currentRoute.value) - } catch (error) { - markAsReady(error) - if (NavigationGuardRedirect.is(error)) { - // push was called while waiting in guards - if (pendingLocation !== toLocation) { - // TODO: trigger onError as well - throw new NavigationCancelled(toLocation, currentRoute.value) - } - // TODO: setup redirect stack - await push(error.to) - return - } else { - // TODO: write tests - // triggerError as well - if (pendingLocation !== toLocation) { - // TODO: trigger onError as well - throw new NavigationCancelled(toLocation, currentRoute.value) - } - - // this throws, so nothing ahead happens - triggerError(error) - } - } - - // push was called while waiting in guards - if (pendingLocation !== toLocation) { - const error = new NavigationCancelled(toLocation, currentRoute.value) - markAsReady(error) - throw error - } - - // NOTE: here we removed the pushing to history part as the history - // already contains current location - - const from = currentRoute.value - currentRoute.value = markNonReactive(toLocation) - updateReactiveRoute() - - // navigation is confirmed, call afterGuards - for (const guard of afterGuards) guard(toLocation, from) - - markAsReady() - } - async function handleScroll( to: RouteLocationNormalized, from: RouteLocationNormalized, @@ -556,7 +499,6 @@ export function createRouter({ function setActiveApp(vm: TODO) { app = vm - updateReactiveRoute() } const router: Router = { @@ -570,7 +512,7 @@ export function createRouter({ onError, isReady, - doInitialNavigation, + history, setActiveApp, } -- 2.47.3