Skip to content

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

Merged
merged 11 commits into from
Nov 16, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions @commitlint/load/fixtures/basic-config/.commitlintrc
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']
8 changes: 8 additions & 0 deletions @commitlint/load/fixtures/basic-config/.commitlintrc.cjs
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'],
},
};
8 changes: 8 additions & 0 deletions @commitlint/load/fixtures/basic-config/.commitlintrc.js
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'],
},
};
8 changes: 8 additions & 0 deletions @commitlint/load/fixtures/basic-config/.commitlintrc.json
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"]
}
}
5 changes: 5 additions & 0 deletions @commitlint/load/fixtures/basic-config/.commitlintrc.yaml
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']
5 changes: 5 additions & 0 deletions @commitlint/load/fixtures/basic-config/.commitlintrc.yml
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']
8 changes: 8 additions & 0 deletions @commitlint/load/fixtures/basic-config/commitlint.config.cjs
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'],
},
};
8 changes: 8 additions & 0 deletions @commitlint/load/fixtures/basic-config/commitlint.config.js
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'],
},
};
8 changes: 8 additions & 0 deletions @commitlint/load/fixtures/basic-config/esm/.commitlintrc.js
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'],
},
};
8 changes: 8 additions & 0 deletions @commitlint/load/fixtures/basic-config/esm/.commitlintrc.mjs
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'],
},
};
3 changes: 3 additions & 0 deletions @commitlint/load/fixtures/basic-template/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"name": "load-test-js"
}
13 changes: 0 additions & 13 deletions @commitlint/load/fixtures/config/package.json

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'],
},
};
3 changes: 3 additions & 0 deletions @commitlint/load/fixtures/extends-js-template/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"name": "load-test-js"
}
117 changes: 92 additions & 25 deletions @commitlint/load/src/load.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -188,41 +188,108 @@ test('respects cwd option', async () => {
});
});

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 isExtendsTemplate = template === 'extends';

test.each(
[
const configFiles = [
'commitlint.config.cjs',
'commitlint.config.js',
'commitlint.config.mjs',
'package.json',
'.commitlintrc',
'.commitlintrc.cjs',
'.commitlintrc.js',
'.commitlintrc.json',
'.commitlintrc.mjs',
'.commitlintrc.yml',
'.commitlintrc.yaml',
...mjsConfigFiles,
].map((configFile) => [configFile])
)('recursive extends with %s', async (configFile) => {
const cwd = await gitBootstrap(`fixtures/recursive-extends-js-template`);
const configPath = path.join(__dirname, `../fixtures/config/${configFile}`);
const config = readFileSync(configPath);

writeFileSync(path.join(cwd, configFile), config);

const actual = await load({}, {cwd});

expect(actual).toMatchObject({
formatter: '@commitlint/format',
extends: ['./first-extended'],
plugins: {},
rules: {
zero: [0, 'never'],
one: [1, 'always'],
two: [2, 'never'],
},
];

const configTestCases = [
...configFiles
.filter((filename) => !filename.endsWith('.mjs'))
.map((filename) => ({filename, isEsm: false})),
...configFiles
.filter((filename) =>
['.mjs', '.js'].some((ext) => filename.endsWith(ext))
)
.map((filename) => ({filename, isEsm: true})),
];

const getConfigContents = ({
filename,
isEsm,
}): string | NodeJS.ArrayBufferView => {
if (filename === 'package.json') {
const configPath = path.join(
__dirname,
`../fixtures/${template}-config/.commitlintrc.json`
);
const commitlint = JSON.parse(
readFileSync(configPath, {encoding: 'utf-8'})
);
return JSON.stringify({commitlint});
} else {
const filePath = ['..', 'fixtures', `${template}-config`, filename];

if (isEsm) {
filePath.splice(3, 0, 'esm');
}

const configPath = path.join(__dirname, filePath.join('/'));
return readFileSync(configPath);
}
};

const esmBootstrap = (cwd: string) => {
const packageJsonPath = path.join(cwd, 'package.json');
const packageJSON = JSON.parse(
readFileSync(packageJsonPath, {encoding: 'utf-8'})
);

writeFileSync(
packageJsonPath,
JSON.stringify({
...packageJSON,
type: 'module',
})
);
};

const templateFolder = [template, isExtendsTemplate ? 'js' : '', 'template']
.filter((elem) => elem)
.join('-');

it.each(
configTestCases
// Skip ESM tests for the extends suite until resolve-extends supports ESM
.filter(({isEsm}) => template !== 'extends' || !isEsm)
// Skip ESM tests if dynamic await is not supported; Jest will crash with a seg fault error
.filter(({isEsm}) => isDynamicAwaitSupported() || !isEsm)
)('$filename, ESM: $isEsm', async ({filename, isEsm}) => {
const cwd = await gitBootstrap(`fixtures/${templateFolder}`);

if (isEsm) {
esmBootstrap(cwd);
}

writeFileSync(
path.join(cwd, filename),
getConfigContents({filename, isEsm})
);

const actual = await load({}, {cwd});

expect(actual).toMatchObject({
formatter: '@commitlint/format',
extends: isExtendsTemplate ? ['./first-extended'] : [],
plugins: {},
rules: {
zero: [0, 'never'],
one: [1, 'always'],
two: [2, 'never'],
},
});
});
});

Expand Down
46 changes: 24 additions & 22 deletions @commitlint/load/src/utils/load-config.ts
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 {
Expand All @@ -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 =
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.

isDynamicAwaitSupported() || isEsmModule(cwd)
? defaultLoaders
: defaultLoadersSync;

const explorer = cosmiconfig(moduleName, {
searchPlaces: [
Expand All @@ -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.

`${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'],
},
});

Expand All @@ -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 = () => {
Expand All @@ -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';
};
4 changes: 2 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -142,12 +142,12 @@ Check the [husky documentation](https://typicode.github.io/husky/#/?id=manual) o
- `.commitlintrc.yml`
- `.commitlintrc.js`
- `.commitlintrc.cjs`
- `.commitlintrc.mjs` (Node >= v20.8.0)
- `.commitlintrc.mjs`
- `.commitlintrc.ts`
- `.commitlintrc.cts`
- `commitlint.config.js`
- `commitlint.config.cjs`
- `commitlint.config.mjs` (Node >= v20.8.0)
- `commitlint.config.mjs`
- `commitlint.config.ts`
- `commitlint.config.cts`
- `commitlint` field in `package.json`
Expand Down