Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix initial routing state after match #2965

Merged
merged 2 commits into from
Jan 27, 2016
Merged

Fix initial routing state after match #2965

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6ab8316
Fix initial routing state after match
taion Jan 23, 2016
a0120b9
More explicit and opaque match context for Router
taion Jan 24, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions docs/guides/advanced/ServerRendering.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you get location from the server request into match?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

browserHistory should already have it in the URL.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is server rendering. How do I get req.url in there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually the "what to do on the client when using server-side-rendering plus async routes" section.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the server, as above, you still match({ routes, location }) and render a <RouterContext>.

On the client, here, you match({ history, routes }) and render a <Router>. Since history is going to be some browser history, it doesn't need to be told its location.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, well that's just because we have shitty docs. What are "async routes"? I know what that means, but it's not explained anywhere else in the docs. This is an example of why they don't work and why we get so many of the same set of questions all the damn time.

Anyways, I do like that this neatens up the API a tad, as we don't have to pass through our history to the Router instance by hand. Sorry for the confusion here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually less bothered by this specific case – both async routes and SSR are relatively advanced, so the documentation around the intersection of the two doesn't need to target beginners.

It's not just "we don't have to pass through our history" in this case though – it's actually necessary that both match and <Router> use the same history, since the transition manager (that we want to re-use) is already bound to a history.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Circling back up on this – thoughts? In 3.0, the requirement ought to be either a history or a matchContext.

render(<Router {...renderProps} />, mountNode)
})
```
51 changes: 35 additions & 16 deletions modules/Router.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand All @@ -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) {
Expand All @@ -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)
Expand All @@ -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) {
Expand Down
36 changes: 22 additions & 14 deletions modules/__tests__/serverRendering-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
)
}
Expand All @@ -75,7 +76,7 @@ describe('server rendering', function () {
const AsyncRoute = {
path: '/async',
getComponent(location, cb) {
setTimeout(cb(null, Async))
setTimeout(() => cb(null, Async))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did this even work before? :|

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wasn't async before.

}
}

Expand Down Expand Up @@ -154,31 +155,38 @@ 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) {
match({ routes, location: '/async' }, function (error, redirectLocation, renderProps) {
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()
})
})
})
})
Expand Down
10 changes: 5 additions & 5 deletions modules/createTransitionManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
Expand Down
38 changes: 30 additions & 8 deletions modules/match.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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 }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I get you're concerned that someone might have a mismatched history/transitionManager/router combo, but is that practically ever going to happen? This is entirely an internal-use, undocumented API, and if you use it, you'll need to learn how the code operates. In fact, you'll have to be reading through the code to even find it in the first place.

So, this seems really weird to pass in the same things twice. It should just be the router and transitionManager props.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, router as well – it's just that in this case, it feels more opaque to delegate everything to the matchContext prop; the affordance is that even if the user somehow directly passes in a different history or router object, that <Router> won't silently use those.

}
)

// 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()
}
})
}

Expand Down