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

Validate options passed to CLIEngine API #10272

Closed
edmorley opened this issue Apr 26, 2018 · 8 comments
Closed

Validate options passed to CLIEngine API #10272

edmorley opened this issue Apr 26, 2018 · 8 comments
Assignees
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion breaking This change is backwards-incompatible cli Relates to ESLint's command-line interface enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion

Comments

@edmorley
Copy link
Contributor

Tell us about your environment

  • ESLint Version: 4.19.1
  • Node Version: 9.11.1
  • npm Version: N/A (yarn)

What parser (default, Babel-ESLint, etc.) are you using?
babel-eslint

Please show your full configuration:

See below.

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

Used CLIEngine like so:

const { CLIEngine } = require('eslint');

const cli = new CLIEngine({
  cwd: "...",
  root: true,
  useEslintrc: false,
  failOnError: true,
  extensions: ["js", "jsx"],
  baseConfig: { extends: ["airbnb"] },
  plugins: ["babel", "react"],
  envs: ["es6", "browser", "commonjs"],
  parser: "babel-eslint",
  parserOptions: {
    ecmaVersion: 2017,
    sourceType: "module",
    ecmaFeatures: {
      objectLiteralDuplicateProperties: false,
      generators: true,
      impliedStrict: true
    }
  },
  settings: {
    react: {
      pragma: "h"
    }
  }
});

const report = cli.executeOnFiles(['src/']);

What did you expect to happen?

For the invalid CLIEngine option settings to trigger a schema validation error, like happens which an invalid option is found in an .eslintrc.js when using the CLI:

Error: ESLint configuration in ...\.eslintrc.js is invalid:
        - Unexpected top-level property "thisOptionIsNotValid".

What actually happened? Please include the actual, raw output from ESLint.

The CLIEngine command silently ignores the invalid option, causing confusion - since I instead thought eslint-plugin-react was at fault.

If there was a schema validation warning, I would have known sooner to search for how to pass options like settings when using CLIEngine (ie using baseConfig instead). It seems that this has tripped up a few other people too - eg #7247, #4402 (and I'm presuming more for other differences between the CLI and CLIEngine).

If it's too expensive/unwanted to add a schema validation check to CLIEngine (eg because the CLI does its own, and so pointless to do a second, if the CLI uses CLIEngine too) - then perhaps there can be a second schema validation method (for the CLIEngine options instead) that consumers of the API can call first if desired?

Many thanks :-)

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Apr 26, 2018
@platinumazure
Copy link
Member

I think this could be a good idea, although it's a breaking change.

@platinumazure platinumazure added enhancement This change enhances an existing feature of ESLint cli Relates to ESLint's command-line interface breaking This change is backwards-incompatible evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Apr 26, 2018
@edmorley
Copy link
Contributor Author

edmorley commented Oct 19, 2018

I think this could be a good idea, although it's a breaking change.

Would adding initially as just a warning be considered breaking?

edmorley added a commit to edmorley/eslint that referenced this issue Oct 19, 2018
This aims to prevent some of the common pitfalls/points of confusion
when configuring `CLIEngine`, such as:
* it not being clear that `baseConfig` supports all of the options
  defined in the `.eslintrc.*` schema, and so can be used for options
  that are not supported as top-level `CLIEngine` arguments such as
  `extends` and `settings.
* some of the CLIEngine options being named the same or almost the
  same as their `.eslintrc.*` equivalents, but being a different
  data type.
* it not being clear that CLIEngine silently ignores invalid options
  (which will hopefully be fixed at some point by eslint#10272).

Refs:
eslint#4402
eslint#5179
eslint#6605
eslint#7247
eslint#7967
eslint#10272
webpack-contrib/eslint-loader#173
webpack-contrib/eslint-loader#252
neutrinojs/neutrino#1181
edmorley added a commit to edmorley/eslint that referenced this issue Nov 24, 2018
This aims to prevent some of the common pitfalls/points of confusion
when configuring `CLIEngine`, such as:
* it not being clear that `baseConfig` supports all of the options
  defined in the `.eslintrc.*` schema, and so can be used for options
  that are not supported as top-level `CLIEngine` arguments such as
  `extends` and `settings.
* some of the CLIEngine options being named the same or almost the
  same as their `.eslintrc.*` equivalents, but being a different
  data type.
* it not being clear that CLIEngine silently ignores invalid options
  (which will hopefully be fixed at some point by eslint#10272).

Refs:
eslint#4402
eslint#5179
eslint#6605
eslint#7247
eslint#7967
eslint#10272
webpack-contrib/eslint-loader#173
webpack-contrib/eslint-loader#252
neutrinojs/neutrino#1181
not-an-aardvark pushed a commit that referenced this issue Dec 8, 2018
This aims to prevent some of the common pitfalls/points of confusion
when configuring `CLIEngine`, such as:
* it not being clear that `baseConfig` supports all of the options
  defined in the `.eslintrc.*` schema, and so can be used for options
  that are not supported as top-level `CLIEngine` arguments such as
  `extends` and `settings.
* some of the CLIEngine options being named the same or almost the
  same as their `.eslintrc.*` equivalents, but being a different
  data type.
* it not being clear that CLIEngine silently ignores invalid options
  (which will hopefully be fixed at some point by #10272).

Refs:
#4402
#5179
#6605
#7247
#7967
#10272
webpack-contrib/eslint-loader#173
webpack-contrib/eslint-loader#252
neutrinojs/neutrino#1181
@eslint-deprecated eslint-deprecated bot added the auto closed The bot closed this issue label Dec 11, 2018
@eslint-deprecated
Copy link

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that issues failing to reach accepted status after 21 days tend to
never be accepted, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

@edmorley
Copy link
Contributor Author

Please can this be reopened for further discussion? :-)

@platinumazure platinumazure removed the auto closed The bot closed this issue label Dec 11, 2018
@platinumazure platinumazure self-assigned this Dec 11, 2018
@platinumazure
Copy link
Member

Thanks @edmorley, I'll reopen.

@platinumazure platinumazure reopened this Dec 11, 2018
@kaicataldo
Copy link
Member

@platinumazure It looks like you've assigned this to yourself. Are you still championing this change? If so, should it go through the RFC process that we have that in place?

@btmills
Copy link
Member

btmills commented Feb 13, 2020

For the v7.0.0 release, we're focusing on implementing RFC40 and the new ESLint class, so we decided in today's TSC meeting not to include this in v7.0.0. If there's still interest in doing this, we can consider it for the v8.0.0 release.

@btmills
Copy link
Member

btmills commented Apr 11, 2021

We incorporated more validations into the new ESLint API, including warning about extra unknown options. The CLIEngine is now deprecated, so we can close this as fixed in the new API!

@btmills btmills closed this as completed Apr 11, 2021
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Oct 9, 2021
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Oct 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion breaking This change is backwards-incompatible cli Relates to ESLint's command-line interface enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion
Projects
None yet
Development

No branches or pull requests

4 participants