-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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
jest: Update for v27 #55013
jest: Update for v27 #55013
Conversation
@domdomegg Thank you for submitting this PR! I see this is your first time submitting to DefinitelyTyped 👋 — I'm the local bot who will help you through the process of getting things through. This is a live comment which I will keep updated. 1 package in this PR
@domdomegg: I see that you have added yourself as an owner, are you sure you want to become an owner? 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
All of the items on the list are green. To merge, you need to post a comment including the string "Ready to merge" to bring in your changes. Diagnostic Information: What the bot saw about this PR{
"type": "info",
"now": "-",
"pr_number": 55013,
"author": "domdomegg",
"headCommitOid": "63b487b9c57fd9c1987bacd04c82105422600444",
"lastPushDate": "2021-08-09T21:41:27.000Z",
"lastActivityDate": "2021-08-10T18:17:11.000Z",
"mergeOfferDate": "2021-08-10T09:02:15.000Z",
"mergeRequestDate": "2021-08-10T18:17:11.000Z",
"mergeRequestUser": "domdomegg",
"hasMergeConflict": false,
"isFirstContribution": true,
"tooManyFiles": false,
"popularityLevel": "Critical",
"pkgInfo": [
{
"name": "jest",
"kind": "edit",
"files": [
{
"path": "types/jest/index.d.ts",
"kind": "definition"
},
{
"path": "types/jest/jest-tests.ts",
"kind": "test"
}
],
"owners": [
"NoHomey",
"jwbay",
"asvetliakov",
"alexjoverm",
"epicallan",
"ikatyang",
"wsmd",
"JamieMason",
"douglasduteil",
"ahnpnl",
"joshuakgoldberg",
"UselessPickles",
"r3nya",
"hotell",
"sebald",
"andys8",
"antoinebrault",
"gstamac",
"ExE-Boss",
"quassnoi",
"Belco90",
"tonyhallett",
"ycmjason",
"devanshj",
"pawfa",
"regevbr",
"gerkindev"
],
"addedOwners": [
"domdomegg"
],
"deletedOwners": [],
"popularityLevel": "Critical"
}
],
"reviews": [
{
"type": "approved",
"reviewer": "orta",
"date": "2021-08-10T09:01:37.000Z",
"isMaintainer": true
},
{
"type": "approved",
"reviewer": "G-Rath",
"date": "2021-08-09T21:50:19.000Z",
"isMaintainer": false
}
],
"mainBotCommentID": 894853661,
"ciResult": "pass"
} |
🔔 @NoHomey @jwbay @asvetliakov @alexjoverm @epicallan @ikatyang @wsmd @JamieMason @douglasduteil @ahnpnl @JoshuaKGoldberg @UselessPickles @r3nya @Hotell @sebald @andys8 @antoinebrault @gstamac @ExE-Boss @quassnoi @Belco90 @tonyhallett @ycmjason @devanshj @pawfa @regevbr @GerkinDev — please review this PR in the next few days. Be sure to explicitly select |
Yes, I am happy to be an owner (also fine not being one, if others object 🤷). |
@@ -340,7 +331,7 @@ declare namespace jest { | |||
fail(error?: string | { message: string }): any; | |||
} | |||
|
|||
type ProvidesCallback = (cb: DoneCallback) => any; | |||
type ProvidesCallback = ((cb: DoneCallback) => void | undefined) | (() => Promise<unknown>); |
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.
While this is correct for test suite & case functions, it's technically not for hooks because you can return any value so long as you don't have the done
callback, as it's meant to make it easier compatibility with short-handed arrow functions; currently this type will require folks to return a promise even if they're not doing anything async.
I think we should make a new HookCallback
to cover this (I'm impartial to if we create a new Lifecycle
-like type or make Lifecycle
take a generic of ProvidesCallback | HookCallback
)
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.
Thanks for the review! 🙌
currently this type will require folks to return a promise even if they're not doing anything async
The ProvidesCallback
type should allow them to return void or undefined or a promise if they don't use done (the tests verify this I believe). If they use done, they must return void or undefined. We could make this more explicit perhaps by changing this line to:
type ProvidesCallback = ((cb: DoneCallback) => void | undefined) | (() => Promise<unknown> | void | undefined);
make it easier compatibility with short-handed arrow functions
I'm not sure of exactly what you mean here. This change will disallow code like:
const getMagicNumber = () => 1234;
jest.beforeAll(() => getMagicNumber());
because we would then be returning a number. This is not void, undefined or Promise.
This is intentional for Jest 27 to match @SimenB's change: "Disallow return values other than a Promise
from hooks and tests"
If you think we should allow hooks to return any
we could easily implement that with something like:
type ProvidesHookCallback = (cb: DoneCallback) => any;
// or just so it's clear that it's a subtype (but wouldn't change the actual typing)
type ProvidesHookCallback = (cb: DoneCallback) => any | ProvidesCallback;
type Lifecycle = (fn: ProvidesHookCallback, timeout?: number) => any;
...but I believe this will create a discrepancy between DefinitelyTyped and Jest which is undesirable. Happy to take the consensus of active reviewers though.
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.
What I'm meaning is that it's still valid to return anything from a hook so long as you don't have done
- I've confirmed this with this on latest jest:
describe('a test', () => {
beforeEach(() => 1);
it('passes', () => {
expect(true).toBe(true);
});
});
We're even doing it in eslint-plugin-jest
:
// no-deprecated-functions.ts
/** @internal */
export const _clearCachedJestVersion = () => (cachedJestVersion = null);
// __tests_/no-deprecated-functions.ts
describe('the rule', () => {
beforeEach(() => _clearCachedJestVersion());
...
});
The problem I'm foreseeing is that people will be using this pattern without issue on Jest 27 right now, only for them to update their types and suddenly get it flagged as an error despite it working at runtime - and unless they scour the changelog and find the entry mentioning this breaking type-only change, I expect they'll open issues and PRs here saying the types are wrong.
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.
Sure, happy to - I only made this change to match the changelog and internal jest types.
As long as nobody else objects I'll change this, as per the end of my previous comment (adding ProvidesHookCallback
).
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.
as per the end of my previous comment (adding ProvidesHookCallback).
I believe the change you described there won't be correct: Lifecycle
is currently used to type both hooks (which do allow returning anything unless you're using done
) & tests (which don't allow returning anything ever).
The current change you've made looks correct for tests, so I think we just need to make a new type (or really duplicate the types as they were before your change) for hooks - which is why I suggested maybe using a generic might be tidier since it would save having two Lifecycle
types.
Nope I'm wrong - it's ProvidesCallback
that is being used for both hooks and tests currently 😅
In saying that, the ProvidesHookCallback
should not allow returning if the callback function is present, as that will cause an error in v27.
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.
What about when using jest-jasmine2
as the test runner? In such cases, returning a Promise
together with the done
callback is completely ok and safe. I had to revert the version back exactly because of this.
We should rely on the runner reporting runtime errors instead of the type system, especially in cases where the runtime is configurable (like for jest). When using jest-circus
, the combination of a done callback and a returned promise will raise a descriptive error:
Test functions cannot both take a 'done' callback and return something. Either use a 'done' callback, or return a promise.
type ProvidesCallback = ((cb: DoneCallback) => void | undefined) | (() => Promise<unknown>); | |
type ProvidesCallback = (cb: DoneCallback) => Promise<void | undefined> | void | undefined; |
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.
Hi @enisdenjo, can you give an example of a test in jest-jasmine2
that requires both a done callback and returning a promise?
As per the Jest changelog and blog, breaking changes in Jest 27 include making TypeScript types stricter to prevent returning a Promise and taking a done callback - no matter the runtime environment.
It might be worth raising an issue on Jest directly rather than the types if you disagree with the change to disallow using both a done callback and returning a promise. If you think here in the types we've misinterpreted what Jest has changed, then happy to discuss - but otherwise we're debating something that is out of scope for 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.
Hey @domdomegg,
can you give an example of a test in jest-jasmine2 that requires both a done callback and returning a promise?
I use done+promise all around, here are a few examples:
- https://github.com/enisdenjo/graphql-ws/blob/06e3326e0467c91535a3d0697f249c1b7db8bfbd/src/__tests__/client.ts#L171-L192
- https://github.com/enisdenjo/graphql-ws/blob/06e3326e0467c91535a3d0697f249c1b7db8bfbd/src/__tests__/client.ts#L380-L402
- https://github.com/enisdenjo/graphql-ws/blob/06e3326e0467c91535a3d0697f249c1b7db8bfbd/src/__tests__/server.ts#L26-L59
- https://github.com/enisdenjo/graphql-ws/blob/06e3326e0467c91535a3d0697f249c1b7db8bfbd/src/__tests__/server.ts#L509-L532
Do note that all above examples run on Jest v27 with the test runner set to jest-jasmine2
.
This PR makes all of them fail with TS, even though its perfectly type legal and safe to do so with jest-jasmine2
.
As per the Jest changelog and blog, breaking changes in Jest 27 include making TypeScript types stricter to prevent returning a Promise and taking a done callback - no matter the runtime environment.
I think you're wrong here, I guess you're talking about this entry in the changelog:
[jest-circus] [BREAKING] Fail tests when multipledone()
calls are made (jestjs/jest#10624)
The change is exclusive to jest-circus
, not whole jest
(as stated in the scope on the beginning of the changelog entry). jest-circus
is a test runner and jestjs/jest#10624 fails tests during runtime.
See #55013 (comment).
It might be worth raising an issue on Jest directly rather than the types if you disagree with the change to disallow using both a done callback and returning a promise.
Personally, I am against this change, it just complicates stuff unnecessarily. You want async tests to leverage the await
and still use a done
in a callback to indicate completion where tests have mixes of promises and events (as my examples show above).
Furthermore, there is already an issue raised for jest-circus
: jestjs/jest#10529. Yeah, sure, you can just do jestjs/jest#10529 (comment); but, test suites are meant to be convenient and easy to use.
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.
My bad, I referenced the wrong changelog entry before. You were probably talking about:
[jest-globals] [BREAKING] Disallow mixing a
done
callback and returning aPromise
from hooks and tests (jestjs/jest#10512)
which does indeed belong to 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.
@domdomegg even though the v27 changelog claims done+promise is disallowed globally, it seems not to be. The done+promise error is only present when using the jest-circus
test runner and the assertion can be found here (inside jest-circus
) only:
Ready to merge |
I just published |
I believe this change should have also updated the dependencies on Jest's type-exposing packages to v27: DefinitelyTyped/types/jest/package.json Lines 4 to 5 in 32ac206
|
Please fill in this template.
npm test <package to test>
.If changing an existing definition: