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

feat(jest): allow enabling Jest global types through "types": ["jest"] in tsconfig.json #12856

Closed
wants to merge 20 commits into from
Closed

feat(jest): allow enabling Jest global types through "types": ["jest"] in tsconfig.json #12856

wants to merge 20 commits into from

Conversation

mrazauskas
Copy link
Contributor

@mrazauskas mrazauskas commented May 17, 2022

Closes #12415

Summary

Based on the idea from #12853 (comment) by @ahnpnl

It was suggested to employ declaration merging of global scope and allow enabling Jest global types through tsconfig.json:

"types": ["jest/globals"]

That’s really nice. After playing a bit, I figured out that it is possible to have declare global directly in index.ts of 'jest' package. Simply install 'jest', add global types through to tsconfig.json:

"types": ["jest"]

Javascript project? Install 'jest', have your JS typed. Not sure I get how this works. Hm.. Seems like in this case globals become available in all files. No control. Not sure if that is fine? In the other hand, installing 'jest' and '@types/jest' in a JS project gives the same effect.

Ah.. It is possible to opt-out through tsconfig.json. So the difference: '@types/jest' is opt-in, but adding this feature would force some JS users to opt-out. Feels breaking. Or at least annoying for sensitive people ;D

Todo

  • update documentation.

Test plan

Type test is added.

@ahnpnl
Copy link
Contributor

ahnpnl commented May 17, 2022

thanks!

@@ -0,0 +1,8 @@
{
Copy link
Contributor Author

@mrazauskas mrazauskas May 17, 2022

Choose a reason for hiding this comment

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

__tests__ and e2e directories needs additional tsconfig.json with "types": ["jest"]. That’s a lot of new files. For now I keep them all the same hoping this would help to review the PR. In few test suites some type fixing here and there is needed. Not touching these now. I will come back with fix PRs case-by-case.

Also here I have to extend the root tsconfig.json, because tsconfig.json in the enclosing directory is excluding __tests__. Seems like excludes can not be overridden.

@@ -38,7 +38,7 @@ const copyrightSnippet = `

const typesNodeReferenceDirective = '/// <reference types="node" />';

const excludedPackages = new Set(['@jest/globals']);
const excludedPackages = new Set(['@jest/globals', 'jest']);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bundling removes declare. In any case, that is just one .d.ts.

@@ -5076,16 +5075,6 @@ __metadata:
languageName: node
linkType: hard

"@types/jest@npm:^27.4.0":
Copy link
Contributor Author

Choose a reason for hiding this comment

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

And... This is gone! ;D

expect({apples: 6, bananas: 3}).toEqual({
apples: expect.toBeWithinRange(1, 10),
bananas: expect.not.toBeWithinRange(11, 20),
});
});
```

:::note

In TypeScript, when using `@types/jest` for example, you can declare the new `toBeWithinRange` matcher in the imported module like this:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This page can have more TS examples, of course. I just wanted to remove this old pain.

@mrazauskas mrazauskas marked this pull request as ready for review May 17, 2022 13:40
@@ -13,3 +13,26 @@ export {
} from '@jest/core';

export {run} from 'jest-cli';

declare global {
export const beforeEach: typeof import('@jest/globals')['beforeEach'];
Copy link
Contributor Author

@mrazauskas mrazauskas May 18, 2022

Choose a reason for hiding this comment

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

export is not needed, because global namespace is always public. I added export just as a hint for future reader that these are being exposed here.

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.

exciting! We should try to add a bunch of the helpers onto the jest namespace as well

e2e/tsconfig.json Outdated Show resolved Hide resolved
docs/UpgradingToJest29.md Outdated Show resolved Hide resolved
"extends": "../../../../tsconfig.json",
"compilerOptions": {
"composite": false,
"esModuleInterop": true,
Copy link
Member

Choose a reason for hiding this comment

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

do we need this? (applies to all)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some tests needs it. For example, commenting out "esModuleInterop": true give type error here:

https://github.com/facebook/jest/blob/ae8cf9a78cb0cd2f54d3bacbc1c8837e540e21dc/packages/pretty-format/src/__tests__/react.test.tsx#L8

My idea was to use the same tsconfig.json file for all packages to simplify review. But I can go through and enable it case by case. This option applies only for files inside __tests__ directories. Perhaps it is fine to keep it?

Copy link
Member

Choose a reason for hiding this comment

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

A separate PR making it unneeded would be awesome, but shouldn't clutter up this pr

Comment on lines +13 to +14
declare const describe: Global.TestFrameworkGlobals['describe'];
declare const test: Global.TestFrameworkGlobals['test'];
Copy link
Member

Choose a reason for hiding this comment

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

why are these needed? shouldn't they just import from @jest/globals?

Copy link
Contributor Author

@mrazauskas mrazauskas Aug 13, 2022

Choose a reason for hiding this comment

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

Ah.. I reworked these into imports, but seems like these create circular references somehow – https://github.com/facebook/jest/runs/7819204971

Edit: @jest/globals depend on expect which depends on test-utils. Unfortunately that’s circular. In contrary @jest/types depends only on @jest/schemas. Fingers crossed we will not need test-utils for schemas ;D

interface Matchers<R> {
toPrettyPrintTo(expected: unknown, options?: OptionsReceived): R;
}
declare module 'expect' {
Copy link
Member

Choose a reason for hiding this comment

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

woo!

@@ -1,22 +1,8 @@
{
"extends": "../tsconfig.json",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let’s extend this one too? Why not?

Hm.. Just noticed that "noEmit": true went missing. Shall I bring it back? And perhaps shall I use it for all __tests__?

@mrazauskas
Copy link
Contributor Author

mrazauskas commented Aug 13, 2022

We should try to add a bunch of the helpers onto the jest namespace as well

Just looked through the jest namespace again. Hm.. Not sure which ones could be useful? Perhaps the ones currently exported from jest-mock as ClassLike, FunctionLike, ConstructorLikeKeys, MethodLikeKeys, PropertyLikeKeys? Can these be useful? Anything else?

@SimenB
Copy link
Member

SimenB commented Aug 13, 2022

I'm thinking of stuff like DoneCallback, MatcherUtils etc. (and mock stuff like you mention), i.e. things you might use as arguments in helpers and whatnot. Yes, these can be imported explicitly from the different modules, but having them on jest. is useful, I think. Although, we shouldn't add too many - it's easier to add when people have use cases later than adding a bunch that are never used and maintain them

@mrazauskas
Copy link
Contributor Author

mrazauskas commented Aug 13, 2022

Right! Actual I can search on GitHub and see which ones are used:

https://github.com/araujo21x/API_adocao_animais/blob/05d9706be29ad002b4c14813bbf66982ec3fde3d/tests/user/registerONG.test.ts#L36

Edit: very exciting to look around. Even jest.TestFn type is used in helpers. Makes sense indeed. I will think about these.

@SimenB
Copy link
Member

SimenB commented Aug 13, 2022

There is also https://github.com/facebook/jest/blob/ae8cf9a78cb0cd2f54d3bacbc1c8837e540e21dc/packages/jest-util/src/__tests__/installCommonGlobals.test.ts#L11 btw.

I'm getting an error, is the namespace thing not working?
image

jest.fn() is typed correctly, fwiw

@mrazauskas
Copy link
Contributor Author

Good catch. The issue is with this PR. All works with import {jest} from '@jest/globals', so it seems like I simply did not add jest namespace to global. Found a solution. Will double check I the morning.

@mrazauskas mrazauskas marked this pull request as draft August 14, 2022 05:27
export const jest: typeof import('@jest/globals')['jest'];

// eslint-disable-next-line @typescript-eslint/no-namespace
namespace jest {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not happy about the duplication with declarations in @jest/globals.

Another problem – it was not a good idea to put everything into index file. Now import {Config} from 'jest' is also augmenting global and pulling in Jest’s globals. That’s not good. So "types": ["jest"] should be reworked into something like "types": ["jest/globals"]. This means that "exports" needs additional entry.

In a way all is fine, but at the same time the implementation becomes more and more hairy. In contrary import {expect, test} from '@jest/globals' is simple and declarative. Does not need all the hairy stuff I mentioned. The more I work on this PR, the more I am a fan of that straight forward import ;D

Copy link
Member

Choose a reason for hiding this comment

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

I think requiring types: ['jest/global-api'] or something very explicit is perfectly fine 👍

@phawxby
Copy link
Contributor

phawxby commented Aug 18, 2022

@mrazauskas just wondering what the progress of this ticket is? I believe @SimenB is hoping to get this in V29. What are the remaining items to look into, would you like me to see if I can carve out some time to help?

@mrazauskas
Copy link
Contributor Author

Brief answer: I am uncertain if this is a good idea. Have to think about it more and to do some testing.

Perhaps it is not worth to postpone the release? This PR is a feature anyways, so it could land any time later.

@SimenB
Copy link
Member

SimenB commented Aug 18, 2022

Yeah, currently leaning towards not having it in 29 - doing import type {Config} from 'jest' or import {runCLI} from 'jest' shouldn't pollute the global (TS) namespace

@mrazauskas
Copy link
Contributor Author

Closing in favour of #13344 and DefinitelyTyped/DefinitelyTyped#62037

@mrazauskas mrazauskas closed this Sep 30, 2022
@mrazauskas mrazauskas deleted the feat-global-types branch October 1, 2022 16:24
@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.

5 participants