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

chore: followup to jest-mock TS migration #7850

Merged
merged 4 commits into from
Feb 10, 2019

Conversation

thymikee
Copy link
Collaborator

@thymikee thymikee commented Feb 10, 2019

Summary

Produces better public interface to the module.
See: #7847 (comment) and DefinitelyTyped/DefinitelyTyped#32882

Test plan

CI happy

@@ -973,7 +985,7 @@ class ModuleMockerClass {
return object[methodName];
}

_spyOnProperty<T extends {}, M extends keyof T>(
private _spyOnProperty<T extends {}, M extends keyof T>(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private _spyOnProperty<T extends {}, M extends keyof T>(
private _spyOnProperty<T extends {}, M extends NonFunctionPropertyNames<T>>(

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This causes TS to choke :( Wanna have a look locally?

Copy link
Member

Choose a reason for hiding this comment

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

@@ -899,7 +911,7 @@ class ModuleMockerClass {
return metadata;
}

isMockFunction(fn: any): boolean {
isMockFunction<T, Y extends unknown[]>(fn: any): fn is Mock<T, Y> {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this makes sense or not

Copy link
Collaborator Author

@thymikee thymikee Feb 10, 2019

Choose a reason for hiding this comment

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

Y is optional and unknown[] by default, so you can omit that. Docs say that type gruard performs runtime check, but I don't see a change in the build output. Anyway, it makes sense to me

Copy link
Member

Choose a reason for hiding this comment

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

the built output is the same, this is just us telling the type system that if the function returns true what the type is. Basically a typeof

object: T,
methodName: M,
): T[M] extends (...args: any[]) => any
? SpyInstance<ReturnType<T[M]>, ArgsType<T[M]>>
: never;

spyOn<T extends {}, M extends keyof T>(
spyOn<T extends {}, M extends NonFunctionPropertyNames<T>>(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

heh, not this is not entirely true because this generic spyOn can accept function and non-functional property names, but TS seems to buy it.

I don't really understand why a function even needs type definitions when it's overloaded like this. Generated types are only for the definitions as well, so that seems really counterintuitive, not to mention it causes headaches on "how to fit all the typings in this def and make TS happy, even though it may be a lie" 😅

Copy link
Member

Choose a reason for hiding this comment

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

heh, yeah. we might want to add actual tests on the types at some point (e.g. https://www.npmjs.com/package/tsd-check or https://www.npmjs.com/package/dts-jest)

@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 May 11, 2021
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.

3 participants