-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Throw explicit errors for common moduleFileExtension failures #7160
Changes from all commits
d0b9d12
a1a7d42
fd7bad4
2e36bb5
8564b9b
6440839
54345b8
be6279a
cb34a10
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
// Jest Snapshot v1, https://goo.gl/fbAQLP | ||
|
||
exports[`show error message when no js moduleFileExtensions 1`] = ` | ||
"● Validation Error: | ||
|
||
moduleFileExtensions must include 'js': | ||
but instead received: | ||
[\\"jsx\\"] | ||
Please change your configuration to include 'js'. | ||
|
||
Configuration Documentation: | ||
https://jestjs.io/docs/configuration.html | ||
|
||
" | ||
`; | ||
|
||
exports[`show error message with matching files 1`] = ` | ||
"FAIL __tests__/test.js | ||
● Test suite failed to run | ||
|
||
Cannot find module './some-json-file' from 'index.js' | ||
|
||
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']. | ||
|
||
See https://jestjs.io/docs/en/configuration#modulefileextensions-array-string | ||
|
||
> 1 | module.exports = require('./some-json-file'); | ||
| ^ | ||
2 | | ||
|
||
at packages/jest-resolve/build/index.js:221:17 | ||
at index.js:1:18 | ||
|
||
" | ||
`; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
/** | ||
* Copyright (c) 2014-present, Facebook, Inc. All rights reserved. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
* | ||
* @flow | ||
*/ | ||
'use strict'; | ||
|
||
import path from 'path'; | ||
import runJest from '../runJest'; | ||
import {cleanup, extractSummary, writeFiles} from '../Utils'; | ||
|
||
const DIR = path.resolve(__dirname, '../resolve_no_extensions-no-js'); | ||
|
||
beforeEach(() => cleanup(DIR)); | ||
afterAll(() => cleanup(DIR)); | ||
|
||
test('show error message with matching files', () => { | ||
const {status, stderr} = runJest('resolve_no_extensions'); | ||
const {rest} = extractSummary(stderr); | ||
|
||
expect(status).toBe(1); | ||
expect(rest).toMatchSnapshot(); | ||
}); | ||
|
||
test('show error message when no js moduleFileExtensions', () => { | ||
writeFiles(DIR, { | ||
'index.jsx': ` | ||
module.exports ={found: true}; | ||
`, | ||
'package.json': ` | ||
{ | ||
"jest": { | ||
"moduleFileExtensions": ["jsx"] | ||
} | ||
} | ||
`, | ||
'test.jsx': ` | ||
const m = require('../'); | ||
|
||
test('some test', () => { | ||
expect(m.found).toBe(true); | ||
}); | ||
`, | ||
}); | ||
|
||
const {status, stderr} = runJest('resolve_no_extensions-no-js'); | ||
|
||
expect(status).toBe(1); | ||
expect(stderr).toMatchSnapshot(); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
const m = require('../'); | ||
|
||
test('some test', () => { | ||
expect(m.found).toBe(true); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
module.exports = require('./some-json-file'); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
{ | ||
"jest": { | ||
"moduleFileExtensions": [ | ||
"js" | ||
] | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
{ | ||
"found": true | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,87 @@ | ||
// flow-typed signature: 7c09aef8ac07163d6ef9e3f50c6bc35c | ||
// flow-typed version: a12a42a747/glob_v7.1.x/flow_>=v0.42.x | ||
|
||
declare module "glob" { | ||
declare type MinimatchOptions = {| | ||
debug?: boolean, | ||
nobrace?: boolean, | ||
noglobstar?: boolean, | ||
dot?: boolean, | ||
noext?: boolean, | ||
nocase?: boolean, | ||
nonull?: boolean, | ||
matchBase?: boolean, | ||
nocomment?: boolean, | ||
nonegate?: boolean, | ||
flipNegate?: boolean | ||
|}; | ||
|
||
declare type Options = {| | ||
...MinimatchOptions, | ||
cwd?: string, | ||
root?: string, | ||
nomount?: boolean, | ||
mark?: boolean, | ||
nosort?: boolean, | ||
stat?: boolean, | ||
silent?: boolean, | ||
strict?: boolean, | ||
cache?: { | ||
[path: string]: boolean | "DIR" | "FILE" | $ReadOnlyArray<string> | ||
}, | ||
statCache?: { | ||
[path: string]: boolean | { isDirectory(): boolean } | void | ||
}, | ||
symlinks?: { [path: string]: boolean | void }, | ||
realpathCache?: { [path: string]: string }, | ||
sync?: boolean, | ||
nounique?: boolean, | ||
nodir?: boolean, | ||
ignore?: string | $ReadOnlyArray<string>, | ||
follow?: boolean, | ||
realpath?: boolean, | ||
absolute?: boolean | ||
|}; | ||
|
||
/** | ||
* Called when an error occurs, or matches are found | ||
* err | ||
* matches: filenames found matching the pattern | ||
*/ | ||
declare type CallBack = (err: ?Error, matches: Array<string>) => void; | ||
|
||
declare class Glob extends events$EventEmitter { | ||
constructor(pattern: string): this; | ||
constructor(pattern: string, callback: CallBack): this; | ||
constructor(pattern: string, options: Options, callback: CallBack): this; | ||
|
||
minimatch: {}; | ||
options: Options; | ||
aborted: boolean; | ||
cache: { | ||
[path: string]: boolean | "DIR" | "FILE" | $ReadOnlyArray<string> | ||
}; | ||
statCache: { | ||
[path: string]: boolean | { isDirectory(): boolean } | void | ||
}; | ||
symlinks: { [path: string]: boolean | void }; | ||
realpathCache: { [path: string]: string }; | ||
found: Array<string>; | ||
|
||
pause(): void; | ||
resume(): void; | ||
abort(): void; | ||
} | ||
|
||
declare class GlobModule { | ||
Glob: Class<Glob>; | ||
|
||
(pattern: string, callback: CallBack): void; | ||
(pattern: string, options: Options, callback: CallBack): void; | ||
|
||
hasMagic(pattern: string, options?: Options): boolean; | ||
sync(pattern: string, options?: Options): Array<string>; | ||
} | ||
|
||
declare module.exports: GlobModule; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,6 +36,7 @@ import DEFAULT_CONFIG from './Defaults'; | |
import DEPRECATED_CONFIG from './Deprecated'; | ||
import setFromArgv from './setFromArgv'; | ||
import VALID_CONFIG from './ValidConfig'; | ||
|
||
const ERROR = `${BULLET}Validation Error`; | ||
const PRESET_EXTENSIONS = ['.json', '.js']; | ||
const PRESET_NAME = 'jest-preset'; | ||
|
@@ -548,6 +549,32 @@ export default function normalize(options: InitialOptions, argv: Argv) { | |
case 'testRegex': | ||
value = options[key] && replacePathSepForRegex(options[key]); | ||
break; | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe we can mention about not being able to inject Jest into runtime here? From the PR summary:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in the comment, or in the error message to the user? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the comment in particular is about the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. comment about type is fine. I was thinking about leaving a message why we care about "js" being always present |
||
if (Array.isArray(value) && !value.includes('js')) { | ||
const errorMessage = | ||
` moduleFileExtensions must include 'js':\n` + | ||
` but instead received:\n` + | ||
` ${chalk.bold.red(JSON.stringify(value))}`; | ||
|
||
// If `js` is not included, any dependency Jest itself injects into | ||
// the environment, like jasmine or sourcemap-support, will need to | ||
// `require` its modules with a file extension. This is not plausible | ||
// in the long run, so it's way easier to just fail hard early. | ||
// We might consider throwing if `json` is missing as well, as it's a | ||
// fair assumption from modules that they can do | ||
// `require('some-package/package') without the trailing `.json` as it | ||
// works in Node normally. | ||
throw createConfigError( | ||
errorMessage + | ||
"\n Please change your configuration to include 'js'.", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we remove 'js' from the config and just always use it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you mean just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 😉 |
||
); | ||
} | ||
|
||
break; | ||
} | ||
case 'automock': | ||
case 'bail': | ||
case 'browser': | ||
|
@@ -571,7 +598,6 @@ export default function normalize(options: InitialOptions, argv: Argv) { | |
case 'listTests': | ||
case 'logHeapUsage': | ||
case 'mapCoverage': | ||
case 'moduleFileExtensions': | ||
case 'name': | ||
case 'noStackTrace': | ||
case 'notify': | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
// Jest Snapshot v1, https://goo.gl/fbAQLP | ||
|
||
exports[`Runtime requireModule with no extension throws error pointing out file with extension 1`] = ` | ||
"Cannot find module 'RegularModuleWithWrongExt' from 'root.js' | ||
|
||
However, Jest was able to find: | ||
'./RegularModuleWithWrongExt.txt' | ||
|
||
You might want to include a file extension in your import, or update your 'moduleFileExtensions', which is currently ['js', 'json', 'jsx', 'node']. | ||
|
||
See https://jestjs.io/docs/en/configuration#modulefileextensions-array-string" | ||
`; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
/** | ||
* Copyright (c) 2014-present, Facebook, Inc. All rights reserved. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
* | ||
*/ | ||
|
||
'use strict'; | ||
|
||
let createRuntime; | ||
|
||
describe('Runtime requireModule with no extension', () => { | ||
beforeEach(() => { | ||
createRuntime = require('createRuntime'); | ||
}); | ||
|
||
it('throws error pointing out file with extension', async () => { | ||
const runtime = await createRuntime(__filename); | ||
|
||
expect(() => | ||
runtime.requireModuleOrMock( | ||
runtime.__mockRootPath, | ||
'RegularModuleWithWrongExt', | ||
), | ||
).toThrowErrorMatchingSnapshot(); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
some file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is such a good error