-
Notifications
You must be signed in to change notification settings - Fork 709
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
docs: adds note about react-router history compatibility #968
base: main
Are you sure you want to change the base?
Conversation
Hi @ryan-sandy! |
Hi @MatanBobi, This is not an edge case, but expected behavior on the most stable versions of react-router (v5) and history. If you follow the documentation as written, your tests will not work. I agree, once react-router (v6) launches, we can remove this note. Until then, is there any harm in having it? As for the issue, I cannot edit it. Moreover, it took me days to isolate and find this issue. A simple note would have prevented it. |
If you need more evidence, this person experienced the same issue. |
Hi @ryan-sandy, first thing, I don't want you to get the impression that your effort isn't valued, it is. |
So the issue here is that the example imports from Possible fixes:
|
@alexkrolick I agree. At it's root, this issue is caused by react-router not setting history as a peerDep. We can bug them about it, but in the mean time, I think it's our job to document the issue and the possible fixes in this document. Either of your first two suggestions are excellent workarounds. We can expand on my note to suggest disabling the linter. You cannot use the Memory or Browser Router. Both of those ignore the I have not dug deep enough into the React-router (v5 at least) to determine if the history library is exported in some capacity. I believe they do not, as they want you to use the |
For this one, I think the answer is unfortunately they don't. In the FAQ they say that to access the history object outside of a component, you'll need to import from As for the other options, I think I'm in favor of recommending to check the compatible |
Following the discussion in this issue, this PR aims to document the reason for that issue.