-
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
Merged
typescript-bot
merged 2 commits into
DefinitelyTyped:master
from
domdomegg:domdomegg/jest-27
Aug 10, 2021
Merged
jest: Update for v27 #55013
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 newLifecycle
-like type or makeLifecycle
take a generic ofProvidesCallback | 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! 🙌
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:I'm not sure of exactly what you mean here. This change will disallow code like:
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:...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:We're even doing it in
eslint-plugin-jest
: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.
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 usingdone
) & 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 twoLifecycle
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 aPromise
together with thedone
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: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,
I use done+promise all around, here are a few examples:
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
.I think you're wrong here, I guess you're talking about this entry in the changelog:The change is exclusive tojest-circus
, not wholejest
(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).
Personally, I am against this change, it just complicates stuff unnecessarily. You want async tests to leverage the
await
and still use adone
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:
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 (insidejest-circus
) only:https://github.com/facebook/jest/blob/7c6cea2696e6f08553189a5e077126d610751576/packages/jest-circus/src/utils.ts#L212-L220