-
-
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
Ensure mocks get resolved too #7687
Conversation
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.
LGTM
packages/jest-resolve/src/index.js
Outdated
} | ||
|
||
// 2. Check if the module is a haste module (but now, in the mocks). | ||
module = this.getModuleMock(dirname + path.sep + 'index.js', moduleName); |
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.
is this index.js
safe here?
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.
It is. getMockModule
uses it to pass it to _resolveStubModuleName
, which does a dirname
again. It's unfortunate that getModule
and getMockModule
have such a different interface; but I'm going to not refactor it for now.
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.
I'd like to have a test for this - other than that, LGTM!
packages/jest-resolve/src/index.js
Outdated
} | ||
|
||
// 2. Check if the module is a haste module (but now, in the mocks). | ||
module = this.getMockModule(dirname + path.sep + 'index.js', moduleName); |
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.
Have you considered how (if at all) this should interact with the paths
option to resolve
?
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.
Yep; this replicates the step 1, but using the mock registry of the Haste map, instead of the module registry. However, it goes after 1, since, between a file and a mock, the file should always be returned by require.resolve
, even if mocked (deciding the responsibility of what object to return is part of require
, not require.resolve
).
Adding this extra step aims to be able to resolve Haste mocks which do not have a real file counter part; e.g.:
+ Foo.js
+ Bar.js
+ __mocks__
+ Foo.js
+ Baz.js
In the above example, require('Foo')
, require('Bar')
and require('Baz')
all work (when using Haste); however only require.resolve('Foo')
and require.resolve('Bar')
work. Also, require.resolve('Foo')
returns the root path, even if Foo
is mocked.
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.
Sorry if I was unclear, I meant something like require.resolve('./mod', {paths: ['../dir']})
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.
paths
are not used for Haste resolutions :)
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.
Missing test and changelog entry
Please test this internally at FB before merging. It can potentially break lots of tests. It's ok to patch the resolution to take this into account, but I think we should rewrite this logic at some point. The code is quite complex and hard to reason about, and it doesn't work exactly as documented (e.g.: you don't need to put mocks for installed modules in a directory that's sibling of |
Yeah, I agree. @SimenB Added a test and changelog :) |
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. |
With the current implementation, mocks can be required by using
require('MyMockName')
, butrequire.resolve
fails. This is because the resolver only checks for existence of a Haste name under the "module" map, but not under the "moduleMock" map.This PR fixes it, by adding a secondary check after a Haste name, to ensure the proper Haste file is loaded if present.