Call getComponent with nextState instead of location

taion · taion · commit 3076dca3c433 · 2016-04-13T17:47:59.000-04:00
diff --git a/docs/API.md b/docs/API.md
@@ -84,7 +84,7 @@ A function used to convert an object from [`<Link>`](#link)s or calls to
 A function used to convert a query string into an object that gets passed to route component props.
 
 ##### `onError(error)`
-While the router is matching, errors may bubble up, here is your opportunity to catch and deal with them. Typically these will come from async features like [`route.getComponents`](#getcomponentslocation-callback), [`route.getIndexRoute`](#getindexroutelocation-callback), and [`route.getChildRoutes`](#getchildrouteslocation-callback).
+While the router is matching, errors may bubble up, here is your opportunity to catch and deal with them. Typically these will come from async features like [`route.getComponents`](#getcomponentsnextstate-callback), [`route.getIndexRoute`](#getindexroutelocation-callback), and [`route.getChildRoutes`](#getchildrouteslocation-callback).
 
 ##### `onUpdate()`
 Called whenever the router updates its state in response to URL changes.
@@ -300,29 +300,29 @@ class Users extends React.Component {
 }
 ```
 
-##### `getComponent(location, callback)`
+##### `getComponent(nextState, callback)`
 Same as `component` but asynchronous, useful for
 code-splitting.
 
 ###### `callback` signature
 `cb(err, component)`
 
 ```js
-<Route path="courses/:courseId" getComponent={(location, cb) => {
+<Route path="courses/:courseId" getComponent={(nextState, cb) => {
   // do asynchronous stuff to find the components
   cb(null, Course)
 }} />
 ```
 
-##### `getComponents(location, callback)`
+##### `getComponents(nextState, callback)`
 Same as `components` but asynchronous, useful for
 code-splitting.
 
 ###### `callback` signature
 `cb(err, components)`
 
 ```js
-<Route path="courses/:courseId" getComponents={(location, cb) => {
+<Route path="courses/:courseId" getComponents={(nextState, cb) => {
   // do asynchronous stuff to find the components
   cb(null, {sidebar: CourseSidebar, content: Course})
 }} />
diff --git a/docs/guides/DynamicRouting.md b/docs/guides/DynamicRouting.md
@@ -10,7 +10,7 @@ A router is the perfect place to handle code splitting: it's responsible for set
 
 React Router does all of its [path matching](/docs/guides/RouteMatching.md) and component fetching asynchronously, which allows you to not only load up the components lazily, *but also lazily load the route configuration*. You really only need one route definition in your initial bundle, the router can resolve the rest on demand.
 
-Routes may define [`getChildRoutes`](/docs/API.md#getchildrouteslocation-callback), [`getIndexRoute`](/docs/API.md#getindexroutelocation-callback), and [`getComponents`](/docs/API.md#getcomponentslocation-callback) methods. These are asynchronous and only called when needed. We call it "gradual matching". React Router will gradually match the URL and fetch only the amount of route configuration and components it needs to match the URL and render.
+Routes may define [`getChildRoutes`](/docs/API.md#getchildrouteslocation-callback), [`getIndexRoute`](/docs/API.md#getindexroutelocation-callback), and [`getComponents`](/docs/API.md#getcomponentsnextstate-callback) methods. These are asynchronous and only called when needed. We call it "gradual matching". React Router will gradually match the URL and fetch only the amount of route configuration and components it needs to match the URL and render.
 
 Coupled with a smart code splitting tool like [webpack](http://webpack.github.io/), a once tiresome architecture is now simple and declarative.
 
@@ -36,7 +36,7 @@ const CourseRoute = {
     })
   },
 
-  getComponents(location, callback) {
+  getComponents(nextState, callback) {
     require.ensure([], function (require) {
       callback(null, require('./components/Course'))
     })
diff --git a/examples/auth-with-shared-root/config/routes.js b/examples/auth-with-shared-root/config/routes.js
@@ -19,14 +19,14 @@ export default {
   component: require('../components/App'),
   childRoutes: [
     { path: '/logout',
-      getComponent: (location, cb) => {
+      getComponent: (nextState, cb) => {
         require.ensure([], (require) => {
           cb(null, require('../components/Logout'))
         })
       }
     },
     { path: '/about',
-      getComponent: (location, cb) => {
+      getComponent: (nextState, cb) => {
         require.ensure([], (require) => {
           cb(null, require('../components/About'))
         })
@@ -38,7 +38,7 @@ export default {
         // Unauthenticated routes
         // Redirect to dashboard if user is already logged in
         { path: '/login',
-          getComponent: (location, cb) => {
+          getComponent: (nextState, cb) => {
             require.ensure([], (require) => {
               cb(null, require('../components/Login'))
             })
@@ -52,7 +52,7 @@ export default {
       childRoutes: [
         // Protected routes that don't share the dashboard UI
         { path: '/user/:id',
-          getComponent: (location, cb) => {
+          getComponent: (nextState, cb) => {
             require.ensure([], (require) => {
               cb(null, require('../components/User'))
             })
@@ -63,7 +63,7 @@ export default {
     },
 
     { path: '/',
-      getComponent: (location, cb) => {
+      getComponent: (nextState, cb) => {
         // Share the path
         // Dynamically load the correct component
         if (auth.loggedIn()) {
@@ -76,7 +76,7 @@ export default {
         })
       },
       indexRoute: {
-        getComponent: (location, cb) => {
+        getComponent: (nextState, cb) => {
           // Only load if we're logged in
           if (auth.loggedIn()) {
             return require.ensure([], (require) => {
@@ -91,7 +91,7 @@ export default {
           childRoutes: [
             // Protected nested routes for the dashboard
             { path: '/page2',
-              getComponent: (location, cb) => {
+              getComponent: (nextState, cb) => {
                 require.ensure([], (require) => {
                   cb(null, require('../components/PageTwo'))
                 })
diff --git a/examples/huge-apps/routes/Calendar/index.js b/examples/huge-apps/routes/Calendar/index.js
@@ -1,6 +1,6 @@
 module.exports = {
   path: 'calendar',
-  getComponent(location, cb) {
+  getComponent(nextState, cb) {
     require.ensure([], (require) => {
       cb(null, require('./components/Calendar'))
     })
diff --git a/examples/huge-apps/routes/Course/index.js b/examples/huge-apps/routes/Course/index.js
@@ -11,7 +11,7 @@ module.exports = {
     })
   },
 
-  getComponent(location, cb) {
+  getComponent(nextState, cb) {
     require.ensure([], (require) => {
       cb(null, require('./components/Course'))
     })
diff --git a/examples/huge-apps/routes/Course/routes/Announcements/index.js b/examples/huge-apps/routes/Course/routes/Announcements/index.js
@@ -9,7 +9,7 @@ module.exports = {
     })
   },
 
-  getComponents(location, cb) {
+  getComponents(nextState, cb) {
     require.ensure([], (require) => {
       cb(null, {
         sidebar: require('./components/Sidebar'),
diff --git a/examples/huge-apps/routes/Course/routes/Announcements/routes/Announcement/index.js b/examples/huge-apps/routes/Course/routes/Announcements/routes/Announcement/index.js
@@ -1,7 +1,7 @@
 module.exports = {
   path: ':announcementId',
 
-  getComponent(location, cb) {
+  getComponent(nextState, cb) {
     require.ensure([], (require) => {
       cb(null, require('./components/Announcement'))
     })
diff --git a/examples/huge-apps/routes/Course/routes/Assignments/index.js b/examples/huge-apps/routes/Course/routes/Assignments/index.js
@@ -9,7 +9,7 @@ module.exports = {
     })
   },
 
-  getComponents(location, cb) {
+  getComponents(nextState, cb) {
     require.ensure([], (require) => {
       cb(null, {
         sidebar: require('./components/Sidebar'),
diff --git a/examples/huge-apps/routes/Course/routes/Assignments/routes/Assignment/index.js b/examples/huge-apps/routes/Course/routes/Assignments/routes/Assignment/index.js
@@ -1,6 +1,6 @@
 module.exports = {
   path: ':assignmentId',
-  getComponent(location, cb) {
+  getComponent(nextState, cb) {
     require.ensure([], (require) => {
       cb(null, require('./components/Assignment'))
     })
diff --git a/examples/huge-apps/routes/Course/routes/Grades/index.js b/examples/huge-apps/routes/Course/routes/Grades/index.js
@@ -1,6 +1,6 @@
 module.exports = {
   path: 'grades',
-  getComponent(location, cb) {
+  getComponent(nextState, cb) {
     require.ensure([], (require) => {
       cb(null, require('./components/Grades'))
     })
diff --git a/examples/huge-apps/routes/Grades/index.js b/examples/huge-apps/routes/Grades/index.js
@@ -1,6 +1,6 @@
 module.exports = {
   path: 'grades',
-  getComponent(location, cb) {
+  getComponent(nextState, cb) {
     require.ensure([], (require) => {
       cb(null, require('./components/Grades'))
     })
diff --git a/examples/huge-apps/routes/Messages/index.js b/examples/huge-apps/routes/Messages/index.js
@@ -1,6 +1,6 @@
 module.exports = {
   path: 'messages',
-  getComponent(location, cb) {
+  getComponent(nextState, cb) {
     require.ensure([], (require) => {
       cb(null, require('./components/Messages'))
     })
diff --git a/examples/huge-apps/routes/Profile/index.js b/examples/huge-apps/routes/Profile/index.js
@@ -1,6 +1,6 @@
 module.exports = {
   path: 'profile',
-  getComponent(location, cb) {
+  getComponent(nextState, cb) {
     require.ensure([], (require) => {
       cb(null, require('./components/Profile'))
     })
diff --git a/modules/__tests__/Router-test.js b/modules/__tests__/Router-test.js
@@ -2,8 +2,10 @@ import expect from 'expect'
 import React, { Component } from 'react'
 import { render, unmountComponentAtNode } from 'react-dom'
 import createHistory from '../createMemoryHistory'
+import { canUseMembrane } from '../deprecateObjectProperties'
 import Route from '../Route'
 import Router from '../Router'
+import shouldWarn from './shouldWarn'
 
 describe('Router', function () {
 
@@ -329,7 +331,8 @@ describe('Router', function () {
 
     it('should support getComponent', function (done) {
       const Component = () => <div />
-      const getComponent = (_, callback) => {
+      const getComponent = (nextState, callback) => {
+        expect(nextState.location.pathname).toBe('/')
         setTimeout(() => callback(null, Component))
       }
 
@@ -349,7 +352,8 @@ describe('Router', function () {
       const foo = () => <div />
       const bar = () => <div />
 
-      const getComponents = (_, callback) => {
+      const getComponents = (nextState, callback) => {
+        expect(nextState.location.pathname).toBe('/')
         setTimeout(() => callback(null, { foo, bar }))
       }
 
@@ -364,6 +368,30 @@ describe('Router', function () {
         })
       })
     })
+
+    it('should supply location properties to getComponent', function (done) {
+      if (canUseMembrane) {
+        shouldWarn('deprecated')
+      }
+
+      const Component = () => <div />
+      const getComponent = (location, callback) => {
+        expect(location.pathname).toBe('/')
+        setTimeout(() => callback(null, Component))
+      }
+
+      render((
+        <Router history={createHistory('/')} render={renderSpy}>
+          <Route path="/" getComponent={getComponent} />
+        </Router>
+      ), node, function () {
+        setTimeout(function () {
+          expect(componentSpy).toHaveBeenCalledWith([ Component ])
+          done()
+        })
+      })
+    })
+
   })
 
   describe('error handling', function () {
diff --git a/modules/deprecateObjectProperties.js b/modules/deprecateObjectProperties.js
@@ -1,20 +1,20 @@
 import warning from './routerWarning'
 
+export let canUseMembrane = false
+
 // No-op by default.
 let deprecateObjectProperties = object => object
 
 if (__DEV__) {
-  let useMembrane = false
-
   try {
     if (Object.defineProperty({}, 'x', { get() { return true } }).x) {
-      useMembrane = true
+      canUseMembrane = true
     }
   /* eslint-disable no-empty */
   } catch(e) {}
   /* eslint-enable no-empty */
 
-  if (useMembrane) {
+  if (canUseMembrane) {
     deprecateObjectProperties = (object, message) => {
       // Wrap the deprecated object in a membrane to warn on property access.
       const membrane = {}
@@ -39,8 +39,6 @@ if (__DEV__) {
         // ownKeys trap on proxies is not universal, even among browsers that
         // otherwise support proxies.
         Object.defineProperty(membrane, prop, {
-          configurable: false,
-          enumerable: false,
           get() {
             warning(false, message)
             return object[prop]
diff --git a/modules/getComponents.js b/modules/getComponents.js
@@ -1,15 +1,43 @@
 import { mapAsync } from './AsyncUtils'
+import { canUseMembrane } from './deprecateObjectProperties'
+import warning from './routerWarning'
 
-function getComponentsForRoute(location, route, callback) {
+function getComponentsForRoute(nextState, route, callback) {
   if (route.component || route.components) {
     callback(null, route.component || route.components)
-  } else if (route.getComponent) {
-    route.getComponent(location, callback)
-  } else if (route.getComponents) {
-    route.getComponents(location, callback)
-  } else {
-    callback()
+    return
   }
+
+  const getComponent = route.getComponent || route.getComponents
+  if (getComponent) {
+    let nextStateWithLocation = { ...nextState }
+    const { location } = nextState
+
+    if (__DEV__ && canUseMembrane) {
+      // I don't use deprecateObjectProperties here because I want to keep the
+      // same code path between development and production, in that we just
+      // assign extra properties to the copy of the state object in both cases.
+      for (const prop in location) {
+        if (!Object.prototype.hasOwnProperty.call(location, prop)) {
+          continue
+        }
+
+        Object.defineProperty(nextStateWithLocation, prop, {
+          get() {
+            warning(false, 'Accessing location properties from the first argument to `getComponent` and `getComponents` is deprecated. That argument is now the router state (`nextState`) rather than the location. To access the location, use `nextState.location`.')
+            return location[prop]
+          }
+        })
+      }
+    } else {
+      Object.assign(nextStateWithLocation, location)
+    }
+
+    getComponent(nextStateWithLocation, callback)
+    return
+  }
+
+  callback()
 }
 
 /**
@@ -21,7 +49,7 @@ function getComponentsForRoute(location, route, callback) {
  */
 function getComponents(nextState, callback) {
   mapAsync(nextState.routes, function (route, index, callback) {
-    getComponentsForRoute(nextState.location, route, callback)
+    getComponentsForRoute(nextState, route, callback)
   }, callback)
 }
 
diff --git a/upgrade-guides/v2.2.0.md b/upgrade-guides/v2.2.0.md
@@ -0,0 +1,25 @@
+# v2.2.0 Upgrade Guide
+
+## `getComponent`, `getComponents` signature
+
+**This is unlikely to affect you, even if you use `getComponent` and `getComponents`.**
+
+The signature of `getComponent` and `getComponents` has been changed from `(location: Location, callback: Function) => any` to `(nextState: RouterState, callback: Function) => any`. That means that instead of writing
+
+```js
+getComponent(location, cb) {
+  cb(fetchComponent(location.query))
+}
+```
+
+You would need to instead write
+
+```js
+getComponent(nextState, cb) {
+  cb(fetchComponent(nextState.location.query))
+}
+```
+
+However, you new also have access to the matched `params` on `nextState`, and can use those to determine which component to return.
+
+You will still be able to access location properties directly on `nextState` until the next breaking release (and in fact they will shadow router state properties, if applicable), but this will case a deprecation warning in development mode.
