Bump cosmiconfig version and conditionally support mjs config #3747

joberstein · 2023-11-08T04:40:46Z

Bump cosmiconfig version and conditionally support mjs config. Also simplify CI test matrix and add more tests for the different config files.

Draft until I do manual testing + need to update docs too.

Description

This PR:

Bumps the cosmiconfig version to latest (v8.3.6)
Adds additional tests for each variation of acceptable config files (.commitlintrc.xxx and commitling.config.xxx)
Conditionally supports loading mjs configuration files (node engine must be >= 20.8.0). Otherwise, mjs configuration files are not supported.
Conditionally supports async loaders for js/cjs configuration files (node engine must be >= 20.8.0). Otherwise, js/cjs configuration files are loaded using the synchronous loaders (underlying implementation is the same).

Motivation and Context

Replacement PR for automated PR: #3654

Also resolves: #3611

Usage examples

With node >= 20.8.0, a user can now specify commitlint config in '.commitlintrc.mjs' or 'commitlint.config.mjs'.

How Has This Been Tested?

Unit/Integration tests
Manual testing - tested by creating a new empty repo locally with a commitlint config for mjs (separately with .commitlintrc.mjs and commitlint.config.mjs). I locally installed @commitlint/cli with a locally installed @commitlint/load. With node 20.9.0, I verified that the mjs config was loaded successfully. With node v18.18.2, I verified that the mjs config was not parsed (current behavior). I did not need the "type": "module" property in my package.json, so I will only modify the README to add the new file types.

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.

…pendency for @commitlint/load

joberstein · 2023-11-08T04:49:42Z

.github/workflows/CI.yml

-      image: 'ubuntu:22.04'
+  build:
+    strategy:
+      matrix:


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.

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.

joberstein · 2023-11-08T04:51:22Z

@commitlint/load/src/load.test.ts

@@ -186,6 +188,44 @@ test('respects cwd option', async () => {
 	});
 });

+const mjsConfigFiles = isDynamicAwaitSupported()


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.

joberstein · 2023-11-08T05:12:19Z

@commitlint/load/src/load.test.ts

+	const configPath = path.join(__dirname, `../fixtures/config/${configFile}`);
+	const config = readFileSync(configPath);
+
+	writeFileSync(path.join(cwd, configFile), config);


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.

@commitlint/load/fixtures/recursive-extends-ts/commitlint.config.ts

escapedcat · 2023-11-08T05:17:52Z

.github/workflows/CI.yml

-      image: 'ubuntu:22.04'
+  build:
+    strategy:
+      matrix:


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.

escapedcat · 2023-11-08T05:20:40Z

If you don't mind you could introduce the matrix checks in a separate PR. We could merge that first, adjust required checks and merge this after.

escapedcat · 2023-11-08T05:21:03Z

Thanks a lot for this @joberstein ! ❤️

joberstein · 2023-11-08T05:35:07Z

If you don't mind you could introduce the matrix checks in a separate PR. We could merge that first, adjust required checks and merge this after.

Sure, I'll make a separate one for that first.

joberstein · 2023-11-09T04:28:00Z

If you don't mind you could introduce the matrix checks in a separate PR. We could merge that first, adjust required checks and merge this after.

Link to separate PR is: #3748

escapedcat · 2023-11-09T04:37:57Z

Merged the matrix PR, wanna rebase this?

joberstein · 2023-11-10T05:00:01Z

@escapedcat I finished manual testing (added a description in the first comment) and updated the docs.

escapedcat · 2023-11-10T05:08:05Z

Impressive, thanks! Will merge and release

B4nan · 2023-11-10T11:02:01Z

Looks like this breaks our CI:

https://github.com/mikro-orm/mikro-orm/actions/runs/6823592237/job/18558301519

I can't reproduce this locally now.

joberstein · 2023-11-10T16:08:37Z

@B4nan I cloned the master branch of your repo and reproduced with a test commit. I noticed that cosmiconfig is at 8.0.0 in its package.json in node_modules though, which is a version from before they added support for mjs files. Going to look into why this version is being installed instead of ^8.0.0 (v8.3.6).

B4nan · 2023-11-10T16:15:25Z

Oh I see, tried to deduplicate the lock file, lets see if that helps

mikro-orm/mikro-orm@f95b57b?w=1

joberstein · 2023-11-10T16:27:39Z

Oh I see, tried to deduplicate the lock file, lets see if that helps

