feat: cancel pending enter/change hooks on location change (#4063)

* feat: cancel pending enter/change hooks on location change

* chore: fix npm build scripts on windows

modules/TransitionUtils.js

@@ -1,31 +1,47 @@
 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 isSync = hook.length < asyncArity
+
+  const transitionHook = (...args) => {
     hook.apply(route, args)
 
-    if (hook.length < asyncArity) {
+    if (isSync) {
       let callback = args[args.length - 1]
       // Assume hook executes synchronously and
       // automatically call the callback.
       callback()
     }
   }
+
+  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 +79,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 +103,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()
+      })
+    })
+  })
 })

package.json

@@ -16,8 +16,8 @@
   "bugs": "https://github.com/ReactTraining/react-router/issues",
   "scripts": {
     "build": "npm run build-cjs && npm run build-es",
-    "build-cjs": "rimraf lib && cross-env BABEL_ENV=cjs babel ./modules -d lib --ignore '__tests__'",
-    "build-es": "rimraf es && cross-env BABEL_ENV=es babel ./modules -d es --ignore '__tests__'",
+    "build-cjs": "rimraf lib && cross-env BABEL_ENV=cjs babel ./modules -d lib --ignore __tests__",
+    "build-es": "rimraf es && cross-env BABEL_ENV=es babel ./modules -d es --ignore __tests__",
     "build-umd": "cross-env NODE_ENV=development webpack modules/index.js umd/ReactRouter.js",
     "build-min": "cross-env NODE_ENV=production webpack -p modules/index.js umd/ReactRouter.min.js",
     "lint": "eslint examples modules scripts tools *.js",