Merge pull request #2748 from taion/route-leave-hook

Tear down route leave hooks upon leaving route

Author: ryanflorence

modules/__tests__/transitionHooks-test.js

@@ -7,33 +7,48 @@ import execSteps from './execSteps'
 import Router from '../Router'
 
 describe('When a router enters a branch', function () {
+  let
+    node, leaveHookSpy, removeLeaveHook,
+    DashboardRoute, NewsFeedRoute, InboxRoute, RedirectToInboxRoute, MessageRoute,
+    routes
 
-  class Dashboard extends Component {
-    render() {
-      return (
-        <div className="Dashboard">
-          <h1>The Dashboard</h1>
-          {this.props.children}
-        </div>
-      )
+  beforeEach(function () {
+    node = document.createElement('div')
+    leaveHookSpy = expect.createSpy()
+
+    class Dashboard extends Component {
+      render() {
+        return (
+          <div className="Dashboard">
+            <h1>The Dashboard</h1>
+            {this.props.children}
+          </div>
+        )
+      }
     }
-  }
 
-  class NewsFeed extends Component {
-    render() {
-      return <div>News</div>
+    class NewsFeed extends Component {
+      componentWillMount() {
+        removeLeaveHook = this.context.router.addRouteLeaveHook(
+          this.props.route,
+          () => leaveHookSpy() // Break reference equality.
+        )
+      }
+
+      render() {
+        return <div>News</div>
+      }
     }
-  }
 
-  class Inbox extends Component {
-    render() {
-      return <div>Inbox</div>
+    NewsFeed.contextTypes = {
+      router: React.PropTypes.object.isRequired
     }
-  }
 
-  let node, DashboardRoute, NewsFeedRoute, InboxRoute, RedirectToInboxRoute, MessageRoute, routes
-  beforeEach(function () {
-    node = document.createElement('div')
+    class Inbox extends Component {
+      render() {
+        return <div>Inbox</div>
+      }
+    }
 
     NewsFeedRoute = {
       path: 'news',
@@ -121,6 +136,35 @@ describe('When a router enters a branch', function () {
     })
   })
 
+  it('calls the route leave hooks when leaving the route', function (done) {
+    const history = createHistory('/news')
+
+    // Stub this function to exercise the code path.
+    history.listenBeforeUnload = () => (() => {})
+
+    render(<Router history={history} routes={routes}/>, node, function () {
+      expect(leaveHookSpy.calls.length).toEqual(0)
+      history.push('/inbox')
+      expect(leaveHookSpy.calls.length).toEqual(1)
+      history.push('/news')
+      expect(leaveHookSpy.calls.length).toEqual(1)
+      history.push('/inbox')
+      expect(leaveHookSpy.calls.length).toEqual(2)
+      done()
+    })
+  })
+
+  it('does not call removed route leave hooks', function (done) {
+    const history = createHistory('/news')
+
+    render(<Router history={history} routes={routes}/>, node, function () {
+      removeLeaveHook()
+      history.push('/inbox')
+      expect(leaveHookSpy).toNotHaveBeenCalled()
+      done()
+    })
+  })
+
   describe('and one of the transition hooks navigates to another route', function () {
     it('immediately transitions to the new route', function (done) {
       const redirectRouteEnterSpy = spyOn(RedirectToInboxRoute, 'onEnter').andCallThrough()

modules/createTransitionManager.js

@@ -75,6 +75,9 @@ export default function createTransitionManager(history, routes) {
 
     runLeaveHooks(leaveRoutes)
 
+    // Tear down confirmation hooks for left routes
+    leaveRoutes.forEach(removeListenBeforeHooksForRoute)
+
     runEnterHooks(enterRoutes, nextState, function (error, redirectInfo) {
       if (error) {
         callback(error)
@@ -99,8 +102,8 @@ export default function createTransitionManager(history, routes) {
 
   let RouteGuid = 1
 
-  function getRouteID(route) {
-    return route.__id__ || (route.__id__ = RouteGuid++)
+  function getRouteID(route, create = true) {
+    return route.__id__ || create && (route.__id__ = RouteGuid++)
   }
 
   const RouteHooks = {}
@@ -141,6 +144,7 @@ export default function createTransitionManager(history, routes) {
     })
   }
 
+  /* istanbul ignore next: untestable with Karma */
   function beforeUnloadHook() {
     // Synchronously check to see if any route hooks want
     // to prevent the current window/tab from closing.
@@ -160,6 +164,28 @@ export default function createTransitionManager(history, routes) {
 
   let unlistenBefore, unlistenBeforeUnload
 
+  function removeListenBeforeHooksForRoute(route) {
+    const routeID = getRouteID(route, false)
+    if (!routeID) {
+      return
+    }
+
+    delete RouteHooks[routeID]
+
+    if (!hasAnyProperties(RouteHooks)) {
+      // teardown transition & beforeunload hooks
+      if (unlistenBefore) {
+        unlistenBefore()
+        unlistenBefore = null
+      }
+
+      if (unlistenBeforeUnload) {
+        unlistenBeforeUnload()
+        unlistenBeforeUnload = null
+      }
+    }
+  }
+
   /**
    * Registers the given hook function to run before leaving the given route.
    *
@@ -180,10 +206,10 @@ export default function createTransitionManager(history, routes) {
     const routeID = getRouteID(route)
     let hooks = RouteHooks[routeID]
 
-    if (hooks == null) {
+    if (!hooks) {
       let thereWereNoRouteHooks = !hasAnyProperties(RouteHooks)
 
-      hooks = RouteHooks[routeID] = [ hook ]
+      RouteHooks[routeID] = [ hook ]
 
       if (thereWereNoRouteHooks) {
         // setup transition & beforeunload hooks
@@ -199,24 +225,11 @@ export default function createTransitionManager(history, routes) {
     return function () {
       const hooks = RouteHooks[routeID]
 
-      if (hooks != null) {
+      if (hooks) {
         const newHooks = hooks.filter(item => item !== hook)
 
         if (newHooks.length === 0) {
-          delete RouteHooks[routeID]
-
-          if (!hasAnyProperties(RouteHooks)) {
-            // teardown transition & beforeunload hooks
-            if (unlistenBefore) {
-              unlistenBefore()
-              unlistenBefore = null
-            }
-
-            if (unlistenBeforeUnload) {
-              unlistenBeforeUnload()
-              unlistenBeforeUnload = null
-            }
-          }
+          removeListenBeforeHooksForRoute(route)
         } else {
           RouteHooks[routeID] = newHooks
         }