feat: cancel pending enter/change hooks on location change

Author: alisd23

modules/TransitionUtils.js

@@ -1,31 +1,49 @@
 import { loopAsync } from './AsyncUtils'
 
-function createTransitionHook(hook, route, asyncArity) {
-  return function (...args) {
+class PendingHooks {
+  hooks = []
+  add = hook => this.hooks.push(hook)
+  remove = hook => this.hooks = this.hooks.filter(h => h !== hook)
+  has = hook => this.hooks.indexOf(hook) !== -1
+  clear = () => this.hooks = []
+}
+
+const enterHooks = new PendingHooks()
+const changeHooks = new PendingHooks()
+
+function createTransitionHook(hook, route, asyncArity, pendingHooks) {
+  const transitionHook = (...args) => {
     hook.apply(route, args)
 
     if (hook.length < asyncArity) {
       let callback = args[args.length - 1]
+      // Add synchronous hook to pendingHooks (gets removed instantly later)
+      pendingHooks.add(transitionHook)
       // Assume hook executes synchronously and
       // automatically call the callback.
       callback()
     }
   }
+
+  if (hook.length >= asyncArity) {
+    pendingHooks.add(transitionHook)
+  }
+
+  return transitionHook
 }
 
 function getEnterHooks(routes) {
   return routes.reduce(function (hooks, route) {
     if (route.onEnter)
-      hooks.push(createTransitionHook(route.onEnter, route, 3))
-
+      hooks.push(createTransitionHook(route.onEnter, route, 3, enterHooks))
     return hooks
   }, [])
 }
 
 function getChangeHooks(routes) {
   return routes.reduce(function (hooks, route) {
     if (route.onChange)
-      hooks.push(createTransitionHook(route.onChange, route, 4))
+      hooks.push(createTransitionHook(route.onChange, route, 4, changeHooks))
     return hooks
   }, [])
 }
@@ -63,9 +81,16 @@ function runTransitionHooks(length, iter, callback) {
  * which could lead to a non-responsive UI if the hook is slow.
  */
 export function runEnterHooks(routes, nextState, callback) {
+  enterHooks.clear()
   const hooks = getEnterHooks(routes)
   return runTransitionHooks(hooks.length, (index, replace, next) => {
-    hooks[index](nextState, replace, next)
+    const wrappedNext = () => {
+      if (enterHooks.has(hooks[index])) {
+        next()
+        enterHooks.remove(hooks[index])
+      }
+    }
+    hooks[index](nextState, replace, wrappedNext)
   }, callback)
 }
 
@@ -80,9 +105,16 @@ export function runEnterHooks(routes, nextState, callback) {
  * which could lead to a non-responsive UI if the hook is slow.
  */
 export function runChangeHooks(routes, state, nextState, callback) {
+  changeHooks.clear()
   const hooks = getChangeHooks(routes)
   return runTransitionHooks(hooks.length, (index, replace, next) => {
-    hooks[index](state, nextState, replace, next)
+    const wrappedNext = () => {
+      if (changeHooks.has(hooks[index])) {
+        next()
+        changeHooks.remove(hooks[index])
+      }
+    }
+    hooks[index](state, nextState, replace, wrappedNext)
   }, callback)
 }
 

modules/__tests__/transitionHooks-test.js

@@ -5,6 +5,7 @@ import createHistory from '../createMemoryHistory'
 import { routerShape } from '../PropTypes'
 import execSteps from './execSteps'
 import Router from '../Router'
+import Route from '../Route'
 
 describe('When a router enters a branch', function () {
   let
@@ -374,5 +375,72 @@ describe('When a router enters a branch', function () {
       })
     })
   })
+})
+
+describe('Changing location', () => {
+  let node, onEnterSpy, onChangeSpy
+
+  const Text = text => () => <p>{text}</p>
+  const noop = () => {}
 
+  const onEnter = (state, replace, cb) => {
+    setTimeout(() => {
+      onEnterSpy()
+      replace('/bar')
+      cb()
+    })
+  }
+  const onChange = (prevState, nextState, replace, cb) => {
+    setTimeout(() => {
+      onChangeSpy()
+      replace('/bar')
+      cb()
+    })
+  }
+  const createRoutes = ({ enter, change }) => [
+    <Route path="/" onChange={change ? onChange : noop} component={Text('Home')}>
+      <Route path="child1" component={Text('Child1')} />
+      <Route path="child2" component={Text('Child2')} />
+    </Route>,
+    <Route path="/foo" onEnter={enter ? onEnter : noop} component={Text('Foo')} />,
+    <Route path="/bar" component={Text('Bar')} />
+  ]
+
+  beforeEach(() => {
+    node = document.createElement('div')
+    onEnterSpy = expect.createSpy()
+    onChangeSpy = expect.createSpy()
+  })
+
+  it('cancels pending async onEnter hook', (done) => {
+    const history = createHistory('/')
+    const routes = createRoutes({ enter: true })
+
+    render(<Router history={history} routes={routes} />, node, () => {
+      history.push('/foo')
+      history.push('/')
+      expect(onEnterSpy.calls.length).toEqual(0)
+      setTimeout(() => {
+        expect(onEnterSpy.calls.length).toEqual(1)
+        expect(node.innerHTML).toContain('Home')
+        done()
+      })
+    })
+  })
+
+  it('cancels pending async onChange hook', (done) => {
+    const history = createHistory('/')
+    const routes = createRoutes({ change: true })
+
+    render(<Router history={history} routes={routes} />, node, () => {
+      history.push('/child1')
+      history.push('/bar')
+      expect(onChangeSpy.calls.length).toEqual(0)
+      setTimeout(() => {
+        expect(onChangeSpy.calls.length).toEqual(1)
+        expect(node.innerHTML).toContain('Bar')
+        done()
+      })
+    })
+  })
 })