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

fix(jest-mock): improve spyOn typings to handle optional properties #13247

Merged
merged 5 commits into from Sep 11, 2022

Conversation

mrazauskas
Copy link
Contributor

Summary

Apparently spyOn overloads do not work properly if an interfaces with optional props is encountered:

interface OptionalInterface {
  methodA?: ((a: boolean) => void) | undefined;
  methodB: (b: string) => boolean;
}

const optionalSpiedObject = {} as OptionalInterface;

spyOn(optionalSpiedObject, 'methodA'); // "No overload matches this call. Etc..."

So this got fixed.

Test plan

Type tests are added.

spyOn<
T extends object,
K extends ConstructorLikeKeys<Required<T>>,
V extends Required<T>[K],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

V is just a helper to avoid SpyInstance<(...args: ConstructorParameters<Required<T>[K]>) => InstanceType<Required<T>[K]>>

object: T,
methodName: M,
methodKey: K,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

methodKey and K are just renames for consistency. MethodLikeKeys, PropertyLikeKeys, methodKey, propertyKey, K – always keys. Easier to understand what is happening.

spyOn<
T extends object,
K extends MethodLikeKeys<Required<T>>,
V extends Required<T>[K],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lines with Required do all the magic, because they turn types like (() => void)) | undefined into () => void). Something what wasn’t a function, becomes one. This part was not working, otherwise logic is the same.

obj: T,
propertyName: M,
accessType: 'get' | 'set' = 'get',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mrazauskas mrazauskas marked this pull request as draft September 11, 2022 12:00
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.

Nice!

@SimenB SimenB merged commit d0ed4e6 into jestjs:main Sep 11, 2022
@mrazauskas mrazauskas deleted the fix-spyOn-optional-props branch September 11, 2022 14:47
@mrazauskas
Copy link
Contributor Author

Interestingly same things here DefinitelyTyped/DefinitelyTyped#62208 fails to compile on TS 4.4. But in Jest repo all is fine on TS 4.3. I always have a feeling that to go through CI on DT is simply luck ;D

@SimenB
Copy link
Member

SimenB commented Sep 28, 2022

@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 Oct 30, 2022
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

3 participants