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.

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

halcaponey
Copy link

resolves #330

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) {
Copy link
Member

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);
Copy link
Member

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.

@developit
Copy link
Member

Sorry for the super long delay in reviewing this @halcaponey! Just a few things I think we can address to get this merged.

@developit
Copy link
Member

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.

@halcaponey
Copy link
Author

The problem comes from !props.static being always true and since a change on the url prop triggers a setState which triggers a shouldComponentUpdate which returns true because the static prop is not defined.

What is the static prop used for ? It's only called in shouldComponentUpdate.

  • If it's false or undefined, shouldComponentUpdate will always return true.
  • if it's true then url prop and onChange prop will determine the result of shouldComponentUpdate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

updating url prop doesn't trigger an update
2 participants