Skip to content
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

Fix/3768: Consider ESM when selecting cosmiconfig loaders #3776

Merged
merged 11 commits into from
Nov 16, 2023

Conversation

joberstein
Copy link
Contributor

@joberstein joberstein commented Nov 16, 2023

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:

  • package.json with "type": "module"
    • commitlint.config.cjs (export default)
    • commitlint.config.js (export default)
    • commitlint.config.mjs (export default)
  • package.json without "type": "module"
    • commitlint.config.cjs (module.exports)
    • commitlint.config.js (module.exports)
    • commitlint.config.mjs (export default)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

const mjsConfigFiles = isDynamicAwaitSupported()
? ['commitlint.config.mjs', '.commitlintrc.mjs']
: [];
describe.each([['basic'], ['extends']])('%s config', (template) => {
Copy link
Contributor Author

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.

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 =
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

@@ -40,22 +46,22 @@ export async function loadConfig(
`.${moduleName}rc.yml`,
`.${moduleName}rc.js`,
`.${moduleName}rc.cjs`,
`.${moduleName}rc.mjs`,
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

@escapedcat
Copy link
Member

Amazing, thanks @joberstein !

@escapedcat escapedcat merged commit 68f4f09 into conventional-changelog:master Nov 16, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment