search

xojs / xo

❤️ JavaScript/TypeScript linter (ESLint wrapper) with great defaults
MIT License
7.7k stars 289 forks source link

The composition of the extension options is broken #447

Open axtgr opened 4 years ago

axtgr commented 4 years ago

Today I was trying to use XO to lint Svelte files. It wasn't working, so I dug a little. Turns out, the extension(s) option doesn't work as I expected, and I think there is more than one issue with it.

This is basically what the docs say about declaring file extensions: you can either pass one or more extension (singular) options via CLI, or specify an array as extensions (plural) in a config file. That's it.

This is what happens in reality:

  1. All options are merged into a single object, basically {...configOptions, ...cliOptions}. It means that both singular and plural aliases are allowed everywhere. https://github.com/xojs/xo/blob/2e39794d5ad4017ed2545d9a43691d4c4b0f7ade/lib/options-manager.js#L240-L244
  2. The options are normalized. The plural alias is preferred over the singular. https://github.com/xojs/xo/blob/2e39794d5ad4017ed2545d9a43691d4c4b0f7ade/lib/options-manager.js#L216
  3. The result is overridden by concatenating some default extensions with the original extensions option passed from CLI, which makes it the only working option. https://github.com/xojs/xo/blob/2e39794d5ad4017ed2545d9a43691d4c4b0f7ade/lib/options-manager.js#L246

And these are IMHO the problems of the current approach:

  1. The priority of options is weird. It's basically this: Plural CLI option > Plural config option > Singular CLI option > Singular config option I think CLI options should always override config options, which can be achieved by normalizing them separately before merging.
  2. Step 3 is probably a bug. I think it's meant to use the result of the first two steps instead of the original option. Can be easily fixed by changing the variable used.
  3. It's not very well documented, so it's not entirely clear what the intended algorithm was and if it even needs fixing.
sindresorhus commented 4 years ago

Thanks for investigating this.

And these are IMHO the problems of the current approach:

  1. Agreed. CLI options should take priority.
  2. Yup. Looks like it.
  3. You're right. It should be better documented. The options have evolved over time.
sindresorhus commented 4 years ago

Ideally, we should throw if the singular form is used in the config or plural is used in the CLI instead of just silently accepting it.