Support jest-preset.js files within Node modules. #4499
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #4499 +/- ##
=========================================
+ Coverage 56.85% 56.9% +0.04%
=========================================
Files 186 186
Lines 6300 6304 +4
Branches 3 3
=========================================
+ Hits 3582 3587 +5
+ Misses 2717 2716 -1
Partials 1 1
Continue to review full report at Codecov.
|
cpojer
reviewed
Sep 18, 2017
| }, | ||
| ); | ||
| const foundModule = PRESET_EXTENSIONS.some(ext => { | ||
| const presetModule = Resolver.findNodeModule( |
There was a problem hiding this comment.
findNodeModule has an extensions options. Mind simplifying your code a bit? :)
cpojer
reviewed
Sep 18, 2017
|
|
||
| try { | ||
| // $FlowFixMe | ||
| preset = (require(presetModule): InitialOptions); |
There was a problem hiding this comment.
I worry about this not working with multiple projects, you'd want the preset to be re-evaluated every time. Can we remove it from the require.cache right after requiring it? :)
There was a problem hiding this comment.
And guard that in a separate try-catch, I don't want Jest to rely on require.cache to be present :D
|
I like it. Added two comments to simplify the code and make it more resilient :) |
|
@cpojer So I'm trying to use the I added some logging, |
|
yeah it seems like your path is wrong, because it is absolute, not relative to the preset. |
|
@cpojer This is actually failing for the existing |
|
It would be a great feature to have. Our use case requires us to generate the |
|
I'll try and tackle this again this weekend. |
|
Hi @milesj, do you think this could land before v22 is released? Thanks! |
|
@bazyli-brzoska I keep forgetting about this :P But most likely can tackle it this week. |
|
@milesj hey! Did you ever find time to take another look at this? Thanks. |
|
@cpojer I looked a little bit but ran into the same issues. Haven't had enough time due to the holidays. |
|
@cpojer I could take a look -- what's the ETA before v22 release so I can estimate how much time we have? |
|
Literally two minutes. |
|
@bazyli-brzoska if all goes well, we release today. But this feature can land in a feature release, e.g. 22.1, it's not a blocker so no worries. |
|
Sorry guys, holidays got in the way. That being said, my christmas break started yesterday, so I'll definitely have time. Most likely today or tomorrow (assuming no one has started it yet). |
|
@milesj No pressure, after all, we do it in our free time ;) I haven't started yet, so it's all yours. 💪 |
|
I rebased and made some changes. However, I seem to be having issues with my Yarn installation that I can't seem to resolve. Continuously getting this kind of output: |
|
My goto for those issues is |
|
The problem happens on all my projects and seems to be an issue with node-gyp/fsevents. Digging a bit further. |
|
Got my yarn working again. Looking into this. Edit: I rebased off master and it seems like test/builds currently fail, which is problematic for my testing. Will wait a bit. |
|
Seems like legitimate failures, is it green locally for you? |
|
My master (rebased today from upstream) has 17 failing tests. Test Suites: 4 failed, 207 passed, 211 total |
|
Master is green on CI and on my machine. Have you tried what I suggested, |
|
Yup a few times, here's the output. |
|
Does it still happen if you install mercurial? |
|
Hey guys! What's the status of this? The tests are clearly failing on CI, can we get that fixed and merge this PR? :) |
|
I can't seem to get my fork to build correctly, even outside of this branch. So if anyone wants to take this over, feel free. |
|
I'm gonna close this for now since there aren't any volunteers who are willing to take this over the finish line. If anybody sees this and wants this feature, please send a new PR rebased on top of master, with tests. Thanks! |
|
@cpojer I'll put this on my backlog and I'll try to handle it within the next month or so. |
|
For anyone following along, this has been merged in #6185 |
|
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. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Currently, Jest configuration may be a .json or .js file, but presets only support .json. This PR makes a change to also support jest-preset.js files within node modules.
This change would allow modules to make use of
process.env.NODE_ENV.Test plan
Unit tests.
And I've verified relative filesystem paths in another project of mine.