CLIEngine#getRules doesn't work before linting files

[okonet]

I'm trying to find a way to dynamically create eslint configs during the execution. The idea is to filter out all auto-fixable rules from all plugins and presets defined in user's .eslintrc configs for specified environments (local development). The motivation is to leave fixes to https://github.com/okonet/lint-staged and remove unnecessary distractions from code editors. At the same time, on CI these rules must result in error if not followed (users can skip precommit hooks).

I've found and tried https://eslint.org/docs/developer-guide/nodejs-api#clienginegetrules already but this gives me same set of rules regardless of my .eslintrc and this set doesn't take any plugins into account.

The documentation says This method returns a map of all loaded rules. but I'm not sure what loaded means in this context.

• ESLint Version:

ESLint 4.19.1 and v5.5.0

• Node Version:

v8 and v10

• npm Version:

v5

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

Please show your full configuration:

Configuration

{
  "rules": {
    "no-semi": 2
  }
}

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

const CLIEngine = require("eslint").CLIEngine;
const cli = new CLIEngine();

console.log(cli.getRules().keys());

node ./getEslintRules.js
[Map Iterator] {
  'accessor-pairs',
  'array-bracket-newline',
  'array-bracket-spacing',
  'array-callback-return',
  'array-element-newline',
  'arrow-body-style',
  'arrow-parens',
  'arrow-spacing',
  'block-scoped-var',
  'block-spacing',
  'brace-style',
  'callback-return',
  'camelcase',
  'capitalized-comments',
  'class-methods-use-this',
  'comma-dangle',
  'comma-spacing',
  'comma-style',
  'complexity',
  'computed-property-spacing',
  'consistent-return',
  'consistent-this',
  'constructor-super',
  'curly',
  'default-case',
  'dot-location',
  'dot-notation',
  'eol-last',
  'eqeqeq',
  'for-direction',
  'func-call-spacing',
  'func-name-matching',
  'func-names',
  'func-style',
  'function-paren-newline',
  'generator-star-spacing',
  'getter-return',
  'global-require',
  'guard-for-in',
  'handle-callback-err',
  'id-blacklist',
  'id-length',
  'id-match',
  'implicit-arrow-linebreak',
  'indent-legacy',
  'indent',
  'init-declarations',
  'jsx-quotes',
  'key-spacing',
  'keyword-spacing',
  'line-comment-position',
  'linebreak-style',
  'lines-around-comment',
  'lines-around-directive',
  'lines-between-class-members',
  'max-depth',
  'max-len',
  'max-lines',
  'max-nested-callbacks',
  'max-params',
  'max-statements-per-line',
  'max-statements',
  'multiline-comment-style',
  'multiline-ternary',
  'new-cap',
  'new-parens',
  'newline-after-var',
  'newline-before-return',
  'newline-per-chained-call',
  'no-alert',
  'no-array-constructor',
  'no-await-in-loop',
  'no-bitwise',
  'no-buffer-constructor',
  'no-caller',
  'no-case-declarations',
  'no-catch-shadow',
  'no-class-assign',
  'no-compare-neg-zero',
  'no-cond-assign',
  'no-confusing-arrow',
  'no-console',
  'no-const-assign',
  'no-constant-condition',
  'no-continue',
  'no-control-regex',
  'no-debugger',
  'no-delete-var',
  'no-div-regex',
  'no-dupe-args',
  'no-dupe-class-members',
  'no-dupe-keys',
  'no-duplicate-case',
  'no-duplicate-imports',
  'no-else-return',
  'no-empty-character-class',
  'no-empty-function',
  'no-empty-pattern',
  'no-empty',
  'no-eq-null',
  ... 157 more items }

What did you expect to happen?

For this config I expect only no-semi to be in the list of loaded rules.

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

Instead I always (independent of config) get this set or rules:

