Navigation Menu

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

Improve errors. Add actual and expected types to MismatchArgs #13

Closed
wants to merge 1 commit into from

Conversation

trevorade
Copy link
Collaborator

Updates MismatchArgs to, rather than create the tuple [never], creates a tuple with an object literal containing the Actual type and the expected type.

How the expected type is shown depends on the specific match. I will most likely post screenshots showing the errors for the various matchers in the PR conversation.

I left toBeCallableWith and toBeConstructibleWith alone as these do not use MismatchArgs and I couldn't work out how to use Variadic Tuple Types to make it work. These error messages are easier to understand anyways.

If microsoft/TypeScript#40468 is merged, we could drop MismatchArgs in favor of having a return that, rather than return true, uses a type throw if the type does not match.

…d type

Updates `MismatchArgs` to, rather than create the tuple `[never]`, creates a tuple with an object literal containing the `Actual` type and the expected type.

How the expected type is shown depends on the specific match. I will most likely post screenshots showing the errors for the various matchers in the PR conversation.

I left `toBeCallableWith` and `toBeConstructibleWith` alone as these do not use `MismatchArgs` and I couldn't work out how to use Variadic Tuple Types to make it work. These error messages are easier to understand anyways.
@trevorade
Copy link
Collaborator Author

trevorade commented Oct 20, 2022

When hovering over an error in VSCode, you get a legible message showing the actual and expected type:
1

This also works with .not:
2

More complicated types work with toEqualTypeOf as well:
3

As well as toMatchTypeOf:
4

All matcher functions which make use of MismatchArgs are supported:
5


Known issues

For toEqualTypeOf and toMatchTypeOf, if you pass a param rather than explicitly specifying the type, the overload at times prevents the useful error message from showing:
7a

The error message can still be seen in a flat manner if you try to enter a second param:
7b

Similarly, sometimes the passed-in param clobbers the mismatch args:
6a

Again, the error message can be seen if you try to enter a second param:
6b

So, as seen, this improves the situation for error messages in expect-type but does not solve it. Having throw be supported for types is really what this library needs.

@mmkal
Copy link
Owner

mmkal commented Oct 21, 2022

This is nice! DX looks nicer here. I haven't run this locally yet, and I have the same question that was raised on the type-fest PR: this doesn't help with CLI error messages, right? Not a blocker, it would still be an improvement.

Some other questions/comments:

Can we build something into MismatchArgs and its first typearg so that we don't have to repeat the "core" type? e.g.

toBeNumber: (...MISMATCH: MismatchArgs<Extends<Actual, number>, ExpectedResult, Actual, {extends: number}>)

Would be nicer something like this:

toBeNumber: (...MISMATCH: MismatchArgs<ShouldExtend<Actual, number>, ExpectedResult>)

Where ShouldExtend could be a wrapper for Extend which adds the extra info for these errors.

Another thing worth noting, esp if the CLI errors aren't improved - you can get intellisense to show you the types on hover, but by hovering on the method, not in the error message. I'll share a screenshot when I can.

Also, in the previous repo this library lived in, I had a PR for throw types, worth taking a look if you're interested: mmkal/ts#152. There are also some other template string type tricks in there that maybe we could borrow from.

@trevorade
Copy link
Collaborator Author

Can we build something into MismatchArgs and its first typearg so that we don't have to repeat the "core" type?

Here's what that implementation looks like (based on my version anyways):
image

(Note: MismatchArgsNew would be named MismatchArgs as expected. I only implemented the change for toBeAny as a proof of concept)

This pattern necessitates writing new types. That is, we can't really directly use IsAny anymore as that is used elsewhere and is exported, we need to add a new CheckIsAny that resolves to a CheckResult type.

This has the advantage of keeping interface ExpectTypeOf a bit cleaner at the cost of more indirection and more lines of checks (probably 6 or 7 CheckXyz types). CheckExtends will be re-used a fair amount so that should reduce code duplication a bit.

I'm happy to fully implement this update but I'd like to make sure we're satisfied with this implementation pattern before committing to it.

