-
Notifications
You must be signed in to change notification settings - Fork 933
Fix/3768: Consider ESM when selecting cosmiconfig loaders #3776
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
Changes from all commits
2d567b3
6dcec0c
b9cd734
145ca9d
b8c0c00
2a545c5
7c7ce18
57ce694
e100334
6ce74e5
caa286f
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,5 @@ | ||
formatter: '@commitlint/format' | ||
rules: | ||
zero: [0, 'never'] | ||
one: [1, 'always'] | ||
two: [2, 'never'] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
module.exports = { | ||
formatter: '@commitlint/format', | ||
rules: { | ||
zero: [0, 'never'], | ||
one: [1, 'always'], | ||
two: [2, 'never'], | ||
}, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
module.exports = { | ||
formatter: '@commitlint/format', | ||
rules: { | ||
zero: [0, 'never'], | ||
one: [1, 'always'], | ||
two: [2, 'never'], | ||
}, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
{ | ||
"formatter": "@commitlint/format", | ||
"rules": { | ||
"zero": [0, "never"], | ||
"one": [1, "always"], | ||
"two": [2, "never"] | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
formatter: '@commitlint/format' | ||
rules: | ||
zero: [0, 'never'] | ||
one: [1, 'always'] | ||
two: [2, 'never'] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
formatter: '@commitlint/format' | ||
rules: | ||
zero: [0, 'never'] | ||
one: [1, 'always'] | ||
two: [2, 'never'] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
module.exports = { | ||
formatter: '@commitlint/format', | ||
rules: { | ||
zero: [0, 'never'], | ||
one: [1, 'always'], | ||
two: [2, 'never'], | ||
}, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
module.exports = { | ||
formatter: '@commitlint/format', | ||
rules: { | ||
zero: [0, 'never'], | ||
one: [1, 'always'], | ||
two: [2, 'never'], | ||
}, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
export default { | ||
formatter: '@commitlint/format', | ||
rules: { | ||
zero: [0, 'never'], | ||
one: [1, 'always'], | ||
two: [2, 'never'], | ||
}, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
export default { | ||
formatter: '@commitlint/format', | ||
rules: { | ||
zero: [0, 'never'], | ||
one: [1, 'always'], | ||
two: [2, 'never'], | ||
}, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
export default { | ||
formatter: '@commitlint/format', | ||
rules: { | ||
zero: [0, 'never'], | ||
one: [1, 'always'], | ||
two: [2, 'never'], | ||
}, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
export default { | ||
formatter: '@commitlint/format', | ||
rules: { | ||
zero: [0, 'never'], | ||
one: [1, 'always'], | ||
two: [2, 'never'], | ||
}, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
{ | ||
"name": "load-test-js" | ||
} |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
export default { | ||
extends: ['./first-extended'], | ||
rules: { | ||
zero: [0, 'never'], | ||
}, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
export default { | ||
extends: ['./first-extended'], | ||
rules: { | ||
zero: [0, 'never'], | ||
}, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
{ | ||
"name": "load-test-js" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,11 @@ | ||
import { | ||
cosmiconfig, | ||
defaultLoadersSync, | ||
Options, | ||
type Loader, | ||
defaultLoaders, | ||
} from 'cosmiconfig'; | ||
import {TypeScriptLoader} from 'cosmiconfig-typescript-loader'; | ||
import {existsSync, readFileSync} from 'fs'; | ||
import path from 'path'; | ||
|
||
export interface LoadConfigResult { | ||
|
@@ -27,7 +28,12 @@ export async function loadConfig( | |
return tsLoaderInstance(...args); | ||
}; | ||
|
||
const {searchPlaces, loaders} = getDynamicAwaitConfig(); | ||
// If dynamic await is supported (Node >= v20.8.0) or directory uses ESM, support | ||
// async js/cjs loaders (dynamic import). Otherwise, use synchronous js/cjs loaders. | ||
const loaders = | ||
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. Simplified this in place of the 'getDynamicAwaitConfig' function; mjs files will now be supported for node 18 as well. Js/cjs files were originally using require, so unless the application is using ESM, require can still be used. |
||
isDynamicAwaitSupported() || isEsmModule(cwd) | ||
? defaultLoaders | ||
: defaultLoadersSync; | ||
|
||
const explorer = cosmiconfig(moduleName, { | ||
searchPlaces: [ | ||
|
@@ -40,22 +46,22 @@ export async function loadConfig( | |
`.${moduleName}rc.yml`, | ||
`.${moduleName}rc.js`, | ||
`.${moduleName}rc.cjs`, | ||
`.${moduleName}rc.mjs`, | ||
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. No need to conditionally support mjs; it's just the tests that can't load these files for node < 20.8.0, and I've excluded the test as appropriate. |
||
`${moduleName}.config.js`, | ||
`${moduleName}.config.cjs`, | ||
`${moduleName}.config.mjs`, | ||
|
||
// files supported by TypescriptLoader | ||
`.${moduleName}rc.ts`, | ||
`.${moduleName}rc.cts`, | ||
`${moduleName}.config.ts`, | ||
`${moduleName}.config.cts`, | ||
|
||
...(searchPlaces || []), | ||
], | ||
loaders: { | ||
'.ts': tsLoader, | ||
'.cts': tsLoader, | ||
|
||
...(loaders || {}), | ||
'.cjs': loaders['.cjs'], | ||
'.js': loaders['.js'], | ||
}, | ||
}); | ||
|
||
|
@@ -71,7 +77,7 @@ export async function loadConfig( | |
return null; | ||
} | ||
|
||
// See the following issues for more context: | ||
// See the following issues for more context, contributing to failing Jest tests: | ||
// - Issue: https://github.com/nodejs/node/issues/40058 | ||
// - Resolution: https://github.com/nodejs/node/pull/48510 (Node v20.8.0) | ||
export const isDynamicAwaitSupported = () => { | ||
|
@@ -83,18 +89,14 @@ export const isDynamicAwaitSupported = () => { | |
return major >= 20 && minor >= 8; | ||
}; | ||
|
||
// If dynamic await is supported (Node >= v20.8.0), support mjs config. | ||
// Otherwise, don't support mjs and use synchronous js/cjs loaders. | ||
export const getDynamicAwaitConfig = (): Partial<Options> => | ||
isDynamicAwaitSupported() | ||
? { | ||
searchPlaces: [`.${moduleName}rc.mjs`, `${moduleName}.config.mjs`], | ||
loaders: {}, | ||
} | ||
: { | ||
searchPlaces: [], | ||
loaders: { | ||
'.cjs': defaultLoadersSync['.cjs'], | ||
'.js': defaultLoadersSync['.js'], | ||
}, | ||
}; | ||
// Is the given directory set up to use ESM (ECMAScript Modules)? | ||
export const isEsmModule = (cwd: string) => { | ||
const packagePath = path.join(cwd, 'package.json'); | ||
|
||
if (!existsSync(packagePath)) { | ||
return false; | ||
} | ||
|
||
const packageJSON = readFileSync(packagePath, {encoding: 'utf-8'}); | ||
return JSON.parse(packageJSON)?.type === 'module'; | ||
}; |
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.
Switched to using this describe.each with a nested it.each to more efficiently test all the different supported configs with and without ESM configured.
The basic template is where all the configuration is self-contained within a single file, and the extends template is the recursive extends setup. The recursive extends doesn't support ESM, so I thought it'd be helpful to have the basic tests as well.