storybookjs / storybook

Storybook is the industry standard workshop for building, documenting, and testing UI components in isolation
https://storybook.js.org
MIT License
84.1k stars 9.25k forks source link

[@storybook/test] Move `@testing-library/dom` to a peerDependency #27532

Open shilman opened 3 months ago

shilman commented 3 months ago

Discussed in https://github.com/storybookjs/storybook/discussions/27501

Originally posted by **MatanBobi** June 1, 2024 ### Is your feature request related to a problem? Please describe. In React Testing Library, we've experienced multiple issues that occur once people install dom-testing-library on their own (as part of `user-event` for example) and also install a library that bundles DTL (like `@storybook/test` and also `@testing-library/react`). These issues occur once there's a version mismatch and multiple version of DTL get installed. ### Describe the solution you'd like Moving DTL to a peerDependency will help with solving this issue. ### Describe alternatives you've considered _No response_ ### Are you able to assist to bring the feature to reality? yes, I can ### Additional context I'm a member of the testing-library organization and due to multiple issues we had with multiple version of DTL installed, we've decided to create a PR to move DTL to a peerDependency in React-Testing-Library: https://github.com/testing-library/react-testing-library/pull/1305 This is something we've thought about for quite a long time and we highly recommend other libraries in the testing-library ecosystem to also do. I'm here and available to assist in any issues that occur or even contribute this change on my own. Feel free to reach out to me or any of us from the testing-library organization.
kasperpeulen commented 3 months ago

@MatanBobi Our setup is a bit complicated.

We prebundle testing-library with tsup:

image

But because tsup somehow messes the types up during prebundling, we have @testing-library/dom, @testing-library/jest-dom and @testing-library/user-event as a direct dependency, just for the types. But for the runtime of those packages we prebundle them.

We are considering 2 different scenario's.

  1. We stop prebundling @testing-library/dom, @testing-library/jest-dom and @testing-library/user-event, and make it a peer dependency as you suggested.

In this case, should all of them be a peer dep?

  1. We continue prebundling and try to find a workaround for the tsup issues, so that it will be an actual devDependency instead.

Will this also solve your issue?

MatanBobi commented 3 months ago

AFAIR, @testing-library/jest-dom doesn't rely on @testing-library/dom at all so you can keep that there. Regarding @testing-library/user-event, I think it can also remain there as it already has a peer dependency on @testing-library/dom and AFAIK it doesn't have any specific state that might conflict. So we can ask users to only install @testing-library/dom as a peer dependency :)

kasperpeulen commented 3 months ago

Thanks for the reply @MatanBobi

And if we completely pre-bundle all of them (so make them devDependencies instead), would that also solve your issue?

MatanBobi commented 3 months ago

Hmmm, it's a good question. I'm not that much into the details of how the module resolution will work in that case. The problem we had was that inside RTL people who were using user-event sometimes received the DTL bundled with RTL and sometimes the one they've installed for user-event. Since I didn't experience this issue on my own but opened it due to a user commenting they're experiencing this issue I might not be the right person to say if it will work or not. Here's the comment that raised the issue: https://github.com/testing-library/react-testing-library/issues/1216#issuecomment-1888971931

kasperpeulen commented 3 months ago

Okay, thanks for the context!

I think that issue is similar as this issue: https://github.com/storybookjs/storybook/issues/27173

I just merged a PR, that at least uses the latest version of DTL, which I think will mitigate this issue a bit.

I do agree that we either should go for peerDeps or devDeps to fully resolve the issue. I think that devDeps (prebundling) probably would be even more ideal, as it comes to this issue specifically. Will keep you updated.

MatanBobi commented 3 months ago

I think that issue is similar as this issue: #27173

Yes that definitely looks similar. Thanks @kasperpeulen for the quick replies and attention :)

yannbf commented 3 months ago

Writing down some thoughts while talking to @kasperpeulen:

MatanBobi commented 3 months ago

Thanks @yannbf, I totally understand regarding releasing it as part of a new major as it's indeed a breaking change (that's what we did also). Thanks again!