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

Add support to .cjs (common) and .mjs (module) config file #2742

Closed
1 task done
gustavonobreza opened this issue Aug 31, 2021 · 11 comments
Closed
1 task done

Add support to .cjs (common) and .mjs (module) config file #2742

gustavonobreza opened this issue Aug 31, 2021 · 11 comments
Labels

Comments

@gustavonobreza
Copy link

gustavonobreza commented Aug 31, 2021

Expected Behavior

If config file is commitlint.config.mjs or commitlint.config.cjs is expected that works

Current Behavior

Is ignored

Affected packages

  • load

Possible Solution

in commitlint/@commitlint/load/src/utils/load-config.ts at loadConfig:

// ...
	const moduleName = 'commitlint';
	const explorer = cosmiconfig(moduleName, {
		searchPlaces: [
			'package.json',
			`.${moduleName}rc`,
			`.${moduleName}rc.json`,
			`.${moduleName}rc.yaml`,
			`.${moduleName}rc.yml`,
			`.${moduleName}rc.ts`,
			`.${moduleName}rc.js`,
			`${moduleName}.config.ts`,
			`${moduleName}.config.js`,
                       // add it here
                       	`${moduleName}.config.cjs`,
			`${moduleName}.config.mjs`,
		],
		loaders: {
			'.ts': TypeScriptLoader,
		},
	};
// ...
```
@escapedcat
Copy link
Member

@k2snowman69
Copy link

Just out of curiousity why does commitlint override the searchPlaces instead of just using the ones provided by cosmiconfig? These all seem like the built in configs other than the typescript ones which can be configured with a custom loader like you are already.

Just curious if there was something I'm missing here and can learn from this code.

@escapedcat
Copy link
Member

@k2snowman69 no worries, good point!
@songhn233 what do you think? Could we just remove this and go with defaults?

@songhn233
Copy link
Contributor

@k2snowman69 But the custom loaders are just an object map for items in searchPlaces reference to cosmiconfig, custom loaders will be merged with the defaults but the search places should override.

Also I think it would be beneficial to specify this searchPlaces configuration for the display. If you want to change the loading behavior of the configuration but don't know cosmiconfig, you may have to spend some time finding the file and forking it to modify, but adding yml or json will also make it easier to search.

@k2snowman69
Copy link

k2snowman69 commented Sep 28, 2021

But the custom loaders are just an object map for items in searchPlaces reference to cosmiconfig, custom loaders will be merged with the defaults but the search places should override.

I'm fairly sure that if you add a custom loader with an extension, cosmiconfig will add that extension to the list of extensions to search. Here's the unit test for it.

Also I think it would be beneficial to specify this searchPlaces configuration for the display

This is the part I disagree with. I was debugging for near about 6 hours trying to figure out why even though commitlint used cosmiconfig it didn't use it's available configurations. I had to dig into commitlint's code base to figure out you were providing custom searchPlaces which overrides cosmiconfigs defaults.

I agree from a documentation standpoint in the configuration section stating "we support several file extensions like cosmiconfig.config.ts, .cosmiconfigrc.js, cosmiconfigrc.json, and others via cosmiconfig". This way you're still documenting some more used file names but as cosmiconfig changes it's support you aren't having to make a code change along side them every time.

@escapedcat
Copy link
Member

This is the part I disagree with. I was debugging for near about 6 hours trying to figure out why even though commitlint used cosmiconfig it didn't use it's available configurations. I had to dig into commitlint's code base to figure out you were providing custom searchPlaces which overrides cosmiconfigs defaults.

I agree from a documentation standpoint in the configuration section stating "we support several file extensions like cosmiconfig.config.ts, .cosmiconfigrc.js, cosmiconfigrc.json, and others via cosmiconfig". This way you're still documenting some more used file names but as cosmiconfig changes it's support you aren't having to make a code change along side them every time.

@k2snowman69 I understand this. If the searchplaces-defaults can be merged automatically with the extra .ts support @songhn233 wants to provide we should do this. Relying and extending defaults is best in this situation.
The docs can be adjusted as you mentioned as well.

This could be a done in a separated PR. Unless @gustavonobreza wants to add this as well.

@songhn233
Copy link
Contributor

This could be a done in a separated PR. Unless @gustavonobreza wants to add this as well.

I agree to change searchPlaces to the default configuration.

But the mjs support mentioned in this issue may have to wait for upstream branch. Reference to cosmiconfig/cosmiconfig#224 and cosmiconfig/cosmiconfig#251.

This change for searchPlaces can be merged with #2779, as there is no actual feature change.

I will later on open a pr that solves both problems. Thanks @k2snowman69 for the suggestion.

@cherryblossom000
Copy link
Contributor

v13.2.0 was technically a breaking change — in v13.1.0 (before #2698), commitlint.config.cjs worked as a config file without having to specify -g commitlint.config.cjs. Like @k2snowman69, I had to debug and look at the code to discover that commitlint.config.cjs is no longer being searched for.

It was quite confusing for me who just ran pnpm recursive upgrade and got this:

⧗   input: chore: upgrade deps
✖   Please add rules to your `commitlint.config.js`
    - Getting started guide: https://git.io/fhHij
    - Example config: https://git.io/fhHip [empty-rules]

✖   found 1 problems, 0 warnings
ⓘ   Get help: https://github.com/conventional-changelog/commitlint/#what-is-commitlint

@songhn233
Copy link
Contributor

I'm fairly sure that if you add a custom loader with an extension, cosmiconfig will add that extension to the list of extensions to search. Here's the unit test for it.

@k2snowman69 I just tried to remove the searchplace config but found that the ts-loader was not being processed correctly.

const explicitPath = configPath ? path.resolve(cwd, configPath) : undefined;
const explore = explicitPath ? explorer.load : explorer.search;
const searchPath = explicitPath ? explicitPath : cwd;
const local = await explore(searchPath);

This unit test just coverages Explorer.load function, but the Explorer.search in the actual scenario is still done through the searchplace, see this reference. The tests for directories also customizes the loader by overriding.

So in order to be able to recognize the .ts file it is still necessary to modify the searchplace object, and it seems that cosmiconfig does not export the default configuration, so it is only possible to keep such an override operation.

Please correct me if I'm wrong, maybe I'm missing something in my review of the code.

@songhn233
Copy link
Contributor

v13.2.0 was technically a breaking change — in v13.1.0 (before #2698), commitlint.config.cjs worked as a config file without having to specify -g commitlint.config.cjs. Like @k2snowman69, I had to debug and look at the code to discover that commitlint.config.cjs is no longer being searched for.

It was my mistake. In the pr at the time we discussed about the support of cjs.

My opinion is that cosmiconfig currently does not support esm. The distinction between cjs and mjs will only be necessary in the future when mjs is supported by default.

However, in fact when "type": "module" is declared in package.json, it is necessary to use cjs in order to support module.export syntax, and there is also a default configuration for cjs before, which will cause potential breaking changes if modified.

In case this happens in the future, for example, cosmiconfig supports mjs but now the searchplace overwrites the default configuration, this should merge .ts or other formats to the default, but now cosmiconfig does not export the default searchplace. So it may be difficult to sync.

@kosmita427

This comment has been minimized.

make-github-pseudonymous-again added a commit to functional-data-structure/finger-tree that referenced this issue Oct 14, 2021
make-github-pseudonymous-again added a commit to functional-data-structure/finger-tree that referenced this issue Oct 15, 2021
make-github-pseudonymous-again added a commit to functional-data-structure/finger-tree that referenced this issue Oct 15, 2021
make-github-pseudonymous-again added a commit to functional-data-structure/finger-tree that referenced this issue Nov 29, 2021
make-github-pseudonymous-again added a commit to functional-data-structure/finger-tree that referenced this issue Nov 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

6 participants