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

feat(@jest/environment, jest-runtime): allow passing a generic type argument to jest.createMockFromModule<T>() method #13202

Merged
merged 17 commits into from Sep 3, 2022

Conversation

mrazauskas
Copy link
Contributor

@mrazauskas mrazauskas commented Aug 31, 2022

Summary

Closes #13201
Resolves #13199

As it was noticed in #13199 currently jest.createMockFromModule() returns unknown type. This does not align neither with @jest/types, nor with the actual behaviour of the method.

The PR adds a possibility to pass a generic type argument T to the method. This way the returned value can be typed as Mocked<T> which matches the shape of the value.

Test plan

A type test is added.

Copy link

@viceice viceice left a comment

Choose a reason for hiding this comment

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

looks good, but needs to be compatible with the @types/jest types

@SimenB
Copy link
Member

SimenB commented Aug 31, 2022

I disagree about that part - source of truth is this repo. Unless @types/jest is more correct, we shouldn't copy their approach - compatibility isn't in and off itself a goal (DT needs to align with us)

@mrazauskas mrazauskas changed the title fix(@jest/environment, jest-mock, jest-runtime): align createMockFromModule() type with its behaviour feat(@jest/environment, jest-runtime): allow jest.createMockFromModule<T>() method to take a generic type argument Sep 3, 2022
Comment on lines +71 to +73
expectType<Mocked<typeof someModule>>(
jest.createMockFromModule<typeof someModule>('moduleName'),
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a minimal test, because the serious ones landed in #13207

@mrazauskas mrazauskas changed the title feat(@jest/environment, jest-runtime): allow jest.createMockFromModule<T>() method to take a generic type argument feat(@jest/environment, jest-runtime): allow passing a generic type argument to jest.createMockFromModule<T>() method Sep 3, 2022
Comment on lines -171 to 173
private readonly _mockMetaDataCache: Map<string, MockFunctionMetadata>;
private readonly _mockMetaDataCache: Map<string, MockMetadata<any>>;
private _mockRegistry: Map<string, any>;
private _isolatedMockRegistry: Map<string, any> | null;
Copy link
Contributor Author

@mrazauskas mrazauskas Sep 3, 2022

Choose a reason for hiding this comment

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

Ideally these three anys should be some T (a type of some mock module). unknown does not work unfortunately, because T cannot be assigned to unknown. The T could be assigned to T, but there is no way to have it here. Tricky indeed.

At the same time, here the shape of the mock is not important at all. Hence any looked fine. Hm.. I will try one more time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. No luck. It might make sense to create type MockModule = any and to use it here instead of any. Looks redundant, but perhaps that way this is more clear?

Copy link
Member

Choose a reason for hiding this comment

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

nah, current approach is fine 👍

@mrazauskas mrazauskas marked this pull request as ready for review September 3, 2022 11:08
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.

great!

@SimenB SimenB merged commit 5c22e4f into jestjs:main Sep 3, 2022
@mrazauskas mrazauskas deleted the feat-createMockFromModule-return-type branch September 3, 2022 11:19
@mrazauskas
Copy link
Contributor Author

@viceice The PR has landed, but I do understand it did not solve your issue. Now someone should make similar change in @types/jest to align everything.

They have Mocked type defined, its shape is slightly different, not sure if that will work. Might be there will be a need to copy (or import?) Mocked from jest-mock. Uff.. After digging though all the complexity for this PR, I am not interested for more of the same (; Just trying to explain. I am not interested to work on @types/jest., but just ping me if something gets unclear. Glad to take a look.

By the way, you can also explicitly import all APIs from @jest/globals instead of relying on @types/jest. That's what I do in my projects. One dependency less – always good idea.

@viceice
Copy link

viceice commented Sep 3, 2022

yeah, i think I'll remove that dependency too and manually declare the required globals until we refactored the whole solution (more that 1000 tests in hundreds of files)

@mrazauskas
Copy link
Contributor Author

That should work too. If you will find something missing or incorrect, please open an issue. In general Jest built-in types are in very good shape. Some niche features like jest.Mock type are not yet implemented, but they are in the plan. Easy to prioritise, if there is a use case.

By the way, it might be tricky to have utility types like jest.Mocked, jest.MockedClass, etc. in a manual .d.ts declaration. If you need them, most probably the implementation should to be copied from here:

https://github.com/facebook/jest/blob/0cfc2ad2991373324306c8daf962c41d0f37793c/packages/jest-globals/src/index.ts#L35-L57

@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 19, 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.

[Bug]: Interface 'JestImportMeta' incorrectly extends interface 'ImportMeta'
5 participants