[jest] fix jest.fn to match spec

[fgrandel]

Please fill in this template.

• Use a meaningful title for the pull request. Include the name of the package modified.
• Test the change in your own code. (Compile and run.)
• Add or edit tests to reflect the change. (Run with npm test.)
• Follow the advice from the readme.
• Avoid common mistakes.
• Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If changing an existing definition:

• Provide a URL to documentation or source code which provides context for the suggested changes: https://jestjs.io/docs/en/jest-object#jestfnimplementation => "optionally takes a mock implementation"
• Increase the version number in the header if appropriate.
• If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }.

fixes #32863

[typescript-bot]

@Jerico-Dev Thank you for submitting this PR!

🔔 @NoHomey @jwbay @asvetliakov @alexjoverm @epicallan @ikatyang @wsmd @JamieMason @douglasduteil @ahnpnl @JoshuaKGoldberg @UselessPickles @r3nya @Hotell @sebald @andys8 @geovanisouza92 - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

[antoinebrault]

You can't change generics without a breaking change. It has to stay the same.

Before I introduced type inference in jest, it was:

function fn<T>(implementation?: (...args: any[]) => any): Mock<T>;

I added a second generic for arguments

function fn<T, Y extends any[]>(implementation: (...args: Y) => T): Mock<T, Y>;

The correct fix would be to set implementation as optional, without touching generics.

function fn<T, Y extends any[]>(implementation?: (...args: Y) => T): Mock<T, Y>;

And add a test:

interface TestFn {
    test(x: number): string
}
// $ExpectType Mock<string, [number]>
const mock12 = jest.fn<ReturnType<TestFn['test']>, ArgsType<TestFn['test']>>();

[fgrandel]

To be honest, I much prefer the previous template, that's why I re-introduced it. In fact I believe that your upgrade has introduced unnecessary breaking changes (as compared to 23.x) which should be considered regressions rather than legitimate breaking changes.

IMO your changes reduce typing in many cases and introduce potential for inconsistency. I believe that something like jest.fn<number> or jest.fn<..., [never]> should NOT be valid. The same is true for other types like Mock which I didn't touch. So I think my PR is actually a fix not a change.

But don't get me wrong: Introducing typed arguments was a good thing which I maintain in my PR.

What was your rationale to do it like you did? Do you agree in principle that my solution would have been easier to upgrade to for most 23.x users, introduces less redundancy and potential for error? Or am I just unaware of some subtlety or less obvious use case?

Maybe you can explain this a little for me and others who happen to stumble upon this?

Of course I'm just guessing and it's for you and the maintainers to decide...

[antoinebrault]

Example: I believe that something like jest.fn<number> or jest.fn<..., [never]> should NOT be valid.

Had a long discussion about default values in my first PR #31094 . There was a lot of pushback about removing defaults and introducing breaking change in existing generics. I would like to simplify the generics like you too.

Passing the function as the generic should've been the way when they first did it, but it wasn't. I agree that we should change it in the future, I just don't think we should do it for a bug in 1 declaration. If we change it, it has to be consistent across the api (Mock, MockInstance, SpyInstance, etc.)

[fgrandel]

Ok, I'll revert to the <X, Y> types and opened up an issue to track the <X, Y> -> <F> change, see #32901. Stay tuned. :-)

[G-Rath]

@Jerico-Dev would you be able to update your "Working sample implementation of the above mocking API" Jest issue to reflect this change, or otherwise update the link in the issue to point to your new PR, if that's required for it to work :)

[fgrandel]

@G-Rath I'm sorry that my feature request still pointed to this issue. The actual change needed to make the feature request work is #32901 (see there). We initially had the two things mixed but then split them in two as one was easily acceptable to the maintainers and the other (the open one) isn't. As long as the maintainers do not show any interest in the change I won't update the PR.

[fgrandel]

I updated jestjs/jest#7832 to point to the right issue/PR, thanks @G-Rath for your heads-up.

[typescript-bot]

@Jerico-Dev One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits or comments. Thank you!

[fgrandel]

I pushed another version that introduces a test showcasing the Api-Mocking use case as proposed by @antoinebrault . Will change the template as soon as I fully understand the background (see my previous post).

[typescript-bot]

🔔 @antoinebrault - Thanks for your review of this PR! Can you please look at the new code and update your review status if appropriate?

[fgrandel]

The last push addresses @antoinebrault 's review comments: Types have been reverted to <X, Y>. Tests reproduce the corrected issue.

[typescript-bot]

We've gotten sign-off from a reviewer 👏. A maintainer will soon review this PR and merge it if there are no issues. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for contributing to DefinitelyTyped!

[typescript-bot]

I just published @types/jest@24.0.5 to npm.