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

Is it possible to have a version not depending on jest, @jest/globals ? #608

Closed
ddolcimascolo opened this issue Jul 5, 2024 · 14 comments · Fixed by #610
Closed

Is it possible to have a version not depending on jest, @jest/globals ? #608

ddolcimascolo opened this issue Jul 5, 2024 · 14 comments · Fixed by #610

Comments

@ddolcimascolo
Copy link
Contributor

Describe the feature you'd like:

A version of this package that does not have jest / @jest/globals as peer dependencies. These dependencies are installed automatically by npm (in recent versions) and if you're using Vitest and not Jest, you still end up installing tons of jest-related dependencies.

Any advice on how this could get implemented? I'm willing to help btw :)

Suggested implementation:

Describe alternatives you've considered:

Teachability, Documentation, Adoption, Migration Strategy:

@gnapse
Copy link
Member

gnapse commented Jul 5, 2024

Any advice on how this could get implemented? I'm willing to help btw :)

That's what I was planning to ask you as I read the first paragraph 😅

I guess we could try to make jest only a dev dependency, and remove the automatic setup entry points. That is, the "how to use instructions" will no longer include the ability to simply import a module from this library. The instructions will always be around this way of using the library, documented in our README:

With another Jest-compatible expect

If you are using a different test runner that is compatible with Jest's expect
interface, it might be possible to use it with this library:

import * as matchers from '@testing-library/jest-dom/matchers'
import {expect} from 'my-test-runner/expect' // <- change this import path to jest or vitest or whatever

expect.extend(matchers)

There's the issue of the name, but I think that's secondary. It can always be named jest-dom. Though confusing, but it can be made independent of jest.

This would have to be a breaking change, but if it works, we can do it.

@ddolcimascolo
Copy link
Contributor Author

What do you think of splitting this package into N packages with

  • A core package (something like expect-dom) holding all the matchers, and any other code not-related to integrating with a test runner
  • A jest-dom package (that depends on the core package) whose sole responsibility is to perform the integration with Jest
  • A vitest-dom package (that depends on the core package) whose sole responsibility is to perform the integration with Vitest

This can scale if more test runners get supported in the future

The breaking change will only be for Vitest users (going from jest-dom to vitest-dom). Upgrade for Jest users will be transparent

@gnapse
Copy link
Member

gnapse commented Jul 5, 2024

Yes, that could work too. But it complicates things. It involves setting up a mono-repo. I can help with reviewing and releasing, but not with implementing it.

Also, it's a change big enough (in terms of its implications) that it warrants asking the opinion of @testing-library/core-maintainers.

@kentcdodds
Copy link
Member

I've been using @jest-dom with vitest without issue. I'm not sure what the problem is...

@kentcdodds
Copy link
Member

Oh, just noticed the peerDeps. Yeah, I'd say just remove that and move on 🤷‍♂️

@kentcdodds
Copy link
Member

Though, they're all marked as optional so it's really not a big deal IMO.

@kentcdodds
Copy link
Member

These dependencies are installed automatically by npm (in recent versions) and if you're using Vitest and not Jest, you still end up installing tons of jest-related dependencies.

In my experience this is not true. For example, in the epic stack:

npm ls @jest/globals
epic-stack-template@ /Users/kentcdodds/code/epicweb-dev/epic-stack
└── (empty)

The only package in that project from @jest is @jest/schemas and that's coming from vitest:

npm ls @jest/schemas
epic-stack-template@ /Users/kentcdodds/code/epicweb-dev/epic-stack
└─┬ [email protected]
  ├─┬ @vitest/[email protected]
  │ └─┬ [email protected]
  │   └── @jest/[email protected]
  └─┬ @vitest/[email protected]
    └─┬ [email protected]
      └── @jest/[email protected] deduped

I'm pretty sure this is not an issue.

@ddolcimascolo
Copy link
Contributor Author

ddolcimascolo commented Jul 5, 2024

Thanks for checking Kent, let me double check why I have these installed then. My npm ls clearly states that they come from jest-dom but the setup in my project being a bit complex (I'm thinking npm workspaces for instance) I might have something else along the way.

David

@ddolcimascolo
Copy link
Contributor Author

ddolcimascolo commented Jul 5, 2024

Ah @kentcdodds, the epic stack project uses the legacy-peer-deps npm option :) it's in .npmrc

For the record: https://docs.npmjs.com/cli/v10/using-npm/config#legacy-peer-deps

@kentcdodds
Copy link
Member

Oh yeah, this is because the new peer deps behavior is stupid. And I guess this is one more reason to disable it 🤷‍♂️

@kentcdodds
Copy link
Member

Anyway, not everyone's gonna want to disable it so I guess just remove the peerDeps and move on. Much better than complicating things with a ton of packages

@ddolcimascolo
Copy link
Contributor Author

Great. Do you want me to open a PR for that?

@ddolcimascolo
Copy link
Contributor Author

Done #610

@ardeois
Copy link

ardeois commented Sep 24, 2024

This PR seems to have broken jest-dom when using pnpm

PNPM by default will not hoist packages to the top level node_modules, which means if jest-dom imports vitest it will fail unless:

  • jest-dom packages declares vitest as peer dependency
  • We override peerDependencies for jest-dom package in our project's package using pnpm.packageExtensions option

Having vitest declared as an optional peer dependency was I think the best option, I'm not sure to understand why this was removed?

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 a pull request may close this issue.

4 participants