react-testing-library icon indicating copy to clipboard operation
react-testing-library copied to clipboard

@testing-library/dom should be a peer dependency

Open magnattic opened this issue 1 year ago • 5 comments

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!

magnattic avatar Aug 05 '22 12:08 magnattic

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?

eps1lon avatar Aug 05 '22 20:08 eps1lon

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: https://github.com/testing-library/react-testing-library/issues/1051

See for example:

  • https://github.com/testing-library/react-testing-library/issues/1051#issuecomment-1183307612
  • https://github.com/testing-library/react-testing-library/issues/1051#issuecomment-1189995643

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.

magnattic avatar Aug 08 '22 10:08 magnattic

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.

boliveira avatar Jan 17 '23 10:01 boliveira

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.

ingmarh avatar Jan 17 '23 14:01 ingmarh

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).

Kurt-von-Laven avatar Sep 28 '23 21:09 Kurt-von-Laven