Fix/3768: Consider ESM when selecting cosmiconfig loaders #3776
Conversation
…pendency for @commitlint/load
| const mjsConfigFiles = isDynamicAwaitSupported() | ||
| ? ['commitlint.config.mjs', '.commitlintrc.mjs'] | ||
| : []; | ||
| describe.each([['basic'], ['extends']])('%s config', (template) => { |
There was a problem hiding this comment.
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.
| 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.
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.
| @@ -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.
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.
|
Amazing, thanks @joberstein ! |
Description
Adjusting the logic for the original issue to consider ESM modules and adds additional automated tests.
This change uses the async loaders (dynamic import) for js/cjs config files if the module is ESM or if the node version is >= 20.8.0 (memory leak with dynamic import was resolved). Otherwise, it uses sync loaders (require) for js/cjs config files (original behavior prior to upgrading cosmiconfig).
This change also allows mjs config files to be used for node 18 applications as well.
Motivation and Context
Resolves: #3768
Related to: #3747
Closes: #3773 (duplicate)
Usage examples
N/A
How Has This Been Tested?
Manually tested by linking the current branch locally to a test repo, and I added extra automated tests.
Manual tests I completed successfully for Node v18:
Types of changes
Checklist: