Fix initial routing state after match #2965
Conversation
|
Actually, maybe not... |
| @@ -75,7 +76,7 @@ describe('server rendering', function () { | |||
| const AsyncRoute = { | |||
| path: '/async', | |||
| getComponent(location, cb) { | |||
| setTimeout(cb(null, Async)) | |||
| setTimeout(() => cb(null, Async)) | |||
There was a problem hiding this comment.
How did this even work before? :|
There was a problem hiding this comment.
It wasn't async before.
|
Don't merge this yet... |
taion
changed the title
taion
changed the title
|
This is good to go now. The idea is to delegate the creation of the |
| @@ -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) => { | |||
There was a problem hiding this comment.
How do you get location from the server request into match?
There was a problem hiding this comment.
browserHistory should already have it in the URL.
There was a problem hiding this comment.
No, this is server rendering. How do I get req.url in there?
There was a problem hiding this comment.
This is actually the "what to do on the client when using server-side-rendering plus async routes" section.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Circling back up on this – thoughts? In 3.0, the requirement ought to be either a history or a matchContext.
|
Can you cherry pick in 63ac75c or just merge in that branch instead? Just wanted to clean up some of that section in Router. |
|
Updated. |
| ...nextState, | ||
| history, | ||
| router, | ||
| matchContext: { history, transitionManager, router } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
@timdorr What do you want to do with |
|
I say drop it and use |
|
How much do you care about this? I actually still think |
|
On a scale of 1 to 10: banana. Which is to say I do not. |
|
I care a little bit – makes the code in |
|
Yeah, let's do it. |
…tate Fix initial routing state after match
Fixes #2935
#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 samehistoryandtransitionManagerformatch()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.