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

[jest] add mock.getMockImplementation type #43531

Merged
merged 1 commit into from
Apr 2, 2020

Conversation

regevbr
Copy link
Contributor

@regevbr regevbr commented Mar 31, 2020

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:

@regevbr
Copy link
Contributor Author

regevbr commented Mar 31, 2020

Please note that I fixated the dependencies as newer version (starting 25.2.0) of jest are using new typescript feature import type which was introduced in 3.8 and not compatible with older typescript versions

@typescript-bot typescript-bot added this to Waiting for Reviewers in Pull Request Status Board Mar 31, 2020
@typescript-bot typescript-bot added Popular package This PR affects a popular package (as counted by NPM download counts). Awaiting reviewer feedback labels Mar 31, 2020
@typescript-bot
Copy link
Contributor

typescript-bot commented Mar 31, 2020

@regevbr Thank you for submitting this PR!

🔔 @NoHomey @jwbay @asvetliakov @alexjoverm @epicallan @ikatyang @wsmd @JamieMason @douglasduteil @ahnpnl @JoshuaKGoldberg @UselessPickles @r3nya @Hotell @sebald @andys8 @antoinebrault @favna @gstamac @ExE-Boss @quassnoi @Belco90 @tonyhallett @ycmjason @devanshj @pawfa - 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.

Copy link
Contributor

@jwbay jwbay left a comment

Choose a reason for hiding this comment

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

Thanks for leaving a note about the version pinning -- it's unfortunate, but I don't see a way around it at the moment.

@typescript-bot typescript-bot moved this from Waiting for Reviewers to Check and Merge in Pull Request Status Board Apr 2, 2020
@typescript-bot typescript-bot added Owner Approved A listed owner of this package signed off on the pull request. Merge:Express and removed Awaiting reviewer feedback labels Apr 2, 2020
@typescript-bot
Copy link
Contributor

A definition owner has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped!

@uniqueiniquity uniqueiniquity merged commit 3cc8ab0 into DefinitelyTyped:master Apr 2, 2020
Pull Request Status Board automation moved this from Check and Merge to Done Apr 2, 2020
@typescript-bot
Copy link
Contributor

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

@berickson1
Copy link
Contributor

The version pinning introduced here prevents me from updating to the latest @types/jest since we're enforcing that there aren't duplicate entries in our yarn.lock. We're using a newer version of jest-diff and pretty-format than specified in pacakge.json
I'm assuming this was problematic for building these types? Can you fix the devDependencies version at a lower version to allow for local development? This would allow consumers to pull a newer versions by restoring the carets in dependencies

@regevbr regevbr deleted the jest1 branch April 2, 2020 19:43
@regevbr
Copy link
Contributor Author

regevbr commented Apr 2, 2020

@berickson1 what you are asking for will cause all users of this package that use typescript version lower than 3.8 to get compilation / typings issues.

I don't mind increasing the min ts version to 3.8 as I'm always using the latest version but that will mean that a lot of user will not be able to get new typings updates for this package.

Also, in my humble opinion, your issue is very nish and affecting 5 millions weekly downloads for it feels like a stretch.

@jwbay what do you think? do you want to get other maintainers involved here?

p.s

@berickson1 I'm interested why do you impose such restrictions?

@berickson1
Copy link
Contributor

If someone is using an old version of typescript and already includes jest-diff in their bundle, they're likely already pinned to a compatible version that works with older versions of typescript. Bumping to the latest @types/jest wouldn't break anything. If not, they can do it manually in their own package.json, (not perfect but not the end of the world) I don't think this is going to break things for most people.

I've seen a bunch of bundle size bloat with packages being double-included via webpack when there's conflicting versions. In this case it probably would shake out right, but for examples I've seen libraries pin to specific versions of lodash, so webpack would bundle lodash twice, once for usage only in call-sites from the "offending" library and once for the rest of the app. In the case of @types it wouldn't be bundled, but could result in this old version being resolved if there was nothing more specific listed by package dependencies.

@regevbr
Copy link
Contributor Author

regevbr commented Apr 3, 2020

@berickson1 I have read up a bit on the matter after your first message. I get why it is needed when using webpack or a like. I'm coming from server side where the matter is of less importance.

What you are suggesting will not help. The reason being that if someone installs a fresh copy of this package, and they don't have jest-diff, npm/yarn will install the latest version of jest-diff which will break their typings.

The only way for them to solve the issue is to manually install or write a specific resolve to pin those libraries. It will take an average developer some time to realize how to fix the matter and will cause frustration.

My suggestion to you for now is to ignore dev dependencies in your dedupe restrictions as they are not relevant to the use case your are protecting yourself against...

@berickson1
Copy link
Contributor

I'd suspect that if someone is adding a fresh copy of this package for the first time, they're probably in a new or relatively new repo, thus likely using a new version of typescript that wouldn't have this issue.

