Throw explicit errors for common moduleFileExtension failures #7160
Conversation
|
Ping me tomorrow once I have access to the computer, but the idea sounds great! |
|
|
SimenB
force-pushed
the
detect-files-outside-moduleFileExtensions
branch
from
6bf6bc3
to
2e36bb5
Compare
Codecov Report
@@ Coverage Diff @@
## master #7160 +/- ##
==========================================
+ Coverage 66.43% 66.48% +0.04%
==========================================
Files 253 253
Lines 10658 10684 +26
Branches 3 4 +1
==========================================
+ Hits 7081 7103 +22
- Misses 3576 3580 +4
Partials 1 1
Continue to review full report at Codecov.
|
packages/jest-runtime/src/index.js
Outdated
| try { | ||
| const slashedDirname = slash(dirname); | ||
|
|
||
| const matches = glob(`${pathToModule}.*`) |
There was a problem hiding this comment.
would be nice to have the hastemap here instead of hitting the FS again, but unless I'm missing something it's not available from runtime
|
@thymikee sup? |
| case 'moduleFileExtensions': { | ||
| value = options[key]; | ||
|
|
||
| // If it's the wrong type, it can throw at a later time |
There was a problem hiding this comment.
maybe we can mention about not being able to inject Jest into runtime here?
From the PR summary:
If js is missing from moduleFileExtension, jest is unable to inject into the runtime. I decided to throw an explicit configuration error rather than fixing jest-jasmine's require, as we'd also need all of our dependencies to do the same (e.g. source-map throws if we do moduleFileExtension: [] now).
There was a problem hiding this comment.
in the comment, or in the error message to the user?
There was a problem hiding this comment.
the comment in particular is about the Array.isArray check, if that wasn't obvious. jest-validate will complain about its type if it's not an array, normalize doesn't have to care
There was a problem hiding this comment.
comment about type is fine. I was thinking about leaving a message why we care about "js" being always present
|
@thymikee how about now? |
We had, but some PR from before we did so probably got merged |
| However, Jest was able to find: | ||
| './some-json-file.json' | ||
|
|
||
| You might want to include a file extension in your import, or update your 'moduleFileExtensions', which is currently ['js']. |
| // works in Node normally. | ||
| throw createConfigError( | ||
| errorMessage + | ||
| "\n Please change your configuration to include 'js'.", |
There was a problem hiding this comment.
Could we remove 'js' from the config and just always use it?
There was a problem hiding this comment.
do you mean just pushing it in? If so, I thought about, but decided against it as I think it would be confusing for the user that we changed their configuration. Better that they explicitly list the config. Do you disagree with that?
There was a problem hiding this comment.
I generally agree with being explicit, but in this case not including it breaks the suite, so why have the user list it at all? It may be better to have a note that says "Jest will always check for '.js' extensions" in the docs and then no one has to think about it
There was a problem hiding this comment.
Sorry, your response was lost in the ether.
I still think it's confusing if you get a js file required without saying the should be without an extension. Requiring the user to be explicit is less magical. Send a PR changing it, and I won't close it, though 😉
|
Sorry I'm late to the party but this is really fly, good work 👍 |
|
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. |
Summary
Inspired by #7158, I finally decided to throw a better error in the cases where people have custom
moduleFileExtensionresulting in more or less obscure errors.I've tackled 2 different issues here (in separate commits, happy to split them up into separate PRs if you want).
requireor updatemoduleFileExtension.jsis missing frommoduleFileExtension, jest is unable to inject into the runtime. I decided to throw an explicit configuration error rather than fixingjest-jasmine'srequire, as we'd also need all of our dependencies to do the same (e.g.source-mapthrows if we domoduleFileExtension: []now).Fixes #4025.
Test plan
Integration tests added