@mmkal mmkal mentioned this pull request Oct 23, 2022
2 tasks
@mmkal
Copy link
Owner

mmkal commented Oct 23, 2022

@trevorade thanks again for this. I do like the idea of another helper type here, especially because I think it'd be good to be able to play around a bit with the { actual: ...; expected: ... } structure.

The discussion on your type-fest PR got me thinking more generally about whether there wasn't some semi-low-hanging fruit where a bit more effort/complexity on the library side could improve the DX fairly significantly. And I also wanted to capture exactly what error messages looked like so it wasn't as necessary to share screenshots about what they looked like. And the existing tests only have valid code - so I added a file errors.test.ts which uses the typescript compiler API and does a jest snapshot of what the errors for some invalid code look like. If you could merge from main, I'd be interested to see if this branch improves them at all, or if it would when combined with #16 - also, it would be great to get your feedback on that if you have any. Sorry to say that it looks like there are some conflicts now - let me know if you need any help resolving them.

@mmkal
Copy link
Owner

mmkal commented Oct 23, 2022

Oh also I invited you to be a collaborator on this repo, so if you'd like to create a branch here instead of on your fork, feel free.

@trevorade
Copy link
Collaborator Author

Thx for the collaborator invite! I appreciate it.

I like the idea of capturing error messages in errors.test.ts! I'll utilize that for this PR where it makes sense.

I'll get a chance to work on this more tomorrow.

mmkal added a commit that referenced this pull request Oct 3, 2023
Fixes #17
Closes #36 (by documenting that `toMatchTypeOf` ~= `toExtend`)
Closes #37 (by documenting that helper types are not part of the API
surface)
Closes #32
Closes #4
Closes #6 (by documenting that you _can't_ use
`.not.toBeCallableWith(...)`)
Closes #38

Use generics to give better error messages than "Arguments for the rest
parameter 'MISMATCH' were not provided" for most `toEqualTypeOf` cases
and many `toMatchTypeOf` cases. This trades off some implementation
complexity for better end-user error messages. Essentially, write a
special `<Expected extends ...>` constraint for each overload of each
method, which does some crazy conditional logic, which boil down to:

- `<Expected extends Actual>` for `toEqualTypeOf` (when we aren't in a
`.not` context)
- `<Expected extends Satisfies<Actual>>` for `toMatchTypeOf`

Anyone interested, have a look at the snapshot updates in
`errors.test.ts.snap` to see what the real world difference is.

Each of these constraints apply only when we know it's going to "fail" -
i.e. `Satisfies<...>` is a fairly meaningless helper type that is used
to try to show errors at the type-constraint level rather than the
`...MISMATCH: MismatchArgs<...>` level which won't give good error
messages.

When using `.not`, the constraint just becomes `extends unknown`, and
you'll have to squint as before.

See also: #14 for the better long-term solution, _if_ the TypeScript
team decide to merge the throw types PR.
See also: #13 for a hopefully-complementary improvement to the
information on hover, which will improve the cases this doesn't cover.

TODO:

- [x] See if the `expectTypeOf({a: 1}).toMatchTypeOf({a: 'one'})` case
can also be improved.
- [x] Document. The constraints are a bit different to what most users
would be used to, so it's worth highlighting the best way to read error
messages and clarify when they might default to "Arguments for the rest
parameter 'MISMATCH' were not provided"

Note: I have publish v0.17.0-1 based on this PR and will hopefully be
able to use [that version in
vitest](vitest-dev/vitest#4206) as a test before
merging.
@mmkal
Copy link
Owner

mmkal commented Oct 3, 2023

@trevorade now that #16 is merged I think this is out of date. But if there's an improvement on top of #16 you'd like to make, let me know. It should probably be in a new PR though since this is conflicted now, so I'll close for now.

Note - I will aim to cut a 1.0.0 release this week if all goes well, so ideally any changes that don't happen before then are non-breaking so I don't have to go to 2.0.0 embarrassingly quickly!

@mmkal mmkal closed this Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants