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

.toMatchTypeOf() does not take into account readonly properties (undocumented case?) #55

Open
mrazauskas opened this issue Mar 11, 2024 · 11 comments · May be fixed by #126
Open

.toMatchTypeOf() does not take into account readonly properties (undocumented case?) #55

mrazauskas opened this issue Mar 11, 2024 · 11 comments · May be fixed by #126
Labels
documentation Improvements or additions to documentation

Comments

@mrazauskas
Copy link
Contributor

From #52 (comment)

The documentation suggests that .toMatchTypeOf() can be used to "allow for extra properties". Just wanted to draw attention that it might need better description and/or examples.

Currently at least for me it sounds like .toMatchTypeOf() and .toEqualTypeOf() are identical, but .toMatchTypeOf() allows testing partial match. That seems to be incorrect, when readonly gets involved:

import { expectTypeOf } from 'expect-type';

expectTypeOf<{ readonly a: string; b: number }>().not.toEqualTypeOf<{ a: string; b: number }>();

expectTypeOf<{ readonly a: string; b: number }>().not.toMatchTypeOf<{ a: string }>(); // should also pass, but it fails

playground

@aryaemami59 aryaemami59 added the documentation Improvements or additions to documentation label Mar 19, 2024
@mmkal
Copy link
Owner

mmkal commented Mar 21, 2024

Discussed this elsewhere, but expectTypeOf<A>().toMatchTypeOf<B>() is pretty much the A extends B check, with a tiny amount of sanity-checking to protect against root-level never (and possibly any in future: #66).

There's an outstanding, old feature request to rename toMatchTypeOf to align more with typescript (i.e. something like toExtend maybe) and less with jest/vitest: #10.

But as mentioned there, since this library is already in use in a bunch of places, I want to get a v1 out before making that kind of change, or even introducing confusion with an alias.

@mrazauskas it's very hard for me to remember what it's like not knowing this stuff, as I've now known it for years - it would be greatly appreciated if you could open a PR against the readme communicating any info that would have been helpful to you when you learned this the hard way recently.

@mrazauskas
Copy link
Contributor Author

mrazauskas commented Oct 7, 2024

Looks like a better idea could be to advise using Pick and Omit instead of .toMatchTypeOf(). These utility types are familiar and work without quirks:

import { expectTypeOf } from 'expect-type';

expectTypeOf<{ readonly a: string; b: number }>().not.toEqualTypeOf<{ a: string; b: number }>();
expectTypeOf<Pick<{ readonly a: string; b: number }, "a">>().not.toEqualTypeOf<{ a: string }>();

(EDIT That’s just a note for future reader. Not an issue for me. Clicked wrong button.)

@mrazauskas mrazauskas reopened this Oct 7, 2024
@mrazauskas
Copy link
Contributor Author

By the way, it might be worth documenting this quirk of .toMatchTypeOf() as well:

expectTypeOf<{ a: string; b: number }>().toMatchTypeOf<{ a: string | number | undefined }>();

Current documentation states: "To allow for extra properties, use .toMatchTypeOf". In reality this is more than just "allow for extra properties". Or I do I miss something? (;

@mmkal
Copy link
Owner

mmkal commented Oct 7, 2024

Yeah, that's probably worth documenting better. Roughly, toMatchTypeOf is checking whether X extends Y (with some never edge-case checks).

{a: string; b: number} does extend {a: string | number | undefined} - so I think this is correct behaviour. I go back and forth on whether toMatchTypeOf is a good name. On the one hand, "match" is not a concept in typescript, unlike "extends". But on the other, the helper isn't exactly the same as an extends check. Brief docs here though: https://github.com/mmkal/expect-type?tab=readme-ov-file#where-is-toextend

@mrazauskas
Copy link
Contributor Author

Yeah, that's probably worth documenting better. Roughly, toMatchTypeOf is checking whether X extends Y (with some never edge-case checks).

What is X and what is Y here? And which are the edge cases?

All what I try to say: it is really hard to document the behaviour of .toMatchTypeOf. And is not possible to alter or remove anything from the API of this library, because users might have used // @ts-expect-error here or there and their tests have a right to pass. Oh well.. (;

@mmkal
Copy link
Owner

mmkal commented Oct 9, 2024

What is X and what is Y here?

expectTypeOf<X>().toMatchTypeOf<Y>()

And which are the edge cases?

If we wrote a toExtend<...>() helper instead which just relied on the extends operator, it would let never pass when it (probably) shouldn't:

expectTypeOf<never>().toExtend<string>() // ok
expectTypeOf<never>().toExtend<number>() // ok
expectTypeOf<never>().toExtend<{abc: boolean}>() // ok

Because never extends everything. The current toMatchTypeOf will fail the above tests, which is I think what most people would expect. But never can hide problems, similarly to any. You can cast an input to never and pass it into any function, and you can do things like this const x: string = 1 as never and the compiler won't complain.


Having said all that, I take your point that it's somewhat confusing. I could potentially be open to a future version of this library which adds toExtend, which replaces the current toMatchTypeOf. And there could maybe be a new utility function that only works with objects, and which effectively does the Pick<...> thing you've done above.

Re backwards compatibility, I agree, we've just released v1 but it was a deliberate choice to not include breaking changes. A lot of people were using v0 of this library already, so v1 was really just a "courtesy". Breaking changes could still arrive in v2.

I'm going to reopen this and see if I can come up with a best-of-both-worlds solution.

@mmkal mmkal reopened this Oct 9, 2024
@mrazauskas
Copy link
Contributor Author

Regarding breaking changes. I was trying to point out that if someone used // @ts-expect-error instead of .not (since documentation isn’t again that), the assertion will be passing even if some matcher is changing behaviour, is renamed, or is removed entirely.

@mmkal
Copy link
Owner

mmkal commented Oct 9, 2024

Opened #126, you can try it out with pnpm add https://pkg.pr.new/mmkal/expect-type@126

Usage:

type MyType = {readonly a: string; b: number; c: {some: {very: {complex: 'type'}}}; d: 'another type'}

expectTypeOf<MyType>().toMatchObjectType<{a: string; b: number}>() // fails - forgot readonly
expectTypeOf<MyType>().toMatchObjectType<{readonly a: string; b?: number}>() // passes

@mrazauskas fair point re // @ts-expect-error. Most users probably never actually "run" the assertions (they're a no-op at runtime). For those that do, removing the runtime method would at least help. However having just released v1 after years of v0, if we go ahead with this we'll just deprecate toMatchTypeOf and carry it around as-is for a long time - it should not be much maintenance overhead since it's the same as the new toExtend.

@mrazauskas
Copy link
Contributor Author

By the way, does pkg.pr.new with TS Playground? That would be really nice.

@mmkal
Copy link
Owner

mmkal commented Oct 9, 2024

I don't think so. There is the "Open in Stackblitz" link which lets you play around though, but you need to add a tsconfig and .ts file manually for a library like this (CC @Aslemammad would be nice if the template included typescript, a basic tsconfig with strict/skipLibCheck, and an index.ts file? Lots of libraries like expect-type, arktype, trpc, zod etc. would benefit from that)

@Aslemammad
Copy link

Yes, it'd be nice, but that's a default option we thought of removing before so it'd just motivate users to have their own templates.

The reason we have it is pure motivation 😅 so I recommend shipping your own template with that config.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
4 participants