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

Bump cosmiconfig version and conditionally support mjs config #3747

Merged
merged 15 commits into from
Nov 10, 2023
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
76 changes: 12 additions & 64 deletions .github/workflows/CI.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,77 +8,25 @@ on:
types: [opened, synchronize]

jobs:
v18:
runs-on: ubuntu-22.04
container:
image: 'ubuntu:22.04'
build:
strategy:
matrix:
Copy link
Contributor Author

@joberstein joberstein Nov 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplified this with the build matrix so they all run the same steps, but @escapedcat do I need to keep the original checks around temporarily?

I did this to take advantage of the fact that the setup node action pulls down node 20.9.0, whereas the original job pulled down only 20.5.x and I didn't see a way to configure it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think pulling latest v20 is fine.
Thanks for introducing the matrix. Simplifies things in the future.
I might need to adjust the required checks in the settings. Will check.

os: [ubuntu-22.04, windows-2022]
node: [18, 20]
runs-on: ${{ matrix.os }}
steps:
- name: Install required dependencies
run: |
apt update
apt install --yes sudo
sudo apt install --yes git
sudo apt install --yes curl
curl --location https://deb.nodesource.com/setup_18.x | sudo --preserve-env bash -
sudo DEBIAN_FRONTEND=noninteractive apt install --yes nodejs
- uses: actions/checkout@v4
# workaround for https://github.com/actions/runner/issues/2033
- name: ownership workaround
run: git config --global --add safe.directory '*'
- name: Install yarn
run: |
npm install --global yarn
node --version
yarn global add yarn@latest
- name: Install dependencies
run: yarn install --ignore-engines --frozen-lockfile
- name: Build packages
run: yarn build
- name: Test
run: yarn test-ci

v20:
runs-on: ubuntu-22.04
container:
image: 'ubuntu:22.04'
steps:
- name: Install required dependencies
run: |
apt update
apt install --yes sudo
sudo apt install --yes git
sudo apt install --yes curl
curl --location https://deb.nodesource.com/setup_20.x | sudo --preserve-env bash -
sudo DEBIAN_FRONTEND=noninteractive apt install --yes nodejs
- uses: actions/checkout@v4
# workaround for https://github.com/actions/runner/issues/2033
- name: ownership workaround
run: git config --global --add safe.directory '*'
- name: Install yarn
run: |
npm install --global yarn
node --version
yarn global add yarn@latest
- name: Install dependencies
run: yarn install --ignore-engines --frozen-lockfile
- name: Build packages
run: yarn build
- name: Test
run: yarn test-ci

windows:
runs-on: windows-2022
steps:
- uses: actions/checkout@v4
- uses: actions/setup-node@v3
with:
max_attempts: 3
- name: Update yarn
run: |
node --version
yarn global add yarn@latest
node-version: ${{ matrix.node }}
cache: yarn

- name: Install dependencies
run: yarn install --ignore-engines --frozen-lockfile

- name: Build packages
run: yarn build

