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

Fix type definitions for promises #10600

Merged
merged 1 commit into from Oct 6, 2020
Merged

Conversation

Vinnl
Copy link
Contributor

@Vinnl Vinnl commented Oct 6, 2020

Summary

When calling mockResolvedValue (-Once), the argument should be
the expected return value unwrapped from its Promise. Likewise,
when mocking a rejected value, the passed argument needs not
necessarily be of the same type as the expected successful return
value.

Test plan

Without this change, the following code will cause TypeScript to complain:

    const mockFetch = jest.fn(window.fetch).mockResolvedValue(new Response());
Argument of type 'Response' is not assignable to parameter of type 'Promise<Response>'.
  Type 'Response' is missing the following properties from type 'Promise<Response>': then, catch, [Symbol.toStringTag], finally

That error will disappear with this PR.

@SimenB
Copy link
Member

SimenB commented Oct 6, 2020

Thanks! Could you add some tests that fails without this change to https://github.com/facebook/jest/blob/999ee461262c217db2827db79296129a13d6e23a/test-types/top-level-jest-namespace.test.ts as well?

@Vinnl
Copy link
Contributor Author

Vinnl commented Oct 6, 2020

@SimenB Thanks for the pointer, done: 3da2a2b.

(Ninja edit: had one test commented-out, sorry.)

(Edit 2: Also switched to using a regular import for the Mock type: 1e18729)

@Vinnl Vinnl force-pushed the promise-types branch 3 times, most recently from 3da2a2b to a09bd56 Compare October 6, 2020 09:34
When calling `mockResolvedValue` (-`Once`), the argument should be
the expected return value unwrapped from its Promise. Likewise,
when mocking a rejected value, the passed argument needs not
necessarily be of the same type as the expected successful return
value.
@@ -10,6 +10,7 @@
import {expectError, expectType} from 'mlh-tsd';
//eslint-disable-next-line import/no-extraneous-dependencies
import {jest} from '@jest/globals';
import type {Mock} from 'jest-mock';
Copy link
Member

Choose a reason for hiding this comment

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

Ideally we'd allow access to this without having to explicitly go through jest-mock, but that's a problem for future us 🙂 For now this is a great improvement! 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, actually running into that in my own code as well, but unfortunately I don't know how to solve that...

Copy link
Member

@SimenB SimenB Oct 6, 2020

Choose a reason for hiding this comment

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

Yeah, solving that problem is a blocker for DefinitelyTyped/DefinitelyTyped#44365

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if just a @jest/utility-types would work, which re-export e.g. Mock, some pretty-format stuff for serializers, Config etc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have enough insight on the inner workings of Jest to have a valid opinion on that, but one challenge I am running into is that the Mock type from jest-mock (i.e. the type of the return value of jest.fn()) is different from the Mock type that's available as jest.Mock from @jest/globals, and it's not clear to me why. W.r.t. to importing it from jest-mock or from @jest/utility-types, either seems fine to me.

Copy link
Member

Choose a reason for hiding this comment

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

import {jest} from '@jest/globals';
import jestMock = require('jest-mock');

jest.fn === jestMock.fn;

at least that's the intention. If that's not true, we've messed up somewhere 😅

Copy link
Contributor Author

@Vinnl Vinnl Oct 6, 2020

Choose a reason for hiding this comment

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

It doesn't seem to be true here: https://codesandbox.io/s/admiring-water-licv4?module=%2Fsrc%2Fjest.test.ts

Am I doing something wrong or should I report a bug?

Copy link
Member

Choose a reason for hiding this comment

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

jest.Mock there comes from @types/jest not from the local jest import, so it's essentially what DefinitelyTyped/DefinitelyTyped#44365 is meant to solve

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah got it. Thanks!

@SimenB SimenB merged commit 911b74e into jestjs:master Oct 6, 2020
Vinnl added a commit to inrupt/solid-client-js that referenced this pull request Oct 6, 2020
Since 9cc1379, Jest supports
disabling the injection of globals. This makes tests easier to
follow, and ensures that all types can be properly checked as well.

(Unfortunately this also exposed a flaw in Jest's type definitions,
which I've fixed here but which isn't released yet:
jestjs/jest#10600)
@SimenB
Copy link
Member

SimenB commented Oct 6, 2020

I'll make a release with the fix as soon as both #10598 and #10599 lands, btw

@SimenB
Copy link
Member

SimenB commented Oct 6, 2020

Vinnl added a commit to inrupt/solid-client-js that referenced this pull request Oct 6, 2020
Use the new release with a fix for
jestjs/jest#10600.
Vinnl added a commit to inrupt/solid-client-js that referenced this pull request Oct 6, 2020
With the release of a new Jest version with a fix for
jestjs/jest#10600, a lot of type
assertions in tests are no longer necessary.
Vinnl added a commit to inrupt/solid-client-js that referenced this pull request Oct 6, 2020
Use the new release with a fix for
jestjs/jest#10600.
Vinnl added a commit to inrupt/solid-client-js that referenced this pull request Oct 6, 2020
Since 9cc1379, Jest supports
disabling the injection of globals. This makes tests easier to
follow, and ensures that all types can be properly checked as well.

(Unfortunately this also exposed a flaw in Jest's type definitions,
which I've fixed here but which isn't released yet:
jestjs/jest#10600)
@Vinnl
Copy link
Contributor Author

Vinnl commented Oct 6, 2020

Just tested the new release in my code, works perfectly. Thanks for the speedy turnover!

Vinnl added a commit to inrupt/solid-client-js that referenced this pull request Oct 6, 2020
Use the new release with a fix for
jestjs/jest#10600.
Vinnl added a commit to inrupt/solid-client-js that referenced this pull request Oct 6, 2020
Since 9cc1379, Jest supports
disabling the injection of globals. This makes tests easier to
follow, and ensures that all types can be properly checked as well.

(Unfortunately this also exposed a flaw in Jest's type definitions,
which I've fixed here but which isn't released yet:
jestjs/jest#10600)
@github-actions
Copy link

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 May 11, 2021
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.

None yet

3 participants