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

@testing-library/dom should be a peer dependency #1103

Closed
magnattic opened this issue Aug 5, 2022 · 6 comments
Closed

@testing-library/dom should be a peer dependency #1103

magnattic opened this issue Aug 5, 2022 · 6 comments

Comments

@magnattic
Copy link

magnattic commented Aug 5, 2022

Problem description:

  • Having multiple versions of @testing-library/dom in the same project leads to bugs that are hard to catch, like confusing act warnings
  • To avoid this, we have to make sure that only one version of @testing-library/dom is installed
  • This is made hard by these facts:
    • @testing-library/dom is a dependency to @testing-library/react
    • @testing-library/user-event has a peer dependency to @testing-library/dom
    • When using yarn pnp in strict mode, a package can only access a peer dependency if the direct parent has the package as a dependency (which makes sense)

So if I use both @testing-library/react and @testing-library/user-event (a common use case), my dependency tree needs to look like this right now:

my-project@npm:1.0.0
│  ├─ @testing-library/dom@npm:8.16.0     <--- possible conflict!
│  ├─ @testing-library/user-event@npm:14.2.5
│  ├─ @testing-library/react@npm:13.3.0
│  │  └─ @testing-library/dom@npm:8.16.0  <--- possible conflict!

And now it's easy for the two versions of @testing-library/dom to diverge, leading to bugs. I have to use solutions like yarn's resolutions field to keep them in sync.

Suggested solution:

Make all packages use the same instance of @testing-library/dom by making it a peer dependency of @testing-library/react.

I understand that this has the downside that the user has to install @testing-library/dom alongside the react package, even if he doesn't need it. But as long as multiple testing-library packages need the same version of @testing-library/dom, this seems to be the cleanest solution to ensure that everything works correctly.

If it was a peer dependency, the above dependency tree would look like this:

my-project@npm:1.0.0
│  ├─ @testing-library/dom@npm:8.16.0
│  ├─ @testing-library/user-event@npm:14.2.5
│  ├─ @testing-library/react@npm:13.3.0

Only one instance, problem solved!

@eps1lon
Copy link
Member

eps1lon commented Aug 5, 2022

The thing is that you can avoid package duplication with npm dedupe or yarn dedupe. But requiring another install step when we move it to peer dependencies is a hard sell since it's another step.

Typically, if you're in a React test you always import from @testing-library/react not /dom. Do you have a repro where duplicated /dom versions cause issues?

@magnattic
Copy link
Author

magnattic commented Aug 8, 2022

Afaik yarn dedupe is an optimization feature to reduce the size of dependencies. It should not be necessary to run this to avoid bugs caused by conflicting deps.

I don't have a reproduction repo right now since I am working on a private company repo, but I can try to create one.
However, I am convinced that many problems seen in this issue here can be traced back to people having installed two different versions of @testing-library/dom: #1051

See for example:

But requiring another install step when we move it to peer dependencies is a hard sell since it's another step.

Is it another step though? This is also required for @testing-library/user-event and in the install guide there it simply says to run
yarn add --dev @testing-library/user-event @testing-library/dom

Something similar could be added for the install docs of @testing-library/react.

Typically, if you're in a React test you always import from @testing-library/react not /dom.

From my experience, this will not be enough to solve the act() warnings, if you use @testing-library/user-event.

Then @react-testing-library/user-event will use version A of the @react-testing-library/dom and @react-testing-library/react will use version B, leading to the problems.

@boliveira
Copy link

I agree with @magnattic, @testing-library/dom should be a peer dependency. Today, it is a dependency of the following packages:

@testing-library/react
@testing-library/cypress
eslint-plugin-jest-dom

If you have all these installed in the same project, you can easily run into problems like act warnings when it should not happen.

@ingmarh
Copy link

ingmarh commented Jan 17, 2023

I can confirm the issue of multiple @testing-library/dom versions leading to unnecessary act() warnings.

In our case, it happened when updating to React 18 in a project with @testing-library/react and eslint-plugin-jest-dom. These dependencies were using a different version of @testing-library/dom each, which led to unnecessary act() warnings that were resolved by reinstalling so that only one version of @testing-library/dom is used.

@Kurt-von-Laven
Copy link

Kurt-von-Laven commented Sep 28, 2023

The thing is that you can avoid package duplication with npm dedupe or yarn dedupe. But requiring another install step when we move it to peer dependencies is a hard sell since it's another step.

This issue doesn't affect npm users since npm doesn't have strict peer dependencies. yarn dedupe can only deduplicate packages with overlapping version ranges, so that doesn't solve the problem unless you specify a very broad version range (e.g., *) for @testing-library/dom. This isn't a very intuitive solution though since this isn't how peer dependencies are typically handled in Yarn, and it requires a relatively deep understanding of package management. I would guess that most Yarn users don't use yarn dedupe, and to support this workaround, they will suddenly have to remember to run it every time they want to upgrade @testing-library/dom, which is from their point of view a random transitive dependency. The out-of-box experience for most Yarn users is that @testing-library/user-event doesn't work, and the docs specifically instruct them not to take the action that would resolve their problem:

If you use one of the framework wrappers, it is important that @testing-library/dom is resolved to the same installation required by the framework wrapper of your choice.
Usually this means that if you use one of the framework wrappers, you should not add @testing-library/dom to your project dependencies. ~ https://testing-library.com/docs/user-event/install/

npm packages with peer dependencies typically suggest installing the package alongside its peer dependencies in a single command. This command is often presented in a view with a button to copy it in the interest of convenience (as yours already is).

@MatanBobi
Copy link
Member

MatanBobi commented Nov 12, 2024

Resolving this one since as of version 16, @testing-library/dom is now a peer dependency.

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

No branches or pull requests

6 participants