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

Split internal module registry from main module registry #1572

Merged
merged 3 commits into from
Sep 2, 2016

Conversation

skevy
Copy link
Contributor

@skevy skevy commented Sep 2, 2016

Here's a possible fix for #1571.

When the mkdirp module is first required, it's required from jest-utils, which is required from jasmine-check.js, which is required using requireInternalModule from jest-jasmine2/index.js. Because requiring internal modules and requiring "normal" modules (e.g. in a test or within your own code) use the same module registry, when mkdirp is required by the app code, Jest already has required it (as an internal module), and thus it doesn't try to require it again and just returns it from the module registry, meaning it doesn't pick up the fs mock from our test file.

This PR splits the module registries so that conflict doesn't happen, and we can reliably use internal modules within Jest without conflicting with the test environment.

P.S. Sorry for being annoying earlier @cpojer...this is my attempt at redemption.

@cpojer
Copy link
Member

cpojer commented Sep 2, 2016

wow, I was just messing with you but if that is what it takes to get you to contribute, that's awesome.

I also cannot believe that you actually went into jest-runtime and just made an awesome fix like this. I literally had this internal registry before but it was way more complex and I thought we didn't need it. Then I added "* Make internal registry that's persistent (?)" to my wishlist. You deserve a medal.

Would you mind also adding a test for this? It can be either an integration test in integration_tests (every folder should contain a test project and then the top level tests folder is used to call runJest (jestception)) or just directly in jest-runtime. I think I prefer integration test with a one or two line code comment on why it is necessary.

@ghost ghost added the CLA Signed ✔️ label Sep 2, 2016
@codecov-io
Copy link

codecov-io commented Sep 2, 2016

Current coverage is 90.31% (diff: 100%)

Merging #1572 into master will increase coverage by 0.01%

@@             master      #1572   diff @@
==========================================
  Files            28         28          
  Lines          1123       1125     +2   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1014       1016     +2   
  Misses          109        109          
  Partials          0          0          

Powered by Codecov. Last update 8b80506...430040f

@skevy
Copy link
Contributor Author

skevy commented Sep 2, 2016

Added integration test (with a longer than 1 line comment, sorrysss)

@cpojer cpojer merged commit d8d4256 into jestjs:master Sep 2, 2016
@cpojer
Copy link
Member

cpojer commented Sep 2, 2016

thanks, this is great. 🎉

tushardhole pushed a commit to tushardhole/jest that referenced this pull request Aug 21, 2017
Split internal module registry from main module registry
@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 14, 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.

3 participants