-
-
Notifications
You must be signed in to change notification settings - Fork 10.7k
Add context.route.location #4589
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.
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
Conversation
if (!this.props.location) { | ||
// Start listening here so we can <Redirect> on the initial render. | ||
this.unlisten = parentRouter.listen(() => { | ||
Object.assign(this.router, parentRouter, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why Object.assign? We support object spread (it's 8 lines above this 😄 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spread would create a new object.
this.router = {
...this.router,
...parentRouter,
match: computeMatch(...)
}
// compiles to
this.router = _extends({}, this.router, parentRouter, {
match: computeMatch(...)
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@timdorr We mutate this.context.router
intentionally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I've got it now 👍
This can be simplified once #4598 is merged. Will hold off on updating this until then. |
This has been updated to work with the new context API. Each The |
we can just use a Route to make a match, no need for special-case code
Hey @pshrmn, I did my best to resolve conflicts with this branch and current v4. Please give it the thumbs up and I'll merge away. |
There are a couple of tweaks that I think need to be made still. I will commit those later tonight. Was there ever any decision on whether to add |
The default `context.route.location` value is the same as `context.history.location`. However, if a <Route> has a `location` prop, then all of its children will see that location object as `context.route.location`. This also updates the `<Switch>` component to pass its location as a prop to the component that it renders.
When calling computeMatch from componentWillReceiveProps, we need to use nextContext, not this.context
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving a few comments on new changes.
@@ -61,7 +61,7 @@ class FakeBrowser extends React.Component { | |||
|
|||
return ( | |||
<MemoryRouter getUserConfirmation={getUserConfirmation}> | |||
<Route render={({ canGo, goBack, goForward, push, location }) => ( | |||
<Route render={({ history, location }) => ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to fix a few things in the examples to get them working with the new { match, location, history }
props.
@@ -62,15 +63,15 @@ class Route extends React.Component { | |||
) | |||
|
|||
this.setState({ | |||
match: this.computeMatch(nextProps) | |||
match: this.computeMatch(nextProps, nextContext) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to use the next context object here. Otherwise when there is no location
prop, the previous location will be used when computing the new match.
const { history } = this.context | ||
const { location } = history | ||
const { history, route } = this.context | ||
const location = this.props.location || route.location |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just making sure that the component rendered by the route receives the correct location.
@pshrmn We're going to add |
@mjackson Yes, I saw that you switched it back shortly after I had asked the question. This should be good to merge if everything looks fine to you. |
Thanks @pshrmn |
When a
<Switch>
or a<Route>
has alocation
prop, its children should match using thatlocation
instead of the currenthistory.location
.This also adds a modal example since the PR changes enable it and that seems to be a popular request.