{
  'accessor-pairs',
  'array-bracket-newline',
  'array-bracket-spacing',
  'array-callback-return',
  'array-element-newline',
  'arrow-body-style',
  'arrow-parens',
  'arrow-spacing',
  'block-scoped-var',
  'block-spacing',
  'brace-style',
  'callback-return',
  'camelcase',
  'capitalized-comments',
  'class-methods-use-this',
  'comma-dangle',
  'comma-spacing',
  'comma-style',
  'complexity',
  'computed-property-spacing',
  'consistent-return',
  'consistent-this',
  'constructor-super',
  'curly',
  'default-case',
  'dot-location',
  'dot-notation',
  'eol-last',
  'eqeqeq',
  'for-direction',
  'func-call-spacing',
  'func-name-matching',
  'func-names',
  'func-style',
  'function-paren-newline',
  'generator-star-spacing',
  'getter-return',
  'global-require',
  'guard-for-in',
  'handle-callback-err',
  'id-blacklist',
  'id-length',
  'id-match',
  'implicit-arrow-linebreak',
  'indent-legacy',
  'indent',
  'init-declarations',
  'jsx-quotes',
  'key-spacing',
  'keyword-spacing',
  'line-comment-position',
  'linebreak-style',
  'lines-around-comment',
  'lines-around-directive',
  'lines-between-class-members',
  'max-depth',
  'max-len',
  'max-lines',
  'max-nested-callbacks',
  'max-params',
  'max-statements-per-line',
  'max-statements',
  'multiline-comment-style',
  'multiline-ternary',
  'new-cap',
  'new-parens',
  'newline-after-var',
  'newline-before-return',
  'newline-per-chained-call',
  'no-alert',
  'no-array-constructor',
  'no-await-in-loop',
  'no-bitwise',
  'no-buffer-constructor',
  'no-caller',
  'no-case-declarations',
  'no-catch-shadow',
  'no-class-assign',
  'no-compare-neg-zero',
  'no-cond-assign',
  'no-confusing-arrow',
  'no-console',
  'no-const-assign',
  'no-constant-condition',
  'no-continue',
  'no-control-regex',
  'no-debugger',
  'no-delete-var',
  'no-div-regex',
  'no-dupe-args',
  'no-dupe-class-members',
  'no-dupe-keys',
  'no-duplicate-case',
  'no-duplicate-imports',
  'no-else-return',
  'no-empty-character-class',
  'no-empty-function',
  'no-empty-pattern',
  'no-empty',
  'no-eq-null',
  ... 157 more items }

[not-an-aardvark]