- name: Test
run: yarn test-ci
4 changes: 4 additions & 0 deletions @commitlint/load/fixtures/config/.commitlintrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
extends:
- './first-extended'
rules:
zero: [0, 'never']
6 changes: 6 additions & 0 deletions @commitlint/load/fixtures/config/.commitlintrc.cjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
module.exports = {
extends: ['./first-extended'],
rules: {
zero: [0, 'never'],
},
};
6 changes: 6 additions & 0 deletions @commitlint/load/fixtures/config/.commitlintrc.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
module.exports = {
extends: ['./first-extended'],
rules: {
zero: [0, 'never'],
},
};
6 changes: 6 additions & 0 deletions @commitlint/load/fixtures/config/.commitlintrc.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
{
"extends": ["./first-extended"],
"rules": {
"zero": [0, "never"]
}
}
6 changes: 6 additions & 0 deletions @commitlint/load/fixtures/config/.commitlintrc.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
export default {
extends: ['./first-extended'],
rules: {
zero: [0, 'never'],
},
};
4 changes: 4 additions & 0 deletions @commitlint/load/fixtures/config/.commitlintrc.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
extends:
- './first-extended'
rules:
zero: [0, 'never']
4 changes: 4 additions & 0 deletions @commitlint/load/fixtures/config/.commitlintrc.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
extends:
- './first-extended'
rules:
zero: [0, 'never']
6 changes: 6 additions & 0 deletions @commitlint/load/fixtures/config/commitlint.config.cjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
module.exports = {
extends: ['./first-extended'],
rules: {
zero: [0, 'never'],
},
};
6 changes: 6 additions & 0 deletions @commitlint/load/fixtures/config/commitlint.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
module.exports = {
extends: ['./first-extended'],
rules: {
zero: [0, 'never'],
},
};
6 changes: 6 additions & 0 deletions @commitlint/load/fixtures/config/commitlint.config.mjs
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
export default {
extends: ['./first-extended'],
rules: {
zero: [0, 'never'],
},
};
13 changes: 13 additions & 0 deletions @commitlint/load/fixtures/config/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
{
"commitlint": {
"extends": [
"./first-extended"
],
"rules": {
"zero": [
0,
"never"
]
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
module.exports = {
extends: ['./second-extended'],
rules: {
one: [1, 'always'],
},
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
module.exports = {
rules: {
two: [2, 'never'],
},
};
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import type {UserConfig} from './types';

const Configuration: UserConfig = {
extends: ['./first-extended'],
extends: ['./first-extended/index.ts'],
escapedcat marked this conversation as resolved.
Show resolved Hide resolved
rules: {
zero: [0, 'never', 'zero']
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import type {UserConfig} from '../types';
module.exports = {
extends: ['./second-extended'],
extends: ['./second-extended/index.ts'],
rules: {
one: [1, 'never', 'one']
}
Expand Down
46 changes: 42 additions & 4 deletions @commitlint/load/src/load.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,12 @@ jest.mock('@scope/commitlint-plugin-example', () => scopedPlugin, {
});

import path from 'path';
import {readFileSync, writeFileSync} from 'fs';
import resolveFrom from 'resolve-from';
import {fix, git, npm} from '@commitlint/test';

import load from './load';
import {isDynamicAwaitSupported} from './utils/load-config';

const fixBootstrap = (name: string) => fix.bootstrap(name, __dirname);
const gitBootstrap = (name: string) => git.bootstrap(name, __dirname);
Expand Down Expand Up @@ -186,6 +188,44 @@ test('respects cwd option', async () => {
});
});

const mjsConfigFiles = isDynamicAwaitSupported()
Copy link
Contributor Author

@joberstein joberstein Nov 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New set of tests for each config type. I removed the older tests and their fixture data, but verified they were still passing in an older commit.

? ['commitlint.config.mjs', '.commitlintrc.mjs']
: [];

test.each(
[
'commitlint.config.cjs',
'commitlint.config.js',
'package.json',
'.commitlintrc.cjs',
'.commitlintrc.js',
'.commitlintrc.json',
'.commitlintrc.yml',
'.commitlintrc.yaml',
'.commitlintrc',
...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);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test.each writes a config file from fixtures/config to the template directory and then loads the found config from the current working directory. Everything except ts files should be supported by the template directory.


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

test('recursive extends', async () => {
const cwd = await gitBootstrap('fixtures/recursive-extends');
const actual = await load({}, {cwd});
Expand Down Expand Up @@ -266,15 +306,13 @@ test('recursive extends with package.json file', async () => {
});
});

// fails since a jest update: https://github.com/conventional-changelog/commitlint/pull/3362
// eslint-disable-next-line jest/no-disabled-tests
test.skip('recursive extends with ts file', async () => {
test('recursive extends with ts file', async () => {
const cwd = await gitBootstrap('fixtures/recursive-extends-ts');
const actual = await load({}, {cwd});

expect(actual).toMatchObject({
formatter: '@commitlint/format',
extends: ['./first-extended'],
extends: ['./first-extended/index.ts'],
plugins: {},
rules: {
zero: [0, 'never', 'zero'],
Expand Down
45 changes: 42 additions & 3 deletions @commitlint/load/src/utils/load-config.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
import {cosmiconfig, type Loader} from 'cosmiconfig';
import {
cosmiconfig,
defaultLoadersSync,
Options,
type Loader,
} from 'cosmiconfig';
import {TypeScriptLoader} from 'cosmiconfig-typescript-loader';
import path from 'path';

Expand All @@ -8,12 +13,12 @@ export interface LoadConfigResult {
isEmpty?: boolean;
}

const moduleName = 'commitlint';

export async function loadConfig(
cwd: string,
configPath?: string
): Promise<LoadConfigResult | null> {
const moduleName = 'commitlint';

let tsLoaderInstance: Loader | undefined;
const tsLoader: Loader = (...args) => {
if (!tsLoaderInstance) {
Expand All @@ -22,6 +27,8 @@ export async function loadConfig(
return tsLoaderInstance(...args);
};

const {searchPlaces, loaders} = getDynamicAwaitConfig();

const explorer = cosmiconfig(moduleName, {
searchPlaces: [
// cosmiconfig overrides default searchPlaces if any new search place is added (For e.g. `*.ts` files),
Expand All @@ -41,10 +48,14 @@ export async function loadConfig(
`.${moduleName}rc.cts`,
`${moduleName}.config.ts`,
`${moduleName}.config.cts`,

...(searchPlaces || []),
],
loaders: {
'.ts': tsLoader,
'.cts': tsLoader,

...(loaders || {}),
},
});

Expand All @@ -59,3 +70,31 @@ export async function loadConfig(

return null;
}

// See the following issues for more context:
// - Issue: https://github.com/nodejs/node/issues/40058
// - Resolution: https://github.com/nodejs/node/pull/48510 (Node v20.8.0)
export const isDynamicAwaitSupported = () => {
const [major, minor] = process.version
.replace('v', '')
.split('.')
.map((val) => parseInt(val));

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'],
},
};
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@
"publish": "lerna publish --conventional-commits",
"reinstall": "yarn clean && yarn install",
"start": "yarn watch",
"test": "cross-env HOME=$PWD jest",
"test-ci": "cross-env HOME=$PWD jest --runInBand",
"test": "cross-env HOME=$PWD NODE_OPTIONS=--experimental-vm-modules jest",
"test-ci": "yarn test --runInBand",
"postinstall": "yarn husky install"
},
"commitlint": {
Expand Down