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
Infer jest globals from @jest/globals #62037
Conversation
@remcohaszing Thank you for submitting this PR! This is a live comment which I will keep updated. 6 packages in this PR
Code ReviewsBecause this is a widely-used package, a DT maintainer will need to review it before it can be merged. You can test the changes of this PR in the Playground. Status
Once every item on this list is checked, I'll ask you for permission to merge and publish the changes. InactiveThis PR has been inactive for 156 days — it is considered abandoned, and therefore closed! Diagnostic Information: What the bot saw about this PR{
"type": "info",
"now": "-",
"pr_number": 62037,
"author": "remcohaszing",
"headCommitOid": "c04757c650c613a837dac93fe07a7d5e5d50f1ed",
"mergeBaseOid": "40cfc02a705b4a5967ff4e9a123f34dddf318970",
"lastPushDate": "2022-08-31T07:41:35.000Z",
"lastActivityDate": "2022-09-11T15:17:44.000Z",
"hasMergeConflict": true,
"isFirstContribution": false,
"tooManyFiles": false,
"hugeChange": false,
"popularityLevel": "Critical",
"pkgInfo": [
{
"name": "expect-puppeteer",
"kind": "edit",
"files": [
{
"path": "types/expect-puppeteer/expect-puppeteer-tests.ts",
"kind": "test"
},
{
"path": "types/expect-puppeteer/index.d.ts",
"kind": "definition"
},
{
"path": "types/expect-puppeteer/package.json",
"kind": "package-meta-ok"
}
],
"owners": [
"tkrotoff",
"jfm710"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Popular"
},
{
"name": "heft-jest",
"kind": "edit",
"files": [
{
"path": "types/heft-jest/index.d.ts",
"kind": "definition"
},
{
"path": "types/heft-jest/mocked.d.ts",
"kind": "definition"
},
{
"path": "types/heft-jest/package.json",
"kind": "package-meta-ok"
}
],
"owners": [
"octogonz",
"iclanton"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Well-liked by everyone"
},
{
"name": "jest-axe",
"kind": "edit",
"files": [
{
"path": "types/jest-axe/index.d.ts",
"kind": "definition"
},
{
"path": "types/jest-axe/package.json",
"kind": "package-meta-ok"
}
],
"owners": [
"erbridge"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Popular"
},
{
"name": "jest-expect-message",
"kind": "edit",
"files": [
{
"path": "types/jest-expect-message/index.d.ts",
"kind": "definition"
},
{
"path": "types/jest-expect-message/jest-expect-message-tests.ts",
"kind": "test"
},
{
"path": "types/jest-expect-message/package.json",
"kind": "package-meta-ok"
}
],
"owners": [
"mike-d-davydov",
"saitonakamura"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Popular"
},
{
"name": "jest-image-snapshot",
"kind": "edit",
"files": [
{
"path": "types/jest-image-snapshot/index.d.ts",
"kind": "definition"
},
{
"path": "types/jest-image-snapshot/jest-image-snapshot-tests.ts",
"kind": "test"
},
{
"path": "types/jest-image-snapshot/package.json",
"kind": "package-meta-ok"
}
],
"owners": [
"dawnmist",
"erbridge",
"peterblazejewicz"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Popular"
},
{
"name": "jest",
"kind": "edit",
"files": [
{
"path": "types/jest/index.d.ts",
"kind": "definition"
},
{
"path": "types/jest/jest-tests.ts",
"kind": "test"
},
{
"path": "types/jest/package.json",
"kind": "package-meta-ok"
}
],
"owners": [
"NoHomey",
"jwbay",
"asvetliakov",
"alexjoverm",
"epicallan",
"ikatyang",
"wsmd",
"JamieMason",
"douglasduteil",
"ahnpnl",
"UselessPickles",
"r3nya",
"hotell",
"sebald",
"andys8",
"antoinebrault",
"gstamac",
"ExE-Boss",
"quassnoi",
"Belco90",
"tonyhallett",
"ycmjason",
"pawfa",
"regevbr",
"gerkindev",
"domdomegg"
],
"addedOwners": [],
"deletedOwners": [],
"popularityLevel": "Critical"
}
],
"reviews": [
{
"type": "stale",
"reviewer": "SimenB",
"date": "2022-08-30T14:14:43.000Z",
"abbrOid": "3c40987"
},
{
"type": "stale",
"reviewer": "mrazauskas",
"date": "2022-08-30T13:55:11.000Z",
"abbrOid": "5dd2387"
}
],
"mainBotCommentID": 1231662208,
"ciResult": "fail",
"ciUrl": "https://github.com/DefinitelyTyped/DefinitelyTyped/commit/c04757c650c613a837dac93fe07a7d5e5d50f1ed/checks?check_suite_id=8064505067"
} |
🔔 @tkrotoff @jfm710 @octogonz @iclanton @erbridge @mike-d-davydov @saitonakamura @dawnmist @peterblazejewicz @NoHomey @jwbay @asvetliakov @alexjoverm @epicallan @ikatyang @wsmd @JamieMason @douglasduteil @ahnpnl @UselessPickles @r3nya @Hotell @sebald @andys8 @antoinebrault @gstamac @ExE-Boss @quassnoi @Belco90 @tonyhallett @ycmjason @pawfa @regevbr @GerkinDev @domdomegg — please review this PR in the next few days. Be sure to explicitly select |
@remcohaszing The CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review. |
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.
Yay, thanks for picking this up!
There's probably a whole bunch of utility types jest is missing.
Also the jasmine types should probably stay somehow (jest-jasmine2
is still maintained
[n: number]: T; | ||
} | ||
} | ||
// Minimum TypeScript Version: 4.5 |
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.
Should be 4.3 (only jsdom types require 4.5).
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.
All should be fine with 4.2 as well. If it helps to resolve errors with depends.
Type tests are running on 4.3 in Jest repo. The limitation comes from current setup, but it might be possible to rework it. I just didn't find time yet (;
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 failed on jest-mock
for lower versions.
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.
Interesting! Sounds like a bug - as mentioned we use TS 4.3 in our own type tests
declare var xit: typeof import('@jest/globals').xit; | ||
declare var test: typeof import('@jest/globals').test; | ||
declare var xtest: typeof import('@jest/globals').xtest; | ||
declare var jest: typeof import('@jest/globals').jest; |
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.
Does this re-export namespace stuff? That was an issue in https://github.com/facebook/jest/pull/12856/files#r945243675
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.
The jest
namespace implementation could follow the code of 'packages/jest/src/index.ts' in the above mentioned PR.
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.
Does this re-export namespace stuff?
It does not, but it could!
It’s just a bit annoying these exports need to be hand-picked.
/cc @mrazauskas who has done a bunch of work on Jest's types |
Also, this will certainly be horribly breaking, so should probably land as part of a future Jest v30 (no timeline) |
@remcohaszing The CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review. |
So far it doesn’t look that bad actually. But yeah, this would have definitely been nice for 29.0.0 😅 |
@remcohaszing The CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! Note: builds which are failing do not end up on the list of PRs for the DT maintainers to review. |
function mocked<T>(item: T, deep?: false): mocked.MaybeMocked<T>; | ||
function mocked<T>(item: T, deep: true): mocked.MaybeMockedDeep<T>; |
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.
Is this really necessary? Jest has jest.mocked()
helper (it was moved from ts-jest
), is that not sufficient?
By the way, typings of jest.mocked()
were reworked recently and the copy above is already outdated. For reference see: https://github.com/facebook/jest/blob/835a93666a69202de2a0429cd5445cb5f56d2cea/packages/jest-mock/src/index.ts#L34-L92, and: https://github.com/facebook/jest/blob/835a93666a69202de2a0429cd5445cb5f56d2cea/packages/jest-mock/src/index.ts#L1219-L1230
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.
So should this be changed to the following then?
declare global mocked: typeof import('@jest/globals').mocked
I’m not familiar with heft-jest
. It’s just affected by the updates.
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 was looking through Heft’s docs. The way they use Jest does not work with ts-jest
, so to have mocked()
working they copied it to @types/heft-jest
. Seemed like they are fine to deprecate @types/heft-jest
after Heft will ship Jest v28, which has jest.mocked()
builtin. I think that would be the best solution. See microsoft/rushstack#3609
import { | ||
configureToMatchImageSnapshot, | ||
MatchImageSnapshotOptions, | ||
toMatchImageSnapshot, | ||
updateSnapshotState, | ||
} from 'jest-image-snapshot'; | ||
|
||
declare const it: any; |
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.
Could this be typed through @types/jest
? Or perhaps simply to import {it} 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.
The value of it
is actually irrelevant. It just makes the tests look more representative. If it’s imported from @types/jest
or @jest/globals
, this would add a dependency (dev dependencies are not allowed in DefinitelyTyped).
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.
Fine with me. It just felt like a regression. I am not a maintainer of any @types/*
packages, just wanted to draw attention.
<T = any>(actual: T, message: string): JestMatchers<T>; | ||
(actual: any, message: string): Matchers<void>; |
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.
Just to be sure. Removal of the generic could possibly mess up other matchers. For example, does it work with .resolves
chainer?
@remcohaszing Unfortunately, this pull request currently has a merge conflict 😥. Please update your PR branch to be up-to-date with respect to master. Have a nice day! |
const spyInterfaceImpl: SpyInterface = {}; | ||
// @ts-expect-error | ||
jest.spyOn(spyInterfaceImpl, 'method', 'get'); | ||
// @ts-expect-error | ||
jest.spyOn(spyInterfaceImpl, 'prop'); | ||
// $ExpectType SpyInstance<number, []> | ||
jest.spyOn(spyInterfaceImpl, 'prop', 'get'); | ||
// $ExpectType SpyInstance<void, [boolean]> || SpyInstance<void, [arg1: boolean]> | ||
jest.spyOn(spyInterfaceImpl, 'method'); |
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.
Just to draw attention – some useful tests might be removed accidentally. E.g., this particular test helped me to figure out that Jest’s build-in spyOn
typings do not handle optional props in interfaces. Fixed in jestjs/jest#13247
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 ideally keep most of the tests here, at least during migration. There's gonna be a bunch we don't want/need, but nice to see run the tests in the meantime
Summary: Originally proposed in react-native-community/discussions-and-proposals#592 Main changes are: - Explicitly importing the global APIs in `App.test.tsx` - Drop `types/jest` from the devDependency list Benefits of these changes will be: - Keep in line with Jest's recommended practice - Remove a dev-dependency to make dependencies slimmer References: - jestjs/jest#13133 - DefinitelyTyped/DefinitelyTyped#62037 ## Changelog [GENERAL] [CHANGED] - Switch from `types/jest` to `jest/globals` for new react-native projects Pull Request resolved: #36068 Test Plan: 1. Create a new RN project from the modified template 2. Ensure yarn test passes 3. Ensure IDEs do not complain about `App.test.tsx` Reviewed By: cipolleschi Differential Revision: D43080151 Pulled By: cortinico fbshipit-source-id: c9161cb930c78f3a1eaa08ccb6483aa38d52fa3c
@remcohaszing To keep things tidy, we have to close PRs that aren't mergeable and don't have activity in the last month. No worries, though — please open a new PR if you'd like to continue with this change. Thank you! |
Summary: Originally proposed in react-native-community/discussions-and-proposals#592 Main changes are: - Explicitly importing the global APIs in `App.test.tsx` - Drop `types/jest` from the devDependency list Benefits of these changes will be: - Keep in line with Jest's recommended practice - Remove a dev-dependency to make dependencies slimmer References: - jestjs/jest#13133 - DefinitelyTyped/DefinitelyTyped#62037 ## Changelog [GENERAL] [CHANGED] - Switch from `types/jest` to `jest/globals` for new react-native projects Pull Request resolved: facebook#36068 Test Plan: 1. Create a new RN project from the modified template 2. Ensure yarn test passes 3. Ensure IDEs do not complain about `App.test.tsx` Reviewed By: cipolleschi Differential Revision: D43080151 Pulled By: cortinico fbshipit-source-id: c9161cb930c78f3a1eaa08ccb6483aa38d52fa3c
Jest provides their own types. If I’m correct (based on #44365), @SimenB, the maintainer of Jest, envisions
@types/jest
to be a way for people to opt into the use of type safe jest globals. These globals are defined in the@jest/globals
package.With these changes, all this package does is expose Jest globals from the
@jest/globals
package.This also removes a bunch of types that no longer exist in Jest, such as
jasmine
,spyOn
, anddone.fail()
. This also fixes incompatibilities between the builtin Jest types and these manually written type definitions, which causes issues such as #62025.The minimum TypeScript version has been changed to 4.5, because the upstream types require this.
Closes #34617
Closes #36299
Closes #44365
Closes #59960
Closes #60546
Closes #62025
Please fill in this template.
npm test <package to test>
.If changing an existing definition:
@jest/globals
package #44365