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

Infer jest globals from @jest/globals #62037

Closed

Conversation

remcohaszing
Copy link
Contributor

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, and done.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.

If changing an existing definition:

  • Provide a URL to documentation or source code which provides context for the suggested changes: fix(jest): use @jest/globals package #44365
  • If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header.

@typescript-bot
Copy link
Contributor

typescript-bot commented Aug 30, 2022

@remcohaszing Thank you for submitting this PR!

This is a live comment which I will keep updated.

6 packages in this PR

Code Reviews

Because 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

  • ❌ No merge conflicts
  • ❌ Continuous integration tests have failed
  • 🕐 A DT maintainer needs to approve changes which affect more than one package

Once every item on this list is checked, I'll ask you for permission to merge and publish the changes.

Inactive

This 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"
}

@typescript-bot
Copy link
Contributor

typescript-bot commented Aug 30, 2022

@typescript-bot typescript-bot added the The CI failed When GH Actions fails label Aug 30, 2022
@typescript-bot typescript-bot moved this from Waiting for Code Reviews to Needs Author Action in New Pull Request Status Board Aug 30, 2022
@typescript-bot
Copy link
Contributor

@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.

Copy link
Contributor

@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.

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
Copy link
Contributor

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).

Copy link
Contributor

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 (;

Copy link
Contributor Author

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.

Copy link
Contributor

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;
Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Contributor Author

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.

@SimenB
Copy link
Contributor

SimenB commented Aug 30, 2022

/cc @mrazauskas who has done a bunch of work on Jest's types

@SimenB
Copy link
Contributor

SimenB commented Aug 30, 2022

Also, this will certainly be horribly breaking, so should probably land as part of a future Jest v30 (no timeline)

@typescript-bot typescript-bot added Edits multiple packages and removed The CI failed When GH Actions fails labels Aug 30, 2022
@typescript-bot typescript-bot moved this from Needs Author Action to Waiting for Code Reviews in New Pull Request Status Board Aug 30, 2022
@typescript-bot typescript-bot added the The CI failed When GH Actions fails label Aug 30, 2022
@typescript-bot typescript-bot moved this from Waiting for Code Reviews to Needs Author Action in New Pull Request Status Board Aug 30, 2022
@typescript-bot
Copy link
Contributor

@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.

@remcohaszing
Copy link
Contributor Author

Also, this will certainly be horribly breaking, so should probably land as part of a future Jest v30

So far it doesn’t look that bad actually. But yeah, this would have definitely been nice for 29.0.0 😅

@typescript-bot typescript-bot removed the The CI failed When GH Actions fails label Aug 31, 2022
@typescript-bot typescript-bot moved this from Needs Author Action to Waiting for Code Reviews in New Pull Request Status Board Aug 31, 2022
@typescript-bot typescript-bot added the The CI failed When GH Actions fails label Aug 31, 2022
@typescript-bot typescript-bot moved this from Waiting for Code Reviews to Needs Author Action in New Pull Request Status Board Aug 31, 2022
@typescript-bot
Copy link
Contributor

@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.

Comment on lines +67 to +68
function mocked<T>(item: T, deep?: false): mocked.MaybeMocked<T>;
function mocked<T>(item: T, deep: true): mocked.MaybeMockedDeep<T>;
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

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;
Copy link
Contributor

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'?

Copy link
Contributor Author

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).

Copy link
Contributor

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.

Comment on lines -19 to +20
<T = any>(actual: T, message: string): JestMatchers<T>;
(actual: any, message: string): Matchers<void>;
Copy link
Contributor

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?

@typescript-bot
Copy link
Contributor

@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!

Comment on lines -597 to -605
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');
Copy link
Contributor

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

Copy link
Contributor

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

facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Feb 7, 2023
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
@typescript-bot typescript-bot added the Abandoned This PR had no activity for a long time, and is considered abandoned label Feb 14, 2023
@typescript-bot typescript-bot removed this from Needs Author Action in New Pull Request Status Board Feb 14, 2023
@typescript-bot
Copy link
Contributor

@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!

OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
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
@remcohaszing remcohaszing deleted the jest-globals branch January 25, 2024 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Abandoned This PR had no activity for a long time, and is considered abandoned Critical package Edits multiple packages Has Merge Conflict This PR can't be merged because it has a merge conflict. The author needs to update it. The CI failed When GH Actions fails
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[@types/jest] jest-each methods should allow done-callback style tests [@types/jest] TS1005: ';' expected.
4 participants