search

eslint / eslint

Find and fix problems in your JavaScript code.
https://eslint.org
MIT License
25.09k stars 4.54k forks source link

Proposal: CLIEngine options in package.json #9484

Closed not-an-aardvark closed 7 years ago

not-an-aardvark commented 7 years ago

Problem

As of ESLint 4.9.0, there are several configuration options that we don't want to allow in config files. For example, we don't allow --rulesdir in config files (https://github.com/eslint/eslint/issues/8769) because the option is project-specific and it wouldn't make sense for shareable configs to use it. We also don't allow --report-unused-disable-directives in a config file because we don't want shareable configs to use the option, due to semver-related issues described in https://github.com/eslint/eslint/issues/9382. Options like --ext wouldn't work in config files because they need to be resolved before the list of files to lint is processed.

As an alternative to config files, users currently configure these behaviors as command-line flags. Unfortunately, specifying options with command-line flags is not ideal for user experience. The main issue is that other tools which run ESLint (such as editor integrations) don't have access to these command-line flags, so they typically require manual configuration by each user in order to work correctly when a project uses CLI flags. This creates a poor experience for users and discourages the use of project-specific rules, which is something I think we should be encouraging.

Existing workarounds

Existing workarounds to this problem include:

  1. Using plugins instead of using --rulesdir. Unfortunately, as discussed in https://github.com/eslint/eslint/issues/8769, this solution is often inconvenient for project-specific rules. Plugins are currently required to be in node_modules/, and it's generally useful to develop custom rules in the same repo as the code that they lint.
  2. Using "loopback" plugins like eslint-plugin-rulesdir, eslint-plugin-local, eslint-plugin-self, and eslint-plugin-local-rules for project-specific linting rules. These plugins reside in node_modules and export rule files from the project that loads the plugin. This allows rules to be loaded from the project repo. Unfortunately, these modules are also inconvenient to use (e.g. they generally require that a project create a new file in the root directory), and they also have footguns (they don't work when ESLint is installed globally).

I don't know of any workarounds for other command-line flags, such as --report-unused-disable-directives and --ext.

Proposed solution

I think we should modify CLIEngine to load default options from a package.json file in the CWD, if present.

Details

  1. When a new instance of CLIEngine is created, it looks for a eslintCliOptions property in a package.json file in the CWD.
  2. If no package.json file is found, or the package.json file has no eslintCliOptions property, the behavior is the same as the current behavior.
  3. When an eslintCliOptions property is found, the values of that object are used as defaults for CLIEngine. For example, cliEngineOptions could contain "reportUnusedDisableDirectives": true, "rulePaths": ["tools/internal-rules"] to reflect the --report-unused-disable-directives and --rulesdir CLI options.
  4. Those values are then used as defaults for that instance of CLIEngine, and are merged in with the other options passed to the constructor. Options in the constructor take precedence over options in package.json, so anything in eslintCliOptions could be overridden by actually using a command-line flag.

Advantages

  1. This would allow projects to declare project-wide options for ESLint (such as reportUnusedDisableDirectives and rulePaths) in a manner that would allow the options to automatically get used by external integrations.
  2. The options would not be stored in a config file, avoiding confusion about whether the options would be inheritable in shared configs.
  3. This would not require us to design new options such as topLevelOptions values in a config file, along the lines of what was discussed in https://github.com/eslint/eslint/issues/2715#issuecomment-305989282.

Disadvantages

I've listed these disadvantages in order of decreasing significance, in my opinion. I consider the first items on the list to be the most significant disadvantages.

  1. This would increase the complexity of ESLint's option resolution behavior, which could lead to increased confusion if a user is wondering why ESLint is doing a particular thing.

  2. Using the fix option for CLIEngine could be very confusing for users, because it is different from the --fix command-line flag. The fix option does not call CLIEngine.outputFixes, so it would effectively be the same as the --fix-dry-run command-line flag. In other words, ESLint would not report most autofixable errors, but the autofixes would not actually get written to the filesystem.

    Potential solutions for this problem include:

    • Doing nothing, and leaving a warning in the docs about using fix in eslintCliOptions.
    • Adding a special case for fix when used in eslintCliOptions, such as outputting a warning or disallowing the property entirely.

  3. Translating command-line options names into CLIEngine option names isn't always intuitive. For example, the --rulesdir command-line flag translates to rulePaths in CLIEngine. This could make CLIEngine configuration confusing for users, because they would need to read the CLIEngine docs when they didn't need to do so previously. Additionally, some CLI flags (e.g. --version) don't have an equivalent in CLIEngine at all. This problem could partially be resolved by doing type-checking at runtime, and disallowing unknown properties.

  4. The rules option for CLIEngine would be redundant with simply specifying rules in a config file. To address this, we could either (a) have the option work anyway for consistency, or (b) add a special case for the rules option (probably to disallow it in eslintCliOptions).

  5. This would slightly hurt performance because it would add a filesystem read whenever a CLIEngine instance is created. (A CLIEngine instance usually reads a lot of files during its lifetime anyway, so I don't think this would be a very big problem.)

  6. This would result in ESLint using three different keys in package.json (eslintConfig, eslintIgnore, and eslintCliOptions), which could be mildly annoying to some users.

not-an-aardvark commented 7 years ago

Closing because the TSC didn't seem to be in favor of this solution when it was discussed along with https://github.com/eslint/eslint/issues/9382 in Thursday's meeting.