From 74f93d6412811054017fe7ecd51bbb306667b955 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 9 May 2016 19:30:23 +0100 Subject: [PATCH 1/4] Add a failing test for deep withRouter() updates --- modules/__tests__/withRouter-test.js | 32 +++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/modules/__tests__/withRouter-test.js b/modules/__tests__/withRouter-test.js index b2df073c58..629c76f71b 100644 --- a/modules/__tests__/withRouter-test.js +++ b/modules/__tests__/withRouter-test.js @@ -11,7 +11,7 @@ describe('withRouter', function () { class App extends Component { render() { expect(this.props.router).toExist() - return

App

+ return

{this.props.router.location.pathname}

} } @@ -39,4 +39,34 @@ describe('withRouter', function () { done() }) }) + + it('updates the context inside static containers', function (done) { + const WrappedApp = withRouter(App) + + class StaticContainer extends Component { + shouldComponentUpdate() { + return false + } + + render() { + return this.props.children + } + } + + const history = createHistory('/') + + render(( + + + + + + + ), node, function () { + expect(node.firstChild.textContent).toEqual('/') + history.push('/hello') + expect(node.firstChild.textContent).toEqual('/hello') + done() + }) + }) }) From d48509628ef9967696657fcb41d4bc679f268d8d Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 9 May 2016 19:47:38 +0100 Subject: [PATCH 2/4] Update context for withRouter() inside static containers (test fails) --- modules/withRouter.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/modules/withRouter.js b/modules/withRouter.js index e9e7f32fda..2ae5e1b90f 100644 --- a/modules/withRouter.js +++ b/modules/withRouter.js @@ -1,5 +1,6 @@ import React from 'react' import hoistStatics from 'hoist-non-react-statics' +import { ContextSubscriber } from './ContextUtils' import { routerShape } from './PropTypes' function getDisplayName(WrappedComponent) { @@ -8,6 +9,7 @@ function getDisplayName(WrappedComponent) { export default function withRouter(WrappedComponent) { const WithRouter = React.createClass({ + mixins: [ ContextSubscriber('router') ], contextTypes: { router: routerShape }, render() { return From bb3e2df6290ae74d1248a49b7808b8d6e9e4e524 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 9 May 2016 19:55:58 +0100 Subject: [PATCH 3/4] Manually update the stale context value --- modules/ContextUtils.js | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/modules/ContextUtils.js b/modules/ContextUtils.js index ab8e7fc85a..0d9f9c7e1f 100644 --- a/modules/ContextUtils.js +++ b/modules/ContextUtils.js @@ -43,8 +43,9 @@ export function ContextProvider(name) { }, componentDidUpdate() { + const nextValue = this.getChildContext()[name] this[listenersKey].forEach(listener => - listener(this[eventIndexKey]) + listener(this[eventIndexKey], nextValue) ) }, @@ -111,8 +112,12 @@ export function ContextSubscriber(name) { this[unsubscribeKey] = null }, - [handleContextUpdateKey](eventIndex) { + [handleContextUpdateKey](eventIndex, nextValue) { if (eventIndex !== this.state[lastRenderedEventIndexKey]) { + if (this.context[name] !== nextValue) { + // React uses a stale value so we update it manually. + this.context[name] = nextValue + } this.setState({ [lastRenderedEventIndexKey]: eventIndex }) } } From 893cbf04a6b1ca410d230e03e8882de72460d978 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 9 May 2016 20:23:52 +0100 Subject: [PATCH 4/4] Preserve the identity of this.router to avoid setting this.context directly --- modules/ContextUtils.js | 10 +++------- modules/Router.js | 5 ++++- package.json | 1 + 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/modules/ContextUtils.js b/modules/ContextUtils.js index 0d9f9c7e1f..53fd494f0e 100644 --- a/modules/ContextUtils.js +++ b/modules/ContextUtils.js @@ -1,6 +1,7 @@ import { PropTypes } from 'react' // Works around issues with context updates failing to propagate. +// Caveat: the context value is expected to never change its identity. // https://github.com/facebook/react/issues/2517 // https://github.com/reactjs/react-router/issues/470 @@ -43,9 +44,8 @@ export function ContextProvider(name) { }, componentDidUpdate() { - const nextValue = this.getChildContext()[name] this[listenersKey].forEach(listener => - listener(this[eventIndexKey], nextValue) + listener(this[eventIndexKey]) ) }, @@ -112,12 +112,8 @@ export function ContextSubscriber(name) { this[unsubscribeKey] = null }, - [handleContextUpdateKey](eventIndex, nextValue) { + [handleContextUpdateKey](eventIndex) { if (eventIndex !== this.state[lastRenderedEventIndexKey]) { - if (this.context[name] !== nextValue) { - // React uses a stale value so we update it manually. - this.context[name] = nextValue - } this.setState({ [lastRenderedEventIndexKey]: eventIndex }) } } diff --git a/modules/Router.js b/modules/Router.js index 3a9f57c419..eeefdbc1dc 100644 --- a/modules/Router.js +++ b/modules/Router.js @@ -6,6 +6,7 @@ import RouterContext from './RouterContext' import { createRoutes } from './RouteUtils' import { createRouterObject } from './RouterUtils' import warning from './routerWarning' +import assign from 'object-assign' const { func, object } = React.PropTypes @@ -88,7 +89,9 @@ const Router = React.createClass({ if (error) { this.handleError(error) } else { - this.router = this.createRouterObject(state) + // Keep the identity of this.router because of a caveat in ContextUtils: + // they only work if the object identity is preserved. + assign(this.router, this.createRouterObject(state)) this.setState(state, this.props.onUpdate) } }) diff --git a/package.json b/package.json index 40bc492a1d..82a8a3fe69 100644 --- a/package.json +++ b/package.json @@ -35,6 +35,7 @@ "history": "^2.0.1", "hoist-non-react-statics": "^1.0.5", "invariant": "^2.2.1", + "object-assign": "^4.1.0", "warning": "^2.1.0" }, "peerDependencies": {