-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
fix(build): Resolve overshadowed types for Vitest global API #3923
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@kamilogorek is attempting to deploy a commit to the trpc Team on Vercel. A member of the Team first needs to authorize it. |
Related |
@@ -78,7 +78,8 @@ | |||
"@examples/trpc-next-prisma-starter>prisma": "https://registry.npmjs.com/prisma/-/prisma-4.10.1.tgz?id=%40examples%2Ftrpc-next-prisma-starter", | |||
"@examples/next-websockets-starter>prisma": "https://registry.npmjs.com/prisma/-/prisma-4.10.1.tgz?id=%40examples%2Fnext-websockets-starter", | |||
"@examples/trpc-next-prisma-todomvc>prisma": "https://registry.npmjs.com/prisma/-/prisma-4.10.1.tgz?id=%40examples%2Ftrpc-next-prisma-todomvc", | |||
"@examples/legacy-next-starter>prisma": "https://registry.npmjs.com/prisma/-/prisma-4.10.1.tgz?id=%40examples%2Flegacy-next-starter" | |||
"@examples/legacy-next-starter>prisma": "https://registry.npmjs.com/prisma/-/prisma-4.10.1.tgz?id=%40examples%2Flegacy-next-starter", | |||
"@types/testing-library__jest-dom": "link:./packages/tests/vendor/@types/testing-library__jest-dom" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this as a package even? Can we not just import the file in setupTests and it'll be registered?
Or is it a peerDep for something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's an indirect transitive dependency that we have to override, explained it in detail here #3916
@@ -0,0 +1,17 @@ | |||
// Type definitions for @testing-library/jest-dom 5.14 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are currently using @testing-library/jest-dom
version 5.16.5
vs 5.14
here, I guess we should update matchers.d.ts
to use the same version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were no API changes since 5.14
and those are the latest available types, so I left the version comment intact to make it easy to realize which version is vendored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 👍🏼
Closes #3916
🎯 Changes
This PR vendors
@types/testing-library__jest-dom
package and modifies it so that it works nicely with Vitest.The only required modification was removing the
jest
triple-slash reference declaration.@testing-library/jest-dom
is rather a complete package, with very sparse updates/bugfixes, so I believe vendors it is the right move here. And if anything, only types need to be bumped, which are all inmatchers.d.ts
that is 1:1 copied and has tool-based formatting disabled, so making a git-diff will not be an issue in the future.Note that matches still live under
jest
namespace and it's not an oversight during vendoring - https://github.com/vitest-dev/vitest/blob/0280825f1a672ba23d9fcbbc6470a4b29d343a73/packages/vitest/src/types/global.ts#L38-L40All redundant imports of
'@testing-library/jest-dom'
were removed, as it's already registered as a part ofsetupTests.ts
which is executed for allpackages/tests
test cases.TypeScript LSP is working as expected now and all CI checks are passing:
✅ Checklist