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

chore: remove @types from dependencies #472

Closed
wants to merge 2 commits into from

Conversation

wallzero
Copy link

@wallzero wallzero commented Aug 29, 2022

Fix #427

What:

Type definitions should be under devDepedencies

Why:

Types are not runtime dependencies. It is also easier for downstream projects to manage types.

How:

Moved @types/testing-library__jest-dom from dependencies to devDepedencies in package.json.

Checklist:

  • Documentation
  • Tests
  • Updated Type Definitions
  • Ready to be merged

@nickserv
Copy link
Member

nickserv commented Aug 29, 2022

We intentionally bundle the types so they don't have to be installed separately with npm. I'd also argue having these types in devDependencies is incorrect, since the types are used when Jest DOM is installed, not when it's in development.

@codecov
Copy link

codecov bot commented Sep 19, 2022

Codecov Report

Merging #472 (3bc2a60) into main (948d90f) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##              main      #472   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           26        26           
  Lines          630       630           
  Branches       236       236           
=========================================
  Hits           630       630           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@nickserv nickserv added the BREAKING CHANGE This change will require a major version bump label Sep 19, 2022
Copy link
Member

@nickserv nickserv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed my mind. I think we should just uninstall the @types package completely to fix Vitest and TypeScript support (#427). We're not using it in development anyway.

Note that I'm intentionally suggesting this as a breaking change, as it's important for accommodating Vitest and TypeScript users. Jest users would need to install this package, and we should document that before publishing a major release. I'd like another maintainer to approve this before merging ideally.

Alternatively, we could just drop the dependency on Jest in the types package, or do that as we move types into this package.

@nickserv nickserv mentioned this pull request Sep 19, 2022
2 tasks
@nickserv nickserv changed the title @types should be devDependencies chore: remove @types from dependencies Sep 19, 2022
@EzraBrooks
Copy link

Any idea when this change will be merged? This would be very helpful for me, as I'd like to drop @types/jest in favor of @jest/globals in my project but cannot because of this transitive dependency that extends the global namespace.

@kentcdodds
Copy link
Member

Vitest (is fantastic and) has made this challenging. I wonder whether there's an established pattern for supporting both since the matchers API in vitest is forked from Jest. I definitely feel like this needs to be solved.

Unfortunately I don't have bandwidth to work on this myself so I'm not going to merge this and put that burden on someone else, but I wanted to come in here and voice my support for doing something about this problem.

@kentcdodds
Copy link
Member

Oh, and personally, I'm actually ok with this change (of course, it would be breaking). It's more in line with the way most packages work with definitely typed anyway 🤷‍♂️

But it would be nice if there was a way to make the types themselves support both vitest and jest at the same time. I have no idea whether that's possible.

@jgoz
Copy link
Collaborator

jgoz commented Aug 12, 2023

@kentcdodds

But it would be nice if there was a way to make the types themselves support both vitest and jest at the same time. I have no idea whether that's possible.

#511

@github-actions
Copy link

🎉 This issue has been resolved in version 6.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BREAKING CHANGE This change will require a major version bump released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TS error when using vitest globals and @testing-library/jest-dom
5 participants