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

Replace usage of Jest globals with @jest/globals package #119

Merged
merged 4 commits into from
Sep 6, 2024

Conversation

macko911
Copy link
Contributor

@macko911 macko911 commented May 19, 2023

Attempts to fix #116

Removed @types/jest package.

Added @jest/globals as peer dependency. I'm not sure about which versions to support, but Jest 28 introduced a breaking change in jest.Mock type definition so I'm not confident how it would work with earlier versions.

Note that this definition is different between @types/jest@29 and @jest/globals@29

The change is described in jestjs/jest#12479
tl;dr the generic type is changed from jest.fn<ReturnType<MyFunctionType>, Parameters<MyFunctionType>> to jest.fn<MyFunctionType>

I did my best to fix the new type errors in the library but I had to add a couple of // @ts-expect-error comments in the process.
The new types work across internal tests and when used as a dependency in our own repo but I'm not sure if they are valid for every existing use case for this library.

Copy link
Contributor

@skovhus skovhus left a comment

Choose a reason for hiding this comment

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

This looks great! 👏

@zavarka
Copy link

zavarka commented Nov 28, 2023

Hi, @skovhus. Thanks for reviewing this merge request. Can you please merge it? I'm hitting this issue too and would love to get this fix. Thank you.

And thank you, @macko911, for putting it together!

@skovhus
Copy link
Contributor

skovhus commented Nov 28, 2023

Hi, @skovhus. Thanks for reviewing this merge request. Can you please merge it? I'm hitting this issue too and would love to get this fix. Thank you.

I would love to, but I'm not a maintainer of jest-mock-extended. ;)

FYI @marchaos

@leighman
Copy link

I think this would also help with bun compatibility since it rewrites references to @jest/globals ?
It doesn't seem to work currently

@darthmaim
Copy link

Hey @marchaos, can we somehow help to move this PR along? Is there something still missing for you?

@marchaos
Copy link
Owner

marchaos commented Sep 6, 2024

Hey, I'll take a look at this now.

src/Mock.ts Outdated
export interface CalledWithMock<T, Y extends any[]> extends jest.Mock<T, Y> {
calledWith: (...args: Y | MatchersOrLiterals<Y>) => jest.Mock<T, Y>;
export interface CalledWithMock<T extends FunctionLike> extends jest.Mock<T> {
calledWith: (...args: [...Parameters<T>] | MatchersOrLiterals<[...Parameters<T>]>) => jest.Mock<T>;
Copy link
Owner

Choose a reason for hiding this comment

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

I think this change here is why the 2 // @ts-expect-error are required in Mock.spec.ts, and I suspect that is a breaking change.

Just seeing if I can improve this to avoid that.

@marchaos
Copy link
Owner

marchaos commented Sep 6, 2024

I've pushed some changes:

  • Fixed the conflicts with master
  • Removed the ts-expect-error on the tests which was a genuine TS issue.
  • A couple of chore tasks - changed ts-ignore to ts-expect error and removed those that weren't ignoring anything, ugraded ts-essentials.

The changed to CalledWithMock to take 1 argument instead of 2 is a breaking change as this type is exported and may cause TS errors for anyone upgrading. It's not something that is used often though.

I ran this branch against a private code base that uses jest-mock-extended extensively, and all worked as expected once I fixed the CalledWithMock, except one place which resulted in a TS error, which seems a bit odd.

image

Would be good for someone to take a look at my changes, and potentially try this branch out for themselves? I can release a beta (4.0.0-beta) to npm of that makes it easier?

export interface CalledWithMock<T, Y extends any[]> extends jest.Mock<T, Y> {
calledWith: (...args: Y | MatchersOrLiterals<Y>) => jest.Mock<T, Y>;
export interface CalledWithMock<T extends FunctionLike> extends jest.Mock<T> {
calledWith: (...args: [...MatchersOrLiterals<Parameters<T>>]) => jest.Mock<T>;
Copy link
Owner

Choose a reason for hiding this comment

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

Changed this to just MatchersOrLiterals since that type will check literals for us - the original type for args was Y | MatchersOrLiterals<Y>, which really could have just been MatchersOrLiterals<Y>

@darthmaim
Copy link

I've just tested this branch in one of my projects (which admittedly only uses jest-mock-extended very sparingly), and everything was working nicely!

@marchaos marchaos merged commit dd07a44 into marchaos:master Sep 6, 2024
@marchaos
Copy link
Owner

marchaos commented Sep 6, 2024

I've pushed a version to npm 4.0.0-beta1 if you want to try this out. Will release 4.0.0 with these fixes ones we've garnered some feedback.

@leighman
Copy link

leighman commented Sep 9, 2024

Gave it a try.

Had an additional type error

Type 'DeepMockProxy<SigningKey>' is not assignable to type 'void | Promise<SigningKey>'

but I think it's actually correct/the problem is with the other types.

Otherwise seems good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Doesn't work if injectGlobals is false
6 participants