-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
do not modify global types via declarations #8571
Conversation
packages/jest-types/src/Global.ts
Outdated
} | ||
import { It, ItConcurrent, ItBase, Describe, DescribeBase } from './Global-sans-augments'; | ||
import { Jasmine } from './Jasmine'; | ||
export * from './Global-sans-augments'; | ||
|
||
declare global { |
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 should not be messing with the global at all, could you just remove this entire declare global {
block? And rollback the other changes in this PR. We might want/need some kind of JestGlobalEnv
:
interface JestGlobal extends NodeJS.Global {
// ...blahblahblah
}
but it should not mutate the actual global type environment
The TS types are mostly internal for now, they are not meant to replace @types/jest
yet. When they are it'll be the jest
package itself sticking things on global
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.
I haven't used jest in a little while, but describe()
is available globally, right? As in, users do not need to import
it. If so it needs to be declared globally somewhere for users of Jest. But I don't know if you want that to be happening within @jest/types
or somewhere 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.
I missed your last sentence about omitting the global declarations for now and eventually moving them into the jest package. Roger that.
Is there any harm in me moving the globals to the jest package right away? If people are still using @types/jest, it'll be ignored. But if they skip that and use the types bundled in jest, it'll pick up the global declaration and everything will work.
Or should it always be opt-in to get the global declarations? E.g. people can import 'jest';
without the global declarations, and they'll need to explicitly /// <reference types="jest/globals" />
to get the global types? Will anyone ever want to import jest from 'jest';
to use the API, without getting the global declarations?
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.
For now, the way to get the global type defs with Jest should be @types/jest
. THat will change at some point, but for now we do not support patching the global types
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.
@SimenB Have you hit issues where the Typescript compiler uses node_modules/jest
types, skipping node_modules/@types/jest
? That's the behavior I'm seeing. It doesn't matter if the user installs a @types
package. It'll be ignored if the non-@types
package bundles its own declarations.
https://repl.it/@AndrewBradley/SpanishHugeVirus-1
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.
no, but you can use
{
"compilerOptions": {
"paths": {
"jest": ["./node_modules/@types/jest", "./node_modules/jest"]
}
}
}
or something like that to prioritize DT's typings
Codecov Report
@@ Coverage Diff @@
## master #8571 +/- ##
==========================================
- Coverage 63.2% 63.19% -0.01%
==========================================
Files 271 271
Lines 11284 11285 +1
Branches 2749 2749
==========================================
Hits 7132 7132
- Misses 3538 3539 +1
Partials 614 614
Continue to review full report at Codecov.
|
2f34d49
to
9c05251
Compare
Another question: I see that a bunch of the jest type declarations refer to NodeJS types but don't declare a Do you want to add that dependency to those modules or instead stub out the global |
That's on purpose due to #8092 |
As far as I know this is good to go. Is there anything else that needs to be added? |
Thanks! |
Hi, I found this PR referenced here https://stackoverflow.com/questions/56181799/how-to-use-mocha-and-jest-with-typescript-without-conflicts Is there any reason why this wasn't released? Or is it released already? I am trying to use Mocha for integration test and jest for unit tests and I'm running into this issue. Thanks! |
This was released in 24.9.0 last week |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
This is a fix for #8570.
Test plan