-
Notifications
You must be signed in to change notification settings - Fork 156
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
rerender on props.url change #332
base: main
Are you sure you want to change the base?
Conversation
src/index.js
Outdated
@@ -160,6 +160,10 @@ class Router extends Component { | |||
return props.url!==this.props.url || props.onChange!==this.props.onChange; | |||
} | |||
|
|||
componentWillReceiveProps(props) { |
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 think we can merge this with shouldComponentUpdate
above.
shouldComponentUpdate(props) {
if (props.url && props.url!==this.state.url) this.routeTo(props.url);
return !props.static || props.url!=this.props.url || props.onChange!=this.props.onChange;
}
src/index.js
Outdated
@@ -160,6 +160,10 @@ class Router extends Component { | |||
return props.url!==this.props.url || props.onChange!==this.props.onChange; | |||
} | |||
|
|||
componentWillReceiveProps(props) { | |||
if (props.url && props.url!==this.state.url) this.routeTo(props.url) && setUrl(props.url); |
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.
Passing a new URL to <Router>
should not call setUrl()
, since doing so will change all other Routers on the page.
Sorry for the super long delay in reviewing this @halcaponey! Just a few things I think we can address to get this merged. |
Update looks good. Only thing I want to check is if this is triggering a double-render when changing the URL prop. Ideally we'd propagate the state without forcing a second render, since the router is already being re-rendered with the new URL as a prop. |
The problem comes from What is the static prop used for ? It's only called in shouldComponentUpdate.
|
resolves #330