Hi, thanks for creating an issue. The issue here seems to be that rules are currently only loaded from a config the first time something is linted, not when calling getRules. The original intent of getRules was to be used after linting text (see #6582).

I would consider the current behavior to be a bug, since it doesn't really make sense that the behavior would be different before and after linting. As a workaround until the problem is fixed, I think using something like cliEngine.executeOnText("") before calling getRules should cause all of the rules to be found.

[sstern6]

Working on this if thats alright

[sstern6]

Been looking into this today and had a few questions. @not-an-aardvark from what I can see, the cliEngine doesnt actually EVER read in the rules from the config, under rules.js -> #load, which is what the cliEngine#getRules is calling, according to the documentation.

Tried to verify what you had commented by calling cliEngine#executeOnText(per your rec) but couldnt verify the no-semi rule was added to the list of rules, but upon more investigation, it looks like https://github.com/eslint/eslint/blob/master/lib/rules.js#L82 only reads the rules from a specified rules dir, not rules from an eslintrc file.

Am i missing something? If I am understanding this correctly, the ticket would need to be updating the clieEngine#getRules to read from the config file OR have the Rules#load read from a config if no ruleDir is passed in.

What do you think?

Thanks

cc @kaicataldo

[not-an-aardvark]

The current logic for loading rules is a fairly convoluted. The rules from plugins actually get loaded here during config processing. As you noticed, rules.js -> #load is used to load bundled rules and rules from a folder provided with the --rulesdir flag, but it isn't used to load rules from plugins.

Currently, CLIEngine loads rules correctly when linting text, since it always processes a config before linting the text, and the rules get added when the config is processed. With getRules, CLIEngine isn't loading any config, so it never adds the plugin rules, resulting in the buggy behavior described in this issue. So I think the simplest solution for now would be to have CLIEngine compute a config in getRules to ensure that the plugin rules get added (i.e. the first solution you proposed).

Eventually, it would be nice to refactor this so that loading a config doesn't have confusing side-effects, but I think that's outside the scope of this issue.

[sstern6]

@not-an-aardvark thank you for the context.

Currently, CLIEngine loads rules correctly when linting text, since it always processes a config before linting the text, and the rules get added when the config is processed.

I tried running per your reccomendation:

const CLIEngine = require("eslint").CLIEngine;
const cli = new CLIEngine();

console.log(cli.getRules().keys());

but the rules from my eslintrc config werent being included in the final output. Maybe i was doing something wrong?

So I think the simplest solution for now would be to have CLIEngine compute a config in getRules to ensure that the plugin rules get added.

Sounds good, it looks like well need to merge this.config.plugins with the result of that #getRules has already created. Ill take a closer look and hopefully have a PR this week!

Thanks

[not-an-aardvark]

To reiterate, the config processing currently happens when linting text. As a result, the following behavior occurs:

const CLIEngine = require("eslint").CLIEngine;
const cli = new CLIEngine();

cli.getRules().has('node/no-unsupported-features'); // => false
cli.executeOnText('');
cli.getRules().has('node/no-unsupported-features'); // => true; at this point getRules() is correct

And another example:

const CLIEngine = require("eslint").CLIEngine;
const cli = new CLIEngine();

cli.getRules().has('node/no-unsupported-features'); // => false
cli.getConfigForFile('');
cli.getRules().has('node/no-unsupported-features'); // => true; at this point getRules() is correct

So it seems like the easiest solution would be to just put a call to this.getConfigForFile CLIEngine#getRules before calling Linter#getRules. It shouldn't be necessary to read this.config.plugins from within CLIEngine. (In the long term, it would be a good idea to decouple CLIEngine from the config logic so that getConfigForFile doesn't have side-effects, but I think it would be better to apply the simpler fix for this bug in the short term.)

[sstern6]

@not-an-aardvark thanks for the clarification.

[nzakas]

@sstern6 are you still working on this?

[sstern6]

@nzakas yes will have this done within the next few days. Sorry got caught up in other issues.

That ok?

[nzakas]

Absolutely! I was just interested in taking a look if you had moved on. No rush at all.

…

On Thu, Nov 15, 2018 at 10:19 AM Scott Stern ***@***.***> wrote: @nzakas <https://github.com/nzakas> yes will have this done within the next few days. Sorry got caught up in other issues. That ok? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#10838 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACWkoq1M_nvHFi5eeTBKscAHpXAwuJyks5uvbAcgaJpZM4WeZF1> .

--

______________________________ Nicholas C. Zakas @SlickNet Author, Principles of Object-Oriented JavaScript <http://amzn.to/29Pmfrm> Author, Understanding ECMAScript 6 <http://amzn.to/29K1mIy>

[sstern6]

@not-an-aardvark @nzakas currently working on this but had a few questions about implementation.

When adding this. getConfigForFile("") in #getRulesa lot of tests broke due to the fact that this conditional was met https://github.com/eslint/eslint/blob/master/lib/config.js#L252 when calling getRules in the test when CliEngine was not passed a config file.

I wasnt sure if we should add additional guards to prevent this from throwing or add an eslint config option to our tests, for example, here, CliEngine is not being passed a config file, allowing the error to throw: https://github.com/eslint/eslint/blob/master/tests/lib/cli-engine.js#L205.

Thanks

[nzakas]

Hmm, this is part of the concern I had with this feature request, as I don't think it makes a lot of sense for getRules() to trigger configuration loading in such a blunt way.

If we still want to do this, it might make more sense to create a getRulesAvailableToFile() method that just does what we actually want. (Looking at this again, I'm not sure getConfigForFile('') is something we actually want to happen. You really should provide a filename since the file could be outside of the current working directory.) That way, we can make it clear to people that the rules being returned are the ones available to the file in question.

[sstern6]

Ok thanks for the feedback, if im understanding correctly, we should make a getRulesAvailableToFile function that loads in the plugins that will be called from #getRules.

getRulesAvailableToFile should take a path to a file name in case its outside of the cwd? And that would be the config file where the plugins would be loaded from?

Could you confirm i am not missing anything/understand what you asked @nzakas .

[not-an-aardvark]

@nzakas Currently, I don't think it's possible for different plugins to be loaded for different files (although the rules themselves can be configured differently). So it seems like having a getRules function without a filepath should still work fine.

[nzakas]

@sstern6 I actually meant getRulesAvailableToFile() should be a completely separate method that exists alongside getRules(), not called from inside it.

@not-an-aardvark that's true, however, you can't know which plugins to load (and therefore make available) without knowing which file you're referring to. If you always pass an empty string, you're assuming there's some file in the current working directory and there's a .eslintrc file in that directory or a parent, and that's a lot of assumptions to make. A file in a subdirectory might contain more plugins to load.

I'm sure there are other ways to solve this, though. Some of the top of my head:

• getRules() could accept an optional parameter that is an ESLint config. If that config has plugins, it could trigger loading of the plugins.
• Add a method to CLIEngine to allow direct loading of plugins, which would make getRules() behave correctly after a plugin was loaded.
• The getRulesAvailableToFile() method I already mentioned.

Bottom line for me is that I don't believe changing getRules() to do a silent dynamic lookup is the right choice. That seems like an unexpected side effect that could create confusion.

[sstern6]

@nzakas would you like to take this one?

[not-an-aardvark]

Marking as "evaluating" because the correct solution isn't clear here.

[nzakas]

I think there's a larger meta question here: what is the purpose of getRules()? If the purpose is simply to return the rules that have been loaded at the time it's called (as indicated in the docs), then I don't think that method should change. I think that's the most rational thing for getRules() to do.

Looking back over the original description, it seems like the use case can be accomplished by doing something along these lines:

const engine = new CLIEngine();

// evaluating the config also loads plugins
const config = engine.getConfigForFile("some/sample/file/to/evaluate");

const rules = engine.getRules();

Given that the desired use case can already be accomplished with existing APIs, my suggestion would be to not make any changes.

[eslint-deprecated]

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.