-
Notifications
You must be signed in to change notification settings - Fork 451
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: drop old @types/jest^27 #3513
chore: drop old @types/jest^27 #3513
Conversation
Pull Request Test Coverage Report for Build 2448505546
💛 - Coveralls |
Interesting. It doesn't reproduce in CI flow. However, if you checkout this branch and: git status # should show zero staged or unstaged changes
git clean -xdf # let's make sure there are no untracked files in the repo
cd examples/ts-only
npm install
npm test you'll probably see the error mentioned above. I also deleted "devDependencies": {
- "jest": "^28.0.2",
- "ts-jest": "28.0.0-next.2",
+ "jest": "^28.0.0",
+ "ts-jest": "^28.0.0",
"typescript": "~4.5.2"
}
} to make sure the latest 28.x versions are installed, but unfortunately I had the same luck. |
Ah, I know why. That's because |
actually you should remove this line Line 74 in 7a7aba9
it is not a breaking change since we removed The dev dependencies have no impact on published package. The example apps need to be updated though, probably switch all to |
So, I removed "@types/jest" now from everywhere — I hope it does not conflict the vision of the project. |
We indeed need to use |
Oh, now we're at the heart of the issue. 😄 I'm afraid
I'm not proficient enough in this part of TypeScript syntax, but I suspect As a weak confirmation, I force-pushed now a change to diff --git a/tsconfig.json b/tsconfig.json
index e9b685be..42d01c29 100644
--- a/tsconfig.json
+++ b/tsconfig.json
@@ -17,6 +17,6 @@
"target": "es2015",
"module": "ESNext",
"lib": ["esnext"],
- "types": ["node", "jest"]
+ "types": ["node", "@jest/globals"]
}
} |
I think I might need your help to advance further with this. I'm not sure if this is an issue with Jest 28 or with TS-Jest itself. 😢 |
I think you just need to find any places where we are using It is the issue caused by removing the type package from dependencies. If we simply add import jest from |
@ahnpnl this is exactly what I am putting under the doubt. See my repo which reproduces the issue with Jest and TypeScript only: https://github.com/noomorph/tsc-jest-28-issue P. S. Just to be on the same page, I am at zero level with your project and its history and peculiarities. I can't triage whether it is miscoordination between your team and Jest team, or it is someone's specific bug, or maybe it is an intended behavior, so I need a perspective from someone knowledgeable. But definitely it is an interesting problem that probably has to be addressed eventually by someone. It just seemed so weird that I must use |
tsconfig.json
Outdated
@@ -17,6 +17,6 @@ | |||
"target": "es2015", | |||
"module": "ESNext", | |||
"lib": ["esnext"], | |||
"types": ["node", "jest"] | |||
"types": ["node", "@jest/globals"] |
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.
This one is the wrong one. This won’t work. You need to remove @jest/globals
here and then open each test file which we are doing jest.
and add an import statement.
import { jest } from ‘@jest/globals’;
An example is
import { jest } from '@jest/globals' |
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.
Previously when we have typing package @types/jest
, typescript compiler understands jest
as namespace. Now we don’t have that namespace anymore so it throws errors and the package @jest/globals
doesn’t expose namespace
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.
@ahnpnl sounds like a catastrophic change for anyone who's using Jest (i.e. one has to augment every single test file with an explicit import ...
), doesn't it?
See the repo I mentioned: https://github.com/noomorph/tsc-jest-28-issue
The problem I'm seeing is that describe
, beforeEach
are not seen as globals for some reason.
If you mean specific tests which use jest.
— okay, I'll do it. But I thought that the problem is a bit deeper than just a single variable.
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.
Another way is something like
declare var jest: typeof import(‘@jest/globals’).jest;
in a d.ts
file, I hope it works the same as import everywhere. The same applies to describe
and others.
Also make sure tsconfig picks that d.ts
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.
What about the other global vars?
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 think the best way to reduce the imports per test file is declaring a namespace globally, or declaring several global vars. It’s more like replicating what @types/jest
does.
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.
@ahnpnl yeah, sort of. I'll make a draft for a typings file and submit it for a review.
But still I feel like it is not morally correct to fix it only in the scope of a single repo — partially because it doesn't sit well in my head how @types/jest
is going to live from now on... 🤔 Will they have 28th version doing exactly what you suggested or not. Won't it create a conflict with jest => node_modules/jest/package.json -> types
and jest => node_modules/@types/jest -> ...
?
If they won't do it, then still it can't be that every project using Jest and TS has to redeclare these globals independently.
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.
or even better, @jest/globals
should change to something like vitest/globals
https://vitest.dev/config/#globals
See their package.json entry https://github.com/vitest-dev/vitest/blob/03a54e0651f51b739b7833817d14d2cbcf474ede/packages/vitest/package.json#L32 Jest can do something like this, which helps a lot to migrate. This is exactly what we discuss here about declaring global var https://github.com/vitest-dev/vitest/blob/03a54e0651f51b739b7833817d14d2cbcf474ede/packages/vitest/globals.d.ts#L1
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.
This discussion is long, but if you want globals typed you need to use @types/jest
. I hope to land DefinitelyTyped/DefinitelyTyped#44365 at some point, but that hasn't happened yet. @jest/globals
is not meant to be used in types
in tsconfig.json
, it's made to be explicitly imported
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 thanks for shedding light on the DT situation. So it doesn't go away, ok.
Today I have some progress, but the tests are failing for E2Es still. Need to look there separately. |
src/legacy/config/config-set.spec.ts
Outdated
let readConfig!: SpyInstance<(...args: any[]) => { config?: any; error?: ts.Diagnostic }> | ||
let parseConfig!: SpyInstance<(...args: any[]) => ts.ParsedCommandLine> |
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.
let readConfig!: SpyInstance<(...args: any[]) => { config?: any; error?: ts.Diagnostic }> | |
let parseConfig!: SpyInstance<(...args: any[]) => ts.ParsedCommandLine> | |
let readConfig!: SpyInstance<typeof ts.readConfigFile> | |
let parseConfig!: SpyInstance<typeof ts.parseJsonConfigFileContent> |
Seems like jest.Mocked
would do better. Working on it (jestjs/jest#12727).
Interestingly jest.mocked()
can’t be used as replacement. I was in doubt if both of them are needed. Thanks for motivation. I will push forward with jest.Mocked
.
Would you pls rebase? I think all good to go |
Rebased. |
Hm, interesting. E2Es just hanged up. I'll try to look out for what went wrong but I might need some help maybe. Meanwhile, I'll fix lint issue and maybe try to debug 1-2 e2e tests. |
Great PR! How could this move forward? npm9 is throwing errors because of mismatch peerDependencies... |
Currently this PR only impacts internally. In release 28.0.4 |
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've included the custom global types to e2e
which makes type check happy. We can merge this.
@ahnpnl thanks a lot for looking into this! Although, it makes me a bit uncomfortable that there are multiple repetitive files now in |
For now we can keep those duplicated files. Once Jest provides the global typings, those files can be removed later. |
Summary
I see that Jest 28 brings its own types, so probably there's no sense to maintain twice the same typings in your example apps.
Test plan
Actually, for some reason, I see failures. Could you explain why?
Does this PR introduce a breaking change?
Other information