FWIW it looks like jest-diff worked around this issue in 25.2.7 and later, so I think this can actually be fixed by bumping to ^25.2.7 (jestjs/jest@d4057ce#diff-71ab3b0daed0524930a50a1e724b5677)

I don't believe that webpack picks and chooses what to bundle based on if it's in devDependencies or not unfortunately :(

@regevbr
Copy link
Contributor Author

regevbr commented Apr 3, 2020

That's great that they addressed it, but they still impose a minimum of 3.4 ts version.
Can you draft a quick pr and the maintainers will take a look at it?

berickson1 added a commit to berickson1/DefinitelyTyped that referenced this pull request Apr 3, 2020
`jest-diff` and `pretty-format` were previously pinned in DefinitelyTyped#43531 to work around backwards compatibility issues with the typescript definitions. Facebook added tooling to allow down-leveling their type definition in jestjs/jest@d4057ce, which landed in 25.2.7, so bump the semver dependency to this.
@berickson1 berickson1 mentioned this pull request Apr 3, 2020
8 tasks
uniqueiniquity pushed a commit that referenced this pull request Apr 3, 2020
* Updated pinned dependencies

`jest-diff` and `pretty-format` were previously pinned in #43531 to work around backwards compatibility issues with the typescript definitions. Facebook added tooling to allow down-leveling their type definition in jestjs/jest@d4057ce, which landed in 25.2.7, so bump the semver dependency to this.

* Pick an older version, this change is available in 25.2.1

* Bump min version to typescript 3.1

* Bump additional typescript versions
/**
* Returns the function that was set as the implementation of the mock (using mockImplementation).
*/
getMockImplementation(): (...args: Y) => T | undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

This definition is wrong. According to https://github.com/facebook/jest/blob/master/packages/jest-mock/src/index.ts#L51, this should be getMockImplementation(): ((...args: Y) => T) | undefined;. I think you just missed a couple of parenthesis.

Copy link
Contributor

Choose a reason for hiding this comment

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

@GerkinDev I wouldn't trust all the type definitions you see on jest site.

Copy link
Contributor

Choose a reason for hiding this comment

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

@tonyhallett this isn't from jest site, but from source code type definitions. Moreover, in real use cases, if actual implementation is not defined, it returns undefined, not a function with same arity that may return undefined.

Anyway, I've PR: #50377

Copy link
Contributor

@tonyhallett tonyhallett Jan 4, 2021

Choose a reason for hiding this comment

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

@GerkinDev I was referring to the jest source code type definitions. My comment was in case you were attempting to match theirs without reading the source code.

According to https://github.com/facebook/jest/blob/master/packages/jest-mock/src/index.ts#L51

Although the jest type definition has getMockImplementation(): Function | undefined;

But
https://github.com/facebook/jest/blob/7430a7824421c122cd07035d800d22e1007408fa/packages/jest-mock/src/index.ts#L622

f.getMockImplementation = () => this._ensureMockConfig(f).mockImpl;

Also see jestjs/jest#9061

Regarding getMockImplementation, I think that's undocumented and should not be used

So your change to

getMockImplementation(): ((...args: Y) => T) | undefined;

in #50377 seems incorrect to me.

Copy link
Contributor

@GerkinDev GerkinDev Jan 4, 2021

Choose a reason for hiding this comment

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

Given

f.getMockImplementation = () => this._ensureMockConfig(f).mockImpl;

https://github.com/facebook/jest/blob/7430a7824421c122cd07035d800d22e1007408fa/packages/jest-mock/src/index.ts#L622

This calls

private _ensureMockConfig<T, Y extends Array<unknown>>(
    f: Mock<T, Y>,
  ): MockFunctionConfig {
    let config = this._mockConfigRegistry.get(f);
    if (!config) {
      config = this._defaultMockConfig();
      this._mockConfigRegistry.set(f, config);
    }
    return config;
  }

https://github.com/facebook/jest/blob/7430a7824421c122cd07035d800d22e1007408fa/packages/jest-mock/src/index.ts#L432-L441

If there is no config, it inits a new one with

  private _defaultMockConfig(): MockFunctionConfig {
    return {
      mockImpl: undefined,
      mockName: 'jest.fn()',
      specificMockImpls: [],
      specificReturnValues: [],
    };
  }

https://github.com/facebook/jest/blob/7430a7824421c122cd07035d800d22e1007408fa/packages/jest-mock/src/index.ts#L454-L461

where mockImpl is undefined.

Back in your sample

f.getMockImplementation = () => this._ensureMockConfig(f).mockImpl;

https://github.com/facebook/jest/blob/7430a7824421c122cd07035d800d22e1007408fa/packages/jest-mock/src/index.ts#L622

it's actually the mockImpl property that is returned.

I maintain my PR and am even more sure of it.


About jestjs/jest#9061, in that case, the type definition should either be removed or correct. An existing incorrect type definition just makes no sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please continue the conversation in #50377. I'll re-post this comment, and I think we should continue there.

typescript-bot pushed a commit that referenced this pull request Jan 7, 2021
… by @GerkinDev

* [@types/jest]: Fix getMockImplementation definition

Please fill in this template.

[x] Use a meaningful title for the pull request. Include the name of the package modified.
[x] Test the change in your own code. (Compile and run.)
[x] Add or edit tests to reflect the change. (Run with npm test.)
[x] Follow the advice from the readme.
[x] Avoid common mistakes.
[x] 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: #43531 (comment)

* Update tests
kaznovac pushed a commit to kaznovac/DefinitelyTyped that referenced this pull request Mar 2, 2021
…tion definition by @GerkinDev

* [@types/jest]: Fix getMockImplementation definition

Please fill in this template.

[x] Use a meaningful title for the pull request. Include the name of the package modified.
[x] Test the change in your own code. (Compile and run.)
[x] Add or edit tests to reflect the change. (Run with npm test.)
[x] Follow the advice from the readme.
[x] Avoid common mistakes.
[x] 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: DefinitelyTyped#43531 (comment)

* Update tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Owner Approved A listed owner of this package signed off on the pull request. Popular package This PR affects a popular package (as counted by NPM download counts).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants