Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add module.loaded, and module.require should not be enumerable #4623

Merged
merged 3 commits into from
Oct 7, 2017

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Oct 7, 2017

Summary
As mentioned in #4614 (comment) there are 2 ways that module in normal node and module in jest diverge (at the top level, #4614 addresses module.parent being faked).

  1. module.requireshould not be enumerable.
  2. module.loaded is missing in jest's implementation. (https://nodejs.org/api/modules.html#modules_module_loaded)

This PR fixes both of those issues.

Test plan
New test added

@SimenB SimenB force-pushed the enumerable-module-require branch from 0c98697 to 4e0bb4c Compare October 7, 2017 10:20
@SimenB SimenB mentioned this pull request Oct 7, 2017
@SimenB SimenB force-pushed the enumerable-module-require branch 3 times, most recently from d7848d1 to baadeae Compare October 7, 2017 11:31
@@ -29,13 +29,14 @@ import {run as cilRun} from './cli';
import {options as cliOptions} from './cli/args';

type Module = {|
Copy link
Member Author

Choose a reason for hiding this comment

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

This should be type Module = typeof module to get the type definitions from flow itself (it would have yelled at us for missing loaded, for instance). A bit more work though as it also doesn't like that require is added the way it is in this PR. Can revisit later

@codecov-io
Copy link

codecov-io commented Oct 7, 2017

Codecov Report

Merging #4623 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #4623      +/-   ##
=========================================
+ Coverage   56.18%   56.2%   +0.01%     
=========================================
  Files         194     194              
  Lines        6546    6548       +2     
  Branches        3       3              
=========================================
+ Hits         3678    3680       +2     
  Misses       2867    2867              
  Partials        1       1
Impacted Files Coverage Δ
packages/jest-runtime/src/index.js 85.21% <100%> (+0.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7b0e108...32e302c. Read the comment docs.

@cpojer
Copy link
Member

cpojer commented Oct 7, 2017

please rebase :)

@SimenB SimenB force-pushed the enumerable-module-require branch from baadeae to ea071a3 Compare October 7, 2017 12:09
@SimenB SimenB force-pushed the enumerable-module-require branch from ea071a3 to 32e302c Compare October 7, 2017 12:10
@SimenB
Copy link
Member Author

SimenB commented Oct 7, 2017

Rebased!

@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 13, 2021
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants