Cli format option doesn't load modules

[byCedric]

I've been busy with some formatters for Commitlint, so far I've created two; json and junit formats.

During initial testing, I encountered some unexpected behavior related to importing these formatters in the CLI package. On line 268, in cli.js, we use require.resolve. I mistakenly assumed it would handle global/local modules and absolute/relative file paths. Unfortunately, this isn't the case. I've tried to use flags.cwd in the resolve method, but this only seems to fix locally installed modules and relative file paths.

TL;DR;
Format module resolves must be improved, I suggest resolve-from and resolve-global. What do you think? I will create a PR with the solution listed below!

Expected Behavior

$ echo 'foo: bar' | npx commitlint -o commitlint-format-junit
<?xml version="1.0" encoding="utf-8"?>
<testsuites>
  ...
</testsuites>

Current Behavior

$ echo 'foo: bar' | npx commitlint -o commitlint-format-junit
/usr/local/lib/node_modules/commitlint/node_modules/@commitlint/cli/lib/cli.js:272
	throw err;

Affected packages

• cli
• core
• prompt
• config-angular

Possible Solution

So, I took a look at @commitlint/load how it's loading the configuration. I noticed that cosmiconfig is used, so I started digging there. It turns out; it's pretty hard to resolve things like this. I would recommend using an external library to fix this. So far I found two libraries, both from Sindre Sorhus which should cover all of our needs; resolve-from and resolve-global (both MIT licensed).

With this we can do this:

function loadFormatter(config, flags) {
	const moduleName = flags.format || config.formatter;
	const moduleResolvers = [
		name => resolveFrom.silent(flags.cwd, name),
		name => resolveGlobal.silent(name),
	];

	const modulePath = moduleResolvers.find(resolve => resolve(moduleName));

	if (modulePath) {
		return require(modulePath);
	}

	throw new Error(`Cannot find format module ${moduleName}`);
}

I've briefly talked with demurgos, from node-tooling slack, about this. And I agree with him that local should be resolved before global, so the order is intentional.

Steps to Reproduce (for bugs)

1. npm install -g commitlint commitlint-format-junit
2. echo 'foo: bar' | npx commitlint -o commitlint-format-junit

Your Environment

I'm running this from a docker container, so I wouldn't pollute my own global installations 😄If you want to use the same environment, spin it up with:

$ docker run -ti --rm -v $PWD:/code -w /code bycedric/ci-node sh
# apk add --no-cache git

Executable	Version
commitlint --version	7.2.0
git --version	2.18.0
node --version	v10.11.0

[marionebl]

Good catch! Going with resolve-from for starters should be enough. I think there is some complexity in this when thinking it through

• As a user i'd expect formatters configured via cli flags to resolve from process.cwd()
• formatters configured on commitlint.config.js should be resolved from the dirname of the config file they were specified in

[byCedric]

Sorry I totally missed your comment! I will update my PR accordingly! Thanks

[byCedric]

I agree with you about the paths and that commitlint.config.js should resolve relatively from it's own folder. Although I'm not sure why you wouldn't want to support globally installed formatters.

Do you have tips or tricks on how to get the path of commitlint.config.js? Right now this is loaded with cosmiconfig in @commitlint/load, but it's only returning the config content. That means there is no info about the location of the config available. I'm not sure if I know a proper solution for this. We can perform parts of the steps in the CLI method, but that doesn't feel SOLID.

[byCedric]

How does this sound?

• @commitlint/cli resolves the format module flag using (at least) resolve-from.
• @commitlint/load resolves the module from config using the base folder provided by cosmiconfig, if possible. If it found a relative module, it replaces the config with an absolute path. Else it just keeps the original config intact.

This results in @commitlint/load doesn't return the exact value from config for formatter. Instead, it returns the absolute (resolved) path. This way you keep the separated concerns of load and cli, without doing too much extra or double work. Can that work?

[byCedric]

I've updated the pull request to allow config-file-relative modules as formatter @marionebl . The approach I used is the same I described above. I'm not sure if that's the way to go, but I don't see a lot of other options without duplicating code. If you have another option which we can explore, I would be happy to help/test/try!

Right now the code in my PR supports the following scenarios:

1. None set, defaults to @commitlint/format (which is handled by CLI require.resolve for commitlint-specific packages)
2. From config using relative file path (e.g. "formatter": "./my/local-module.js")
3. From config using absolute file path (e.g. "formatter": "/usr/.../my-module.js")
4. From config using (global) module name (e.g. "formatter": "my-global-formatter")
5. From cli flag using relative file path from cwd (e.g. --format ./local-module.js)
6. From cli flag using absolute file path from cwd (e.g. --format $(PWD)/my/module.js)
7. From cli flag using (global) module name (e.g. --format commitlint-format-junit)

I'd recon the absolute file paths are a bit weird (wouldn't recommend it either), but it's actually being resolved by require.resolve. And it also takes priority before resolveFrom and resolveGlobal.

I know you are not a fan of global modules. My ultimate perfect scenario use case is being able to use it from a pre-built docker image, without installing the project dependencies. It takes our (Bitbucket) Pipelines from an average of 1.5 - 2 min to around 10 sec for commit validation. That's why I would really love global support 😄

[marionebl]

@byCedric I added a review to your PR. Apart from a minor thing I am fine with all your points - great work! 👍

Are you be interested in maintaining commitlint? I'll be on vacation for two weeks starting on Friday 12th, so I'd introduce you to the project when I am back. What do you think?

[byCedric]

Thanks @marionebl! I'll fix it after I had something to eat! And I'm glad you like the approach too, I think I was a bit lost otherwise 😅

Yeah definitely! Like I said earlier, I like this project, and we (at Peakfijn) are feeling the benefits from using it 😬I'm happy to keep helping 😄Have fun on your vacation!