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

Ensure mocks get resolved too #7687

Merged
merged 1 commit into from
Jan 24, 2019
Merged

Ensure mocks get resolved too #7687

merged 1 commit into from
Jan 24, 2019

Conversation

mjesun
Copy link
Contributor

@mjesun mjesun commented Jan 23, 2019

With the current implementation, mocks can be required by using require('MyMockName'), but require.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.

Copy link
Member

@rickhanlonii rickhanlonii left a comment

Choose a reason for hiding this comment

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

LGTM

}

// 2. Check if the module is a haste module (but now, in the mocks).
module = this.getModuleMock(dirname + path.sep + 'index.js', moduleName);
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Contributor

@jeysal jeysal left a 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!

}

// 2. Check if the module is a haste module (but now, in the mocks).
module = this.getMockModule(dirname + path.sep + 'index.js', moduleName);
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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']})

Copy link
Contributor Author

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 :)

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.

Missing test and changelog entry

@rubennorte
Copy link
Contributor

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 node_modules, you can put them anywhere).

@mjesun
Copy link
Contributor Author

mjesun commented Jan 24, 2019

Yeah, I agree. require should rely 100% in require.resolve, by telling it if the module is expected to be mocked or not.

@SimenB Added a test and changelog :)

@mjesun mjesun merged commit 28971c5 into jestjs:master Jan 24, 2019
@mjesun mjesun deleted the resolve-mocks branch January 24, 2019 16:04
ulivz added a commit to vuejs/vuepress that referenced this pull request Apr 14, 2019
captain-yossarian pushed a commit to captain-yossarian/jest that referenced this pull request Jul 18, 2019
devs-cloud pushed a commit to devs-cloud/vue-dev that referenced this pull request Dec 27, 2019
@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 May 12, 2021
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.

7 participants