-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
refactor(jest-mock)!: change the default jest.mocked
helper’s behaviour to deep mocked
#13125
refactor(jest-mock)!: change the default jest.mocked
helper’s behaviour to deep mocked
#13125
Conversation
jest.mocked
helper’s behaviour to deep mocked
Mye only question is: import thing from 'thing';
jest.mock('thing');
const mockedThing = jest.mocled(thing); Would |
Current behaviour: The test is passing. Hence Swapping the defaults makes this example work without type errors. Behaviour becomes aligned between |
Yep! |
docs/MockFunctionAPI.md
Outdated
import fetch from 'node-fetch'; | ||
import {expect, jest, test} from '@jest/globals'; | ||
import type {fetch} from './fetch'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks wrong - a type import will fail the typeof
below. But why not keep the node-fetch
import?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm.. Checked it. type
works, TS does not complain. Similar to the above: https://github.com/facebook/jest/blob/2f4340cb1da435a9b147e238f7034f99653b8070/docs/MockFunctionAPI.md?plain=1#L500-L505
I was not happy that ESLint (import/order
) is pushing to keep 'fetch'
import above '@jest/globals'
. Probably some ignore comment could fix this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome, thanks!
Just checked. The example from #13125 (comment) now does not give any type error. Sweet. Thanks! |
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. |
Split from #12727
Summary
It seems to me that by default
jest.mocked()
should wrap the deep methods of passed object. Currently it requires and argument:jest.mocked(someObject, true)
. Also note that without reading the docs it isn’t clear whattrue
does.Consider this example:
The test will pass. Seems like
jest.mock()
is mocking deeply nested methods. So I thinkjest.mocked()
should do the same by default (andjest.Mocked<T>
as well).Possibly shallow mocked definitions were implemented to improve performance of TS compiler. Or to work around some limitation, i.e. handling of recursive types. If I got it right,
ts-jest
implementedjest.mock()
in the early days of TypeScript v3. Performance and handling of recursive types improved a lot in TS v4.Perhaps it’s time to swap the defaults? I mean, to make the behaviour of
jest.mocked()
deep by default and to allow opting out with an option:jest.mocked(someObject, {shallow: true})
.Test plan
Type tests added.