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: replace @types/jest with a new private @jest/test-globals package #13344

Merged
merged 15 commits into from
Oct 1, 2022
Merged

chore: replace @types/jest with a new private @jest/test-globals package #13344

merged 15 commits into from
Oct 1, 2022

Conversation

mrazauskas
Copy link
Contributor

@mrazauskas mrazauskas commented Sep 30, 2022

Summary

What if @types/jest would be replaced with a new private package @jest/test-globals? Here is how I came to this:

  1. There is a need to have some .d.ts file with type declarations of Jest globals. For example, it could live in root of the repo. In the .d.ts it is unavoidable to have something like import('@jest/globals')['beforeAll'] which would reference a package like @jest/globals which might be not yet build. TypeScript will see these broken references and will not be happy.
  2. A better idea would be to keep this .d.ts file in some package and to use project references. This way TypeScript would build the referenced projects first. The problem is solved.
  3. The private @jest/type-utils looked like a good candidate to be that package. I was adding "./globals": {"types": "./build/globals.d.ts"} to package.json, but TypeScript was still not happy to see "types": ["@jest/test-utils/globals"] in tsconfig.josn. Also everything looked clumsy.
  4. A new private @jest/test-globals package only for type declaration of Jest global APIs looked like a cleaner solution. (Indeed it is somewhat similar to feat(jest): allow enabling Jest global types through "types": ["jest"] in tsconfig.json #12856.)

It might sound unnecessary to have one more private package, but I think it is worth a discussion.

First the new package has very clear task – to provide type declarations of Jest's global test APIs for tests of Jest repo. Task is clear and simple as well as the usage of the package: "types": ["@jest/test-globals"]. (Also replacing of @types/jest package with another package is sort of logic.)

One more detail to keep in mind. DefinitelyTyped/DefinitelyTyped#62037 is not a solution for Jest repo, because in that case @types/jest will include import('@jest/globals'), i.e. it will again reference packages which are not yet build. So having a private package which is replacing @types/jest makes even more sense.

One detail is left to think about:

  • examples are still using @types/jest. Since there is no future for @types/jest in this repo, I will that to use imports from @jest/globals

Test plan

Green CI.

@SimenB
Copy link
Member

SimenB commented Sep 30, 2022

It might sound unnecessary to have one more private package, but I think it is worth a discussion.

I don't mind more private packages 🙂

One more detail to keep in mind. DefinitelyTyped/DefinitelyTyped#62037 is not a solution for Jest repo, because in that case @types/jest will include import('@jest/globals'), i.e. it will again reference packages which are not yet build. So having a private package which is replacing @types/jest makes even more sense.

That make sense, but the contents of the files should probably be identical.

@SimenB
Copy link
Member

SimenB commented Sep 30, 2022

Super exciting stuff! I'll take a look tomorrow - currently on my way to a concert 🙂

@mrazauskas
Copy link
Contributor Author

Yes, I think this is the best idea so far. Have a nice evening (;

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

this is super exciting!

(came back from concert full of adrenaline, so... 😅 )

packages/babel-jest/src/__tests__/tsconfig.json Outdated Show resolved Hide resolved
@@ -5,6 +5,8 @@
* LICENSE file in the root directory of this source tree.
*/

/// <reference lib="dom" />
Copy link
Member

Choose a reason for hiding this comment

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

really? how? it's running in node env, so shouldn't need or use any DOM APIs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

ah, for rAF it makes sense 👍

/**
* Wraps a class, function or object type with Jest mock type definitions.
*/
export type Mocked<T extends object> = JestMocked<T>;
Copy link
Member

Choose a reason for hiding this comment

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

object seems weakly typed, but that's not for this PR to figure out 😀

@SimenB
Copy link
Member

SimenB commented Oct 1, 2022

conflicts 😅

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

super awesome! 👍

@SimenB SimenB merged commit eafabf9 into jestjs:main Oct 1, 2022
@mrazauskas mrazauskas deleted the chore-remove-@types/jest branch October 1, 2022 07:40
@mrazauskas
Copy link
Contributor Author

Nice! Now it would be good to run type checks for test files. I was trying out already. This will be fun ;D

@github-actions
Copy link

github-actions bot commented Nov 1, 2022

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.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants