Repository: rackt/react-router
Pull request #2965 (2016-01-23 19:52:18)

Fix initial routing state after match
Fixes https://github.com/rackt/react-router/issues/2935

https://github.com/rackt/react-router/pull/2883 was actually busted and had a test that didn't cover what it was supposed to.

~~In an ideal world we'd just use the same `history` and `transitionManager` for `match()` and `<Router>` on the client side, but the deprecation wrapping API messes with that too much, so this might be the best we can do for now.~~

taion: https://github.com/rackt/react-router/pull/2883 was actually busted and had a test that didn't cover what it was supposed to.

In an ideal world we'd just use the same `history` and `transitionManager` for `match()` and `<Router>` on the client side, but the deprecation wrapping API messes with that too much, so this might be the best we can do for now.
taion: Actually, maybe not...
taion: Don't merge this yet...
taion: This is good to go now. The idea is to delegate the creation of the `router` and `transitionManager` to `match()` when possible, and just use those same objects (that now have all the correct state) in `<Router>` rather than creating new ones.

Patch:
diff --git a/docs/guides/advanced/ServerRendering.md b/docs/guides/advanced/ServerRendering.md
index bc51cb91e..124a8113d 100644
--- a/docs/guides/advanced/ServerRendering.md
+++ b/docs/guides/advanced/ServerRendering.md
@@ -44,7 +44,7 @@ For data loading, you can use the `renderProps` argument to build whatever conve
 
 Server rendering works identically when using async routes. However, the client-side rendering needs to be a little different to make sure all of the async behavior has been resolved before the initial render, to avoid a mismatch between the server rendered and client rendered markup.
 
-Instead of rendering
+On the client, instead of rendering
 
 ```js
 render(<Router history={history} routes={routes} />, mountNode)
@@ -53,7 +53,7 @@ render(<Router history={history} routes={routes} />, mountNode)
 You need to do
 
 ```js
-match({ routes, location }, (error, redirectLocation, renderProps) => {
-  render(<Router {...renderProps} history={history} />, mountNode)
+match({ history, routes }, (error, redirectLocation, renderProps) => {
+  render(<Router {...renderProps} />, mountNode)
 })
 ```
diff --git a/modules/Router.js b/modules/Router.js
index 0642a9abc..89c341883 100644
--- a/modules/Router.js
+++ b/modules/Router.js
@@ -29,7 +29,10 @@ const Router = React.createClass({
     render: func,
     createElement: func,
     onError: func,
-    onUpdate: func
+    onUpdate: func,
+
+    // PRIVATE: For client-side rehydration of server match.
+    matchContext: object
   },
 
   getDefaultProps() {
@@ -41,10 +44,12 @@ const Router = React.createClass({
   },
 
   getInitialState() {
-    // Use initial state from renderProps when available, to allow using match
-    // on client side when doing server-side rendering.
-    const { location, routes, params, components } = this.props
-    return { location, routes, params, components }
+    return {
+      location: null,
+      routes: null,
+      params: null,
+      components: null
+    }
   },
 
   handleError(error) {
@@ -57,22 +62,14 @@ const Router = React.createClass({
   },
 
   componentWillMount() {
-    let { history } = this.props
-    const { routes, children } = this.props
-
     const { parseQueryString, stringifyQuery } = this.props
     warning(
       !(parseQueryString || stringifyQuery),
       '`parseQueryString` and `stringifyQuery` are deprecated. Please create a custom history. http://tiny.cc/router-customquerystring'
     )
 
-    if (isDeprecatedHistory(history)) {
-      history = this.wrapDeprecatedHistory(history)
-    }
+    const { history, transitionManager, router } = this.createRouterObjects()
 
-    const transitionManager = createTransitionManager(
-      history, createRoutes(routes || children)
-    )
     this._unlisten = transitionManager.listen((error, state) => {
       if (error) {
         this.handleError(error)
@@ -81,8 +78,30 @@ const Router = React.createClass({
       }
     })
 
-    this.router = createRouterObject(history, transitionManager)
-    this.history = createRoutingHistory(history, transitionManager)
+    this.history = history
+    this.router = router
+  },
+
+  createRouterObjects() {
+    const { matchContext } = this.props
+    if (matchContext) {
+      return matchContext
+    }
+
+    let { history } = this.props
+    const { routes, children } = this.props
+
+    if (isDeprecatedHistory(history)) {
+      history = this.wrapDeprecatedHistory(history)
+    }
+
+    const transitionManager = createTransitionManager(
+      history, createRoutes(routes || children)
+    )
+    const router = createRouterObject(history, transitionManager)
+    const routingHistory = createRoutingHistory(history, transitionManager)
+
+    return { history: routingHistory, transitionManager, router }
   },
 
   wrapDeprecatedHistory(history) {
diff --git a/modules/createTransitionManager.js b/modules/createTransitionManager.js
index 750897514..5d7ec17a6 100644
--- a/modules/createTransitionManager.js
+++ b/modules/createTransitionManager.js
@@ -217,12 +217,12 @@ export default function createTransitionManager(history, routes) {
           unlistenBeforeUnload = history.listenBeforeUnload(beforeUnloadHook)
       }
     } else {
-      warning(
-        false,
-        'adding multiple leave hooks for the same route is deprecated; manage multiple confirmations in your own code instead'
-      )
-
       if (hooks.indexOf(hook) === -1) {
+        warning(
+          false,
+          'adding multiple leave hooks for the same route is deprecated; manage multiple confirmations in your own code instead'
+        )
+
         hooks.push(hook)
       }
     }
diff --git a/modules/match.js b/modules/match.js
index 332c2f7e6..344df5bc4 100644
--- a/modules/match.js
+++ b/modules/match.js
@@ -11,13 +11,13 @@ import { createRouterObject, createRoutingHistory } from './RouterUtils'
  * This function matches a location to a set of routes and calls
  * callback(error, redirectLocation, renderProps) when finished.
  *
- * Note: You probably don't want to use this in a browser. Use
- * the history.listen API instead.
+ * Note: You probably don't want to use this in a browser unless you're using
+ * server-side rendering with async routes.
  */
 function match({ history, routes, location, ...options }, callback) {
   invariant(
-    location,
-    'match needs a location'
+    history || location,
+    'match needs a history or a location'
   )
 
   history = history ? history : createMemoryHistory(options)
@@ -26,9 +26,19 @@ function match({ history, routes, location, ...options }, callback) {
     createRoutes(routes)
   )
 
-  // Allow match({ location: '/the/path', ... })
-  if (typeof location === 'string')
-    location = history.createLocation(location)
+  let unlisten
+
+  if (location) {
+    // Allow match({ location: '/the/path', ... })
+    if (typeof location === 'string')
+      location = history.createLocation(location)
+  } else {
+    // Pick up the location from the history via synchronous history.listen
+    // call if needed.
+    unlisten = history.listen(historyLocation => {
+      location = historyLocation
+    })
+  }
 
   const router = createRouterObject(history, transitionManager)
   history = createRoutingHistory(history, transitionManager)
@@ -37,8 +47,20 @@ function match({ history, routes, location, ...options }, callback) {
     callback(
       error,
       redirectLocation,
-      nextState && { ...nextState, history, router }
+      nextState && {
+        ...nextState,
+        history,
+        router,
+        matchContext: { history, transitionManager, router }
+      }
     )
+
+    // Defer removing the listener to here to prevent DOM histories from having
+    // to unwind DOM event listeners unnecessarily, in case callback renders a
+    // <Router> and attaches another history listener.
+    if (unlisten) {
+      unlisten()
+    }
   })
 }
 


Test patch:
diff --git a/modules/__tests__/serverRendering-test.js b/modules/__tests__/serverRendering-test.js
index 6ccbd29ad..a1aabb3f2 100644
--- a/modules/__tests__/serverRendering-test.js
+++ b/modules/__tests__/serverRendering-test.js
@@ -50,6 +50,7 @@ describe('server rendering', function () {
       return (
         <div className="Async">
           <h1>Async</h1>
+          <Link to="/async" activeClassName="async-is-active">Link</Link>
         </div>
       )
     }
@@ -75,7 +76,7 @@ describe('server rendering', function () {
   const AsyncRoute = {
     path: '/async',
     getComponent(location, cb) {
-      setTimeout(cb(null, Async))
+      setTimeout(() => cb(null, Async))
     }
   }
 
@@ -154,18 +155,20 @@ describe('server rendering', function () {
   describe('server/client consistency', function () {
     // Just render to static markup here to avoid having to normalize markup.
 
-    it('should match for synchronous route', function (done) {
+    it('should match for synchronous route', function () {
+      let serverString
+
       match({ routes, location: '/dashboard' }, function (error, redirectLocation, renderProps) {
-        const serverString = renderToStaticMarkup(
+        serverString = renderToStaticMarkup(
           <RouterContext {...renderProps} />
         )
-        const browserString = renderToStaticMarkup(
-          <Router {...renderProps} history={createMemoryHistory('/dashboard')} />
-        )
-
-        expect(browserString).toEqual(serverString)
-        done()
       })
+
+      const browserString = renderToStaticMarkup(
+        <Router history={createMemoryHistory('/dashboard')} routes={routes} />
+      )
+
+      expect(browserString).toEqual(serverString)
     })
 
     it('should match for asynchronous route', function (done) {
@@ -173,12 +176,17 @@ describe('server rendering', function () {
         const serverString = renderToStaticMarkup(
           <RouterContext {...renderProps} />
         )
-        const browserString = renderToStaticMarkup(
-          <Router {...renderProps} history={createMemoryHistory('/async')} />
-        )
 
-        expect(browserString).toEqual(serverString)
-        done()
+        match({ history: createMemoryHistory('/async'), routes }, function (error, redirectLocation, renderProps) {
+          const browserString = renderToStaticMarkup(
+            <Router {...renderProps} />
+          )
+
+          expect(browserString).toEqual(serverString)
+          expect(browserString).toMatch(/async-is-active/)
+
+          done()
+        })
       })
     })
   })