-
-
Notifications
You must be signed in to change notification settings - Fork 10.4k
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
Issue #5114 warn about history prop in Router-derived components #5151
Conversation
'`<BrowserRouter history={...}` prop has been ignored. For custom history, ' + | ||
'make sure to `import {Router}` and not `import {... as Router}`.') | ||
} | ||
|
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.
This should just use the warning
package. You'll need to install it for react-router-dom
, but it is already installed for react-router
. Basically, just change this block to:
warning(
!this.props.history,
...
);
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.
This is a minor nit, but I would prefer if there were spaces inside of the curly brackets. import { Router }
and import { ... as Router }
.
… 5114-history-warning
the additional 2
@pshrmn I have better things to do that trying to debug how to test this :(
|
I used wrong syntax in previous change, please disregard. |
99d9716
to
50667ad
Compare
The code looks good to me now, thanks!
|
warning( | ||
!this.props.history, | ||
'`<BrowserRouter history={...}` prop has been ignored. For custom history, ' + | ||
'make sure to `import { Router }` and not `import { ... as Router }`.' |
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.
You can fill in the ellipsis in these warnings with the actual Component name.
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 thought of it as a general warning to avoid using any of the Browser/Hash/Memory/Static Router with history, do you think the ellipsis makes the message unclear?
OK, cleaned up the messaging a bit. Thanks! |
Adding a warning as suggested in issue #5114.