fix: update spy type inference #643
Conversation
|
✔️ Deploy Preview for vitest-dev ready! 🔨 Explore the source changes: a94601e 🔍 Inspect the deploy log: https://app.netlify.com/sites/vitest-dev/deploys/61f901e67b2bab00070e2c46 😎 Browse the preview: https://deploy-preview-643--vitest-dev.netlify.app |
| @@ -120,7 +120,7 @@ export function spyOn<T, G extends Properties<Required<T>>>( | |||
| export function spyOn<T, M extends Classes<Required<T>>>( | |||
| object: T, | |||
| method: M | |||
| ): Required<T>[M] extends new (...args: infer A) => infer R | |||
| ): Required<T>[M] extends (...args: infer A) => infer R | |||
There was a problem hiding this comment.
Wouldn't this change make this type the same as Methods case on line 126?
From the type info it looks like you can spy on instance methods by spying on the constructor. I'm not sure if this is the case with tinyspy.
There was a problem hiding this comment.
Yes, it will. Maybe we need to change the order or something
There was a problem hiding this comment.
For context: we didn't have this overload, but added it in some issue to mock classes. This aligns with jest and works well, since classes are functions.
There was a problem hiding this comment.
Maybe we need to change the order or something
Probably need to use conditional types to merge those two cases into one.
There was a problem hiding this comment.
I think Classes returns incorrect types here, since the idea here is to limit overload. never should never happen here in theory, so conditional types wouldn't be needed.
There was a problem hiding this comment.
I missed that part, my bad.
I'm not too familiar with TypeScript's conditional types.
Is it possible to check if it extends a union type? Or maybe something along the lines of:
(Required<T>[M] extends (...args: infer A) => infer R) || (Required<T>[M] extends new (...args: infer A) => infer R) ? SpyInstance<A, R> : neverThere was a problem hiding this comment.
I missed that part, my bad.
I'm not too familiar with TypeScript's conditional types. Is it possible to check if it extends a union type?
You need to chain them like so:
A extends B
? B extends C
? true
: false
: neverIt's really ugly. But as I said it shouldn't be needed, since M extends Classes<Required<T>>, so Required<T>[M] should only contain constructors. extends (...args: infer A) => infer R is just a formality to get types of arguments and return type.
There was a problem hiding this comment.
Classes type is strange here. It is a dictionary of constructors for some reason. Am I missing something?
There was a problem hiding this comment.
I pushed a commit that should just merge the two overloads in a backward-compatible way.
DerYeger
force-pushed
the
bug/673_spy_type_inference
branch
from
21ec4c2
to
bbb4dcb
Compare
|
Rebasing broke the spy test. Did something change since my initial branching? |
Yes, there was a fix for #645 in tinyspy. Will check this case in tinyspy. |
|
Maybe this is another bug. I know we have a similar issue with timers. Can you try assigning |
|
It worked before the rebase. Are you sure that they are not the same using jsdom? I'm pretty sure that they are the same object, as |
Just checked it in tinyspy repo. It works with happy-dom but not with jsdom. And it does not work even when using it without |
I'm glad that the test was useful for something else than the typecheck after all |
|
@DerYeger tinylibs/tinyspy#18 (comment) - check my comment on tinyspy. We cannot mock localStorage like this. |
Hmm, weird that it worked before |
|
tinyspy was mocking prototype but this caused other issues. As @sheremet-va stated tinylibs/tinyspy#18 (comment) you can spy on the prototype explicitly. |
|
@DerYeger Can you disable that failing custom reporters test? I will look into how to write it properly later |
Done |
Closes #637.
I'm pretty sure this should solve the error.If I understand the current typing correctly, a method was required to be a constructor.
This should remove that constraint.
If we do want to spy on constructors I can add another overload declaration.