mikro-orm/mikro-orm@f95b57b?w=1

Apparently it's an issue with yarn (maybe npm also?) in that it won't re-install transitive dependencies to meet semver requirements. So even though commitlint/load is using ^8.0.0, since the consuming repo already has installed it as 8.0.0, it won't update unless the commitlint/load cosmiconfig version changes to something compatible (in this case ^8.2.0). I'd like to use an exact version though, so it won't be assumed to update for dependent projects.

I think what you have should work as well though.

joberstein added 10 commits October 23, 2023 22:53

feat(load): use cosmiconfig-typescript-loader v5 to remove ts-node de…

2d567b3

…pendency for @commitlint/load

Merge branch 'feat/bump-cosmiconfig-typescript-loader-2'

6dcec0c

Merge branch 'conventional-changelog:master' into master

b9cd734

Merge branch 'conventional-changelog:master' into master

145ca9d

feat: conditionally support loading mjs config based on the node version

df34d80

Merge branch 'conventional-changelog:master' into master

b8c0c00

Merge remote-tracking branch 'refs/remotes/origin/master'

1fad45a

chore: update 'test-ci' command

9e04c19

chore: add yarn cache to ci

ff2541f

chore: swap node and checkout order. Remove ownership workaround

b233fbd

joberstein commented Nov 8, 2023

View reviewed changes

chore(load): remove duplicate tests

b160218

joberstein commented Nov 8, 2023

View reviewed changes

escapedcat reviewed Nov 8, 2023

View reviewed changes

joberstein added 4 commits November 8, 2023 23:54

Merge branch 'conventional-changelog:master' into master

2a545c5

Merge remote-tracking branch 'refs/remotes/origin/master'

919e056

Merge branch 'master' into feat/3611

88416b9

docs: add mjs config files to README

fdecba0

joberstein marked this pull request as ready for review November 10, 2023 04:58

escapedcat merged commit a2b65fc into conventional-changelog:master Nov 10, 2023
4 checks passed

commit-lint bot mentioned this pull request Nov 10, 2023

[pull] master from conventional-changelog:master ammar-knowledge/commitlint#30

Merged

This was referenced Nov 10, 2023

Pin cosmiconfig to specific version #3755

Merged

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump cosmiconfig version and conditionally support mjs config #3747

Bump cosmiconfig version and conditionally support mjs config #3747

joberstein commented Nov 8, 2023 •

edited

Loading

joberstein Nov 8, 2023 •

edited

Loading

escapedcat Nov 8, 2023

joberstein Nov 8, 2023 •

edited

Loading

joberstein Nov 8, 2023

escapedcat Nov 8, 2023

escapedcat commented Nov 8, 2023

escapedcat commented Nov 8, 2023

joberstein commented Nov 8, 2023

joberstein commented Nov 9, 2023

escapedcat commented Nov 9, 2023

joberstein commented Nov 10, 2023

escapedcat commented Nov 10, 2023

B4nan commented Nov 10, 2023

joberstein commented Nov 10, 2023 •

edited

Loading

B4nan commented Nov 10, 2023

joberstein commented Nov 10, 2023

Bump cosmiconfig version and conditionally support mjs config #3747

Bump cosmiconfig version and conditionally support mjs config #3747

Conversation

joberstein commented Nov 8, 2023 • edited Loading

Description

Motivation and Context

Usage examples

How Has This Been Tested?

Types of changes

Checklist:

joberstein Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

escapedcat Nov 8, 2023

Choose a reason for hiding this comment

joberstein Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

joberstein Nov 8, 2023

Choose a reason for hiding this comment

escapedcat Nov 8, 2023

Choose a reason for hiding this comment

escapedcat commented Nov 8, 2023

escapedcat commented Nov 8, 2023

joberstein commented Nov 8, 2023

joberstein commented Nov 9, 2023

escapedcat commented Nov 9, 2023

joberstein commented Nov 10, 2023

escapedcat commented Nov 10, 2023

B4nan commented Nov 10, 2023

joberstein commented Nov 10, 2023 • edited Loading

B4nan commented Nov 10, 2023

joberstein commented Nov 10, 2023

joberstein commented Nov 8, 2023 •

edited

Loading

joberstein Nov 8, 2023 •

edited

Loading

joberstein Nov 8, 2023 •

edited

Loading

joberstein commented Nov 10, 2023 •

edited

Loading