fix(@vitest/expect): support type-safe declaration of custom matchers#7656
fix(@vitest/expect): support type-safe declaration of custom matchers#7656sheremet-va merged 20 commits intovitest-dev:mainfrom
Conversation
✅ Deploy Preview for vitest-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
packages/expect/src/types.ts
Outdated
|
|
||
| export interface CustomMatchersObject extends Record<string, (...args: Array<any>) => any> {} | ||
|
|
||
| type CustomMatchersToRawMatchers<T extends CustomMatchersObject> = { |
There was a problem hiding this comment.
This mapping type is necessary because the userland should declare custom matchers like currently showcased in the docs:
interface CustomMatchers<R = unknown> {
toBeFoo: (input: number) => R
}The mapping is to correctly annotate the raw matchers that you declare in expect.extend()'s argument (correct this, correct received: unknown, and then inferred expected args).
Potential improvementsIdeally, I'd love for the // This would be perfect!
declare module 'vitest' {
interface Assertion<T = any> extends CustomMatchers<T> {}
interface CustomMatchersObject extends CustomMatchers {}
}However, given how the responsibility of these types differ, I'd say it does make sense to explicitly extend |
|
I would love to add a type test for this, too. Please advise me where would be the best place to put it! Thanks. |
@vitest/browser
@vitest/coverage-istanbul
@vitest/expect
@vitest/coverage-v8
@vitest/mocker
@vitest/pretty-format
@vitest/runner
@vitest/snapshot
@vitest/spy
@vitest/ui
@vitest/utils
vite-node
vitest
@vitest/web-worker
@vitest/ws-client
commit: |
|
I recognize the issue, but I disagree with the solution. My issues with it:
|
|
@sheremet-va, I'd love for it to infer the raw matcher functions from the The difficulty here is that |
As long as it's exporting them, it should work fine.
I cannot give any pointers, I'm sorry. Feel free to experiment. |
|
I gave this a try in the latest commit, but I cannot get the inference to work for some reason. From what I can see, the newly added Adding properties onto the SolutionTurns out, types have to be exported in order for this kind of augmentation to work. I've exported Augmentation now works: declare module 'vitest' {
interface Assertion<T = any> extends CustomMatchers<T> {}
interface MatchersDeclaration extends CustomMatchers {}
}We can certainly discuss the interface name. I find |
81cab12 to
a6e2edc
Compare
|
The next thing I'd love to do is collocate these two type augmentations into a single user action. You never want to have either of them, always both. So is should be a single augmentation you perform. Something like declare module 'vitest' {
interface Matchers extends CustomMatchers
}Why not utilize
|
CustomMatchersObject for type-safe expect.extend()|
I will put aside some time to finish this in the upcoming weeks. Thanks for understanding. |
UpdatePushed a few changes. The most important one being this: Now you can extend a single (new) interface CustomMatchers<R = unknown> {
toEqualSchema: (schema: Schema) => R
}
declare module 'vitest' {
interface Matchers extends CustomMatchers {}
}Such declaration will correctly annotate expect.extend({
toEqualSchema(received, expected) {
received // any
expected // Schema
}
})All the changes introduced are backward-compatible. You can still extend the Dropping the
|
|
I've also added some type tests for this. Would appreciate someone from the team to review this and, hopefully, move along with releasing it. Let's bring stellar type-safety to Vitest end-to-end! ✨ |
|
It looks like running |
|
I did not review this yet, but I wanted to say that I am very grateful for this PR! I will check it when I am done with high priority tasks. The type augmentation is something we needed to look into for some time now. |
|
@sheremet-va 🙏 I am glad to be able to improve Vitest, even if by a little. I would love to teach a better way to declare type-safe custom matchers, and this change is exactly what's missing. Would be grateful for your review whenever you have a moment. Thank you. |
|
@sheremet-va, linting fails on some arbitrary license check: I don't believe my changes have anything to do with this. I wonder how isn't this failing on main? I will rebase against main to see if that was already fixed there. |
It fails because you changed the license file when it shouldn't be changed |
|
@sheremet-va, that is extremely weird. I've never edited that file. I suggest you look into your automation, perhaps something auto-edits it in some cases. Will revert. |
|
Got a regression in typechecking: Looks like some of my latest changes broke the custom matcher forwarding if extending the Edit: The error was caused because I was extending |
You probably ran build command at one point without the latest dependencies. This file is generated automatically |
|
Yeah, I think you're right! Still my bad I let it slip through into the commit. I will be more careful with the future changes. Anyhow, I think the implementation is done on my part. I'm waiting for the tests to pass to confirm everything's okay. If you could please give this another review I would be grateful! |
packages/vitest/src/types/global.ts
Outdated
| // Allow unused `T` to preserve its name for extensions. | ||
| // Type parameter names must be identical when extending those types. | ||
| // eslint-disable-next-line | ||
| interface Matchers<T> {} |
There was a problem hiding this comment.
The changes in this file seem excessive, we already expose Matchers in @vitest/expect, we just need to also extend Assertion there. Is there a reason why you extend ExpectStatic, but don't extend Assertion in the expect package?
There was a problem hiding this comment.
See
vitest/packages/expect/src/types.ts
Line 631 in d5765f7
There was a problem hiding this comment.
Without this, it seems like you can't use Matchers type only with the @vitest/expect package - the vitest is required
There was a problem hiding this comment.
This is a great point. I put Matchers in vitest because I wasn't well familiar with the packages relationship back then. I followed your advice and moved the extension to @vitest/expect so that now these changes extend ExpectStatic and Assertion from @vitest/expect and do not touch the vitest changes at all. Type tests confirm that is enough. Awesome!
|
Thank you so much for reviewing this! Is there anything I can do to help with getting this released this week? |
You can help by writing a small documentation for this feature 😄 This PR will be part of 3.2, which is scheduled for May 19th. We can probably include this in the next beta release though. |
|
I propose we replace the current suggestions on extending the matchers with this. Would that work? If the docs are versioned, devs can reference the old solution (which still works) but new devs will interface with the new solution, which is shorter and fixes a few issues. Yeah, even having this in beta is quite enough to unblock me so that sounds good! |
Docs are versioned only by major. 3.1 won't have this feature, so we need to specify that it is only available since 3.2. |
Description
I am proposing this change to improve the type-safety experience when extending matchers.
Problem
Right now, the
MatchersObjectargument ofexpect.extend()is not open to extension. Although you can map your custom matchers by doing this......the argument you provide to
expect.extend()is still not type-safe:The issue is that extending the
Assertionsinterface is really meant only to have the custom matchers known on theexpect()return type when using it, but not when declaring those custom matchers inexpect.extend(). These two are controlled by different types.Proposed solution
Important
Please see the discussion below and the diff for the up-to-date information on how this feature actually got implemented.
First, here's the experience I'm after:
This already works locally with the proposed changes and finally completes the type-safety story of custom matchers in Vitest.
The following changes are needed to make this experience happen:
@vitest/expectintroduces a placeholder interfaceCustomMatchersObject. This is meant purely to be extended in the userland as you can see me doing above.MatchersObjecttype is preserved, but now it also remaps the custom matchers fromCustomMatchersObjecttoRawMatcherFn, inferring the arguments (expected values).RawMatcherFnhas to be extended to accept a new type argument to represent a narrower type for theexpectedarray of values.You can find the implementation for this in this pull request.
I will document the new functionality and update the Extending matchers recipe once these changes are approved. Thank you.
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
pnpm-lock.yamlunless you introduce a new test example.Tests
pnpm test:ci.Documentation
pnpm run docscommand.Changesets
feat:,fix:,perf:,docs:, orchore:.