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(expect): improve typings of toThrow() and toThrowError() matchers #11929

Merged
merged 1 commit into from
Oct 5, 2021
Merged

fix(expect): improve typings of toThrow() and toThrowError() matchers #11929

merged 1 commit into from
Oct 5, 2021

Conversation

mrazauskas
Copy link
Contributor

@mrazauskas mrazauskas commented Oct 5, 2021

Resolves #10087

Summary

It looks like the typings issue described in #10087 is caused by definition of Constructable in expect package – https://github.com/facebook/jest/blob/f13abff8df9a0e1148baf3584bcde6d1b479edc7/packages/expect/src/types.ts#L93-L95

In @types/jest it is defined differently:

interface Constructable {
  new (...args: any[]): any;
}

Changing unknowns back into anys solves the issue.

It is possible to revert back or to move forward and to use ErrorConstructor instead of Constructable. This solves the problem as well and is more explicit, right?

Test plan

Added tests. As usual.

@mrazauskas mrazauskas changed the title fix(expect): improve typings of toThrow() and toThrowError() fix(expect): improve typings of toThrow() and toThrowError() matchers Oct 5, 2021
@codecov-commenter
Copy link

codecov-commenter commented Oct 5, 2021

Codecov Report

Merging #11929 (245dbbb) into main (f13abff) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main   #11929   +/-   ##
=======================================
  Coverage   68.75%   68.75%           
=======================================
  Files         322      322           
  Lines       16589    16589           
  Branches     4786     4786           
=======================================
  Hits        11405    11405           
  Misses       5152     5152           
  Partials       32       32           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f13abff...245dbbb. Read the comment docs.

@@ -306,11 +302,11 @@ export interface Matchers<R> {
/**
* Used to test that a function throws when it is called.
*/
toThrow(error?: string | Constructable | RegExp | Error): R;
toThrow(error?: string | RegExp | Error | ErrorConstructor): R;
Copy link
Member

Choose a reason for hiding this comment

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

this should probably be just be just error?: unknown for the same reason TS has made stuff in catch unknown by default in strict mode: https://devblogs.microsoft.com/typescript/announcing-typescript-4-4/#use-unknown-catch-variables

Copy link
Member

@SimenB SimenB Oct 5, 2021

Choose a reason for hiding this comment

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

@SimenB
Copy link
Member

SimenB commented Oct 5, 2021

I'm actually working on typing the entire expect package better right now. Good timing! 😀 (I haven't touched specific matchers, tho, so no overlap with this PR)

@SimenB
Copy link
Member

SimenB commented Oct 5, 2021

@mrazauskas
Copy link
Contributor Author

Sure. I will add a test too.

@SimenB
Copy link
Member

SimenB commented Oct 5, 2021

You run the type tests via yarn test-types in root, btw. In this case a test doesn't matter much, but we should expand our type tests, and this seems like a nice one to have as we build it up 🙂

@mrazauskas mrazauskas requested a review from SimenB October 5, 2021 16:29
Copy link
Member

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

Thanks!

@SimenB SimenB merged commit e420155 into jestjs:main Oct 5, 2021
@mrazauskas
Copy link
Contributor Author

By the way, if it is not overlapping, I could add type tests for other matchers too. That was fun.

@SimenB
Copy link
Member

SimenB commented Oct 5, 2021

That'd be awesome! ❤️

I don't have any plans to do more work for the types now (and if I do it'll be how to add custom matchers, not messing with the ones we have 🙂)

@mrazauskas mrazauskas deleted the fix-expect-types branch October 18, 2021 08:38
@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 Nov 18, 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.

Typing failure on expect().toThrow(Error) with explicit imports
4 participants