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

feat(@jest/expect): Expose type of actual to Matchers #13848

Merged
merged 8 commits into from Feb 2, 2023

Conversation

benjaminjkraft
Copy link
Contributor

Summary

Matchers isn't as typed as some users would like (see #13334, #13812). For users who want to customize it by extending the Matchers interface, it's useful to have access to the type of actual (the argument of expect) so you can do, say,

interface Matchers<R, T> {
    toTypedEqual(expected: T): R
}

This commit exposes it. The first-party matchers still have the same types as before.

Test Plan

typecheck

Matchers isn't as typed as some users would like (see jestjs#13334, jestjs#13812).
For users who want to customize it by extending the `Matchers`
interface, it's useful to have access to the type of `actual` (the
argument of `expect`) so you can do, say,
```ts
interface Matchers<R, T> {
    toTypedEqual(expected: T): R
}
```
This commit exposes it. The first-party matchers still have the same
types as before.
Copy link
Contributor

@mrazauskas mrazauskas left a comment

Choose a reason for hiding this comment

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

Thanks! I think this is useful for custom matchers in general. For example, the inferred type of actual is used in snapshot matchers through @jest/expect. I think user of expect should be allowed to have similar functionality for a custom matcher.

Also because this was allowed before #12404 and still works with @types/jest. That's why I see this PR as a fix of a regression.


Could you add:

type N = Matchers<void, string>;

Under this line:

https://github.com/facebook/jest/blob/836157f4807893bb23a4758a60998fbd61cb184c/packages/expect/__typetests__/expect.test.ts#L21

Also please add a type test with your example of toTypedEqual here (simply to prevent regression):

https://github.com/facebook/jest/blob/836157f4807893bb23a4758a60998fbd61cb184c/packages/expect/__typetests__/expect.test.ts#L108-L109

CHANGELOG.md Outdated
@@ -2,6 +2,8 @@

### Features

- `[@jest/expect]` provide type of `actual` as a generic argument to `Matchers` to allow better-typed extensions (#TBD)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can go under fixes as well.

Suggested change
- `[@jest/expect]` provide type of `actual` as a generic argument to `Matchers` to allow better-typed extensions (#TBD)
- `[expect, @jest/expect]` Provide type of `actual` as a generic argument to `Matchers` to allow better-typed extensions ([#13848](https://github.com/facebook/jest/pull/13848))

@benjaminjkraft
Copy link
Contributor Author

Thanks for all the helpful comments! I think I've addressed all of them. A couple notes:

  • I can't rename the variable to _T because if there are multiple declarations of an interface the type parameters get matched by name (so users would have to do Matchers<R, _T>). Sadly that means we need a ts-ignore as well as an eslint-ingore :( . Other ideas welcome.
  • I put the expect type tests in a different file to show that users can declare either Matchers<R> or Matchers<R, T> (but typescript requires they are consistent within a file), and also added some to jest-expect since it has its own copy of the wiring

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Tom Mrazauskas <tom@mrazauskas.de>
CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Tom Mrazauskas <tom@mrazauskas.de>
Comment on lines 137 to 139
// @ts-expect-error unused variable T (can't use _T since users redeclare Matchers)
// eslint-disable-next-line @typescript-eslint/no-unused-vars
export interface Matchers<R extends void | Promise<void>, T = unknown> {
Copy link
Contributor

Choose a reason for hiding this comment

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

What about this:

Suggested change
// @ts-expect-error unused variable T (can't use _T since users redeclare Matchers)
// eslint-disable-next-line @typescript-eslint/no-unused-vars
export interface Matchers<R extends void | Promise<void>, T = unknown> {
export interface Matchers<R extends void | Promise<void>, T = unknown> {
/** @internal */
_doKeepT(expect: T): R;

Copy link
Contributor

@mrazauskas mrazauskas Feb 1, 2023

Choose a reason for hiding this comment

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

Could have some longer comment, but seems to be working. Or?

stripInternal documentation: https://www.typescriptlang.org/tsconfig#stripInternal

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, great! I thought of adding a fake method but didn't realize I could hide it like that.

Copy link
Contributor

@mrazauskas mrazauskas left a comment

Choose a reason for hiding this comment

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

Two last details from my side. The rest looks good.

Type tests are passing, because emitted types keep T as expected:

Screenshot 2023-02-01 at 21 43 11

It is marked as unused in this screenshot of .d.ts file. So I went checking if that is not a problem for a user having "skipLibCheck": true in a tsconfig.json. TypeScript did not complain. Seems to be fine.

packages/expect/src/types.ts Outdated Show resolved Hide resolved
packages/expect/src/types.ts Outdated Show resolved Hide resolved
Co-authored-by: Tom Mrazauskas <tom@mrazauskas.de>
@benjaminjkraft
Copy link
Contributor Author

It is marked as unused in this screenshot of .d.ts file. So I went checking if that is not a problem for a user having "skipLibCheck": true in a tsconfig.json. TypeScript did not complain. Seems to be fine.

Thanks for checking, I looked at the .d.ts but didn't think to check if it could cause upstream errors. Just to confirm, this works even if they also have noUnusedParameters enabled?

Co-authored-by: Tom Mrazauskas <tom@mrazauskas.de>
@mrazauskas
Copy link
Contributor

Just to confirm, this works even if they also have noUnusedParameters enabled?

Yes, noUnusedParameters was enabled and in general it was rather strict TS configuration.

@SimenB
Copy link
Member

SimenB commented Feb 2, 2023

skipLibCheck: false I assume?

@mrazauskas
Copy link
Contributor

Right, false it was. I had in mind that it was enabled, so fingers typed true (;

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 427fe2b into jestjs:main Feb 2, 2023
@benjaminjkraft benjaminjkraft deleted the expect-types branch February 4, 2023 01:52
@SimenB
Copy link
Member

SimenB commented Feb 7, 2023

https://github.com/facebook/jest/releases/tag/v29.4.2

@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 Mar 10, 2023
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

4 participants