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

docs: adds note about react-router history compatibility #968

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ryan-sandy
Copy link

@ryan-sandy ryan-sandy commented Nov 23, 2021

Following the discussion in this issue, this PR aims to document the reason for that issue.

@MatanBobi
Copy link
Member

Hi @ryan-sandy!
Thanks for taking the time to open this!
This looks to me like an edge-case that IMHO doesn't worth mentioning in the docs (since an explanation can be found in the issue you linked there).
I do recommend editing the issue's title so it will be easily found in searches :)
I'm closing this one, thanks again for your time and effort.

@MatanBobi MatanBobi closed this Nov 28, 2021
@ryan-sandy
Copy link
Author

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.

@ryan-sandy
Copy link
Author

If you need more evidence, this person experienced the same issue.

#897

@MatanBobi
Copy link
Member

MatanBobi commented Dec 5, 2021

Hi @ryan-sandy, first thing, I don't want you to get the impression that your effort isn't valued, it is.
For this issue in particular, my opinion is that if we document every problem that people have when using third party packages with testing-library, we'll get irrelevant docs that "drown" the reader with somewhat irrelevant data (I'm trying to think about scale here).
If people can find a solution for this problem in a search, I won't be too eager to add it.
Having said that, I'll re-open this for more feedback from other folks :)

@MatanBobi MatanBobi reopened this Dec 5, 2021
@alexkrolick
Copy link
Collaborator

So the issue here is that the example imports from history, which is a dependency of react-router. Typically importing from a nested dep is discouraged and a lot of people have lint rules about it. So the course of action is to install the package as a devDep in the parent package. But, since it's not a peerDep of react-router, the package manager doesn't care if you install the matching version or not. This can happen any time, not just on RR v5.

Possible fixes:

  • Recommend checking the compatible version of history in the package.json of React Router:
  • Recommend not installing history at the top level and ignoring any lint rules that tell you to do it
  • Find a way to write this test without importing from history. Maybe RR re-exports what we need?
    • I thought at some point we wanted to use BrowserRouter which uses the window's history global, but maybe that fell out of favor (there are definitely limitations to that approach in JSDOM)

@ryan-sandy
Copy link
Author

@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 history property. This means you cannot set initial states, which limits your testing capacity.

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 useHistory hook.

@MatanBobi
Copy link
Member

MatanBobi commented Dec 7, 2021

  • Recommend not installing history at the top level and ignoring any lint rules that tell you to do it
    • Find a way to write this test without importing from history. Maybe RR re-exports what we need?

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 history package directly so I guess they are not re-exporting it.

As for the other options, I think I'm in favor of recommending to check the compatible history version. I don't like to tell people to turn off their developer tooling (even though that's not an issue related to our packages).

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.

3 participants