Skip to content

Commit

Permalink
feat(router): remove partial Promise from router.go
Browse files Browse the repository at this point in the history
BREAKING CHANGE: The `router.go()` methods doesn't return anything
(like in Vue 3) anymore. The existing implementation was wrong as it
would resolve the promise for the following navigation if `router.go()`
was called with something that wasn't possible e.g. `router.go(-20)`
right after entering the application would not do anything. Even worse,
the promise returned by that call would resolve **after the next
navigation**. There is no proper native API to implement this
promise-based api properly, but one can write a version that should work
in most scenarios by setting up multiple hooks right before calling
`router.go()`:
```js
export function go(delta) {
  return new Promise((resolve, reject) => {
    function popStateListener() {
      clearTimeout(timeout)
    }
    window.addEventListener('popstate', popStateListener)

    function clearHooks() {
      removeAfterEach()
      removeOnError()
      window.removeEventListener('popstate', popStateListener)
    }

    // if the popstate event is not called, consider this a failure
    const timeout = setTimeout(() => {
      clearHooks()
      reject(new Error('Failed to use router.go()'))
      // It's unclear of what value would always work here
    }, 10)

    setImmediate

    const removeAfterEach = router.afterEach((_to, _from, failure) => {
      clearHooks()
      resolve(failure)
    })
    const removeOnError = router.onError(err => {
      clearHooks()
      reject(err)
    })

    router.go(delta)
  })
}
```
  • Loading branch information
posva committed Sep 11, 2020
1 parent b58e0aa commit 6ed6eee
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 45 deletions.
17 changes: 0 additions & 17 deletions __tests__/router.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -175,23 +175,6 @@ describe('Router', () => {
})
})

it('can await router.go', async () => {
const { router } = await newRouter()
await router.push('/foo')
let currentRoute = router.currentRoute.value
const [p1, r1] = fakePromise()
router.beforeEach(async (to, from, next) => {
await p1
next()
})
let p = router.go(-1)
expect(router.currentRoute.value).toBe(currentRoute)
r1()
// resolves to undefined as a working navigation
await expect(p).resolves.toBe(undefined)
expect(router.currentRoute.value).not.toBe(currentRoute)
})

it('can pass replace option to push', async () => {
const { router, history } = await newRouter()
jest.spyOn(history, 'replace')
Expand Down
36 changes: 8 additions & 28 deletions src/router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -233,25 +233,22 @@ export interface Router {
replace(to: RouteLocationRaw): Promise<NavigationFailure | void | undefined>
/**
* Go back in history if possible by calling `history.back()`. Equivalent to
* `router.go(-1)`. Returns a Promise. See the limitations at
* {@link Router.go}.
* `router.go(-1)`.
*/
back(): Promise<NavigationFailure | void | undefined>
back(): ReturnType<Router['go']>
/**
* Go forward in history if possible by calling `history.forward()`.
* Equivalent to `router.go(1)`. Returns a Promise. See the limitations at
* {@link Router.go}.
* Equivalent to `router.go(1)`.
*/
forward(): Promise<NavigationFailure | void | undefined>
forward(): ReturnType<Router['go']>
/**
* Allows you to move forward or backward through the history. Returns a
* Promise that resolves when the navigation finishes. If it wasn't possible
* to go back, the promise never resolves or rejects
* Allows you to move forward or backward through the history. Calls
* `history.go()`.
*
* @param delta - The position in the history to which you want to move,
* relative to the current page
*/
go(delta: number): Promise<NavigationFailure | void | undefined>
go(delta: number): void

/**
* Add a navigation guard that executes before any navigation. Returns a
Expand Down Expand Up @@ -1025,24 +1022,7 @@ export function createRouter(options: RouterOptions): Router {
.catch(triggerError)
}

function go(delta: number) {
return new Promise<NavigationFailure | void | undefined>(
(resolve, reject) => {
let removeError = errorHandlers.add(err => {
removeError()
removeAfterEach()
reject(err)
})
let removeAfterEach = afterGuards.add((_to, _from, failure) => {
removeError()
removeAfterEach()
resolve(failure)
})

routerHistory.go(delta)
}
)
}
const go = (delta: number) => routerHistory.go(delta)

let started: boolean | undefined
const installedApps = new Set<App>()
Expand Down

0 comments on commit 6ed6eee

Please sign in to comment.