eslint v6 support

[ljharb]

Tracking issue for adding eslint v6 to travis-ci and peer deps.

(ref airbnb/javascript#2036)

[sheepsteak]

I've had a go at trying to do this and it seems the major issue is that getConfigForFile that is used here - https://github.com/sarbbottam/eslint-find-rules/blob/5c192eb/src/lib/rule-finder.js#L26 - now requires a file path to be passed in. I think in previous ESLint versions it also wanted one but would fall back to cwd. I think it now requires a file because depending on the file path passed in a different set of rules could be present (through the use of overrides maybe?).

So there are some decisions to be made about how to get around this.

[ljharb]

cc @mysticatea @platinumazure who i think were involved with this eslint change

[mysticatea]

Per our document, --print-config and getConfigForFile() require a file path. That fallback was not intentional.

ESLint may use different configs for each source code file because it supports cascading and glob-based configuration. Because I'm not familiar with this tool, I'm not sure if what decision is proper.

[ljharb]

@mysticatea ideally it would take a directory or a glob; it could even be an array of all found config files within a directory.

The intention of the tool is to help manage config files themselves.

[mysticatea]

Please mind getConfigForFile() is the method to get the config for a file, so the name is getConfigForFile(). :)

A config file doesn't 1:1 map to a config object that getConfigForFile() method returns because the method determines which overrides entries were applied. The criteria of overrides entries can overlap each other. This means that it will have to return all combinations of overrides entries in all of the config files and shareable configs. We can propose such an API but I'm worried about a combinatorial explosion.

For now, how about...

const glob = require("glob")
const { CLIEngine } = require("eslint")
const engine = new CLIEngine()

function getConfigs(pattern) {
    const configs = new Set()

    for (const filePath of glob.sync(pattern, { dot: true, matchBase: true })) {
        if (!engine.isPathIgnored(filePath)) {
            configs.add(engine.getConfigForFile(filePath))
        }
    }

    return configs
}

console.log(getConfigs("lib/*.js"))

This should work both of ESLint 6 and older versions.

[ljharb]

This approach definitely seems to work to get the configured rules; unfortunately it doesn't seem to be sufficient to make this package work on eslint 6 :-/

[ljharb]

My first attempt is here: master...ljharb:eslint_6

[nosilleg]

@ljharb Are there any issues with your first attempt that are preventing a merge? (I'm not able to run the code given my current location/circumstances).

My initial thought from looking at the code is that it can report different rules/plugins than the current version since your new code will account for some overrides. The fact that the overrides can have non *.js (as per the new glob) criteria means that not all overrides will be evaluated.

To ovoid those differences would replacing the current getConfigForFile() with getConfigForFile("./does-not-exist.a9<"); suffice?

This could match an overrides, but should be less likely.

[ljharb]

@nosilleg it’s not complete, so it doesn’t work yet. If you have any suggestions and want to make a PR, that would be appreciated :-)

[nosilleg]

@ljharb I'm not going to be able to run code for at least a week, so can't do a full PR and test run.

Based on the discussions and a read of the code the proposed change would be to simply change this line: https://github.com/sarbbottam/eslint-find-rules/blob/master/src/lib/rule-finder.js#L26

To:

return cliEngine.getConfigForFile("./does-not-exist.a9<");

A quick test on RunKit seems to give useful results. https://runkit.com/5d4493461549b300131329cf/5d4493541549b300131329e1

Obviously since I haven't run the code there may be things that I'm missing

[lddubeau]

@ljharb I took your branch and added a commit to deal with the test failures. My branch is here. The commit I added is this one.

The problem was that eslint 6 tries to resolve plugin module names to file names prior to calling require and that's something that proxyquire does not handle automatically.

With my commit, all tests pass.

[ljharb]

@lddubeau thanks, that's super appreciated!

I'm still getting some failures locally; I'm seeing different failures in CI: https://travis-ci.com/ljharb/eslint-find-rules/builds/121896205

[lddubeau]

@ljharb Glancing quickly at the CI report, shouldn't there be also be a bunch of tests with ESLINT=6?

I see two different failures:

1. Can't load eslint/lib/shared/relative-module-resolver. That's going to happen on eslint < 6 since that file does not exist there. A try... catch around the offending require allows tests to pass on eslint 5. I've amended my commit to take care of that.

2. The other failure I'm seeing is are linting errors which appear to be due to the changes you made in your commit. When I sent my comment yesterday I had noticed this linting failure but I was very pressed for time and concerned more about providing a heads up to make sure somebody's time was not wasted duplicating my work than about any other consideration. I've added a commit that fixes this issue.

I've force-pushed my branch just now.

[lddubeau]

@ljharb At the moment, I'm inclined to think @nosilleg's approach to get the configuration is what we need.

Now bear in mind I'm not familiar with the internals of eslint and I'm not familiar at all with eslint-find-rules. (The reason I'm here is that eslint-find-rules's incompatibility with eslint 6 blocks the release of an eslint-6-compatible airbnb eslint configuration, which my own stock eslint configuration depends on. So I'm blocked from moving to eslint 6 due to the issue here. I figured I could whine or try to help, and I decided to try to help rather than whine.)

It does appear that in eslint 6, cliEngine.getConfigForFile("./does-not-exist.a9<"); does the same as cliEngine.getConfigForFile() did in eslint 5. So this preserves the status quo.

This essentially prevents the overrides sections of configuration files from applying but:

1. eslint-find-rules was also ignoring them when calling getConfigForFile without parameters.

2. eslint supports cascading configurations, which eslint-find-rules has also been ignoring by calling getConfigForFile without parameters.

---

Now maybe there's an argument to be made that eslint-find-rules ignoring overrides and cascading configurations was wrong and now it should take both into account. If indeed eslint-find-rules should be modified to take into account overrides and cascading configurations, I have three observations:

1. This would be a breaking change: there exist some configurations and file trees for which the results returned by eslint-find-rules would change.

2. I'd suggest still allowing a flag (e.g. an --only-invariant flag on the command line) that would allow those users who only need the old behavior to still get this behavior and not scan the disk for files needlessly.

3. eslint-find-rules should allow users to specify the pattern of files to search rather than use a hardcoded search. For instance, although these use the same config.js file, this command:

   eslint -c config.js 'src/**/*.js'
   

   can use a different set of rules from this one:

   eslint -c config.js 'test/**/*.js'
   

   I'd expect eslint-find-rules to take a file pattern like eslint does. Otherwise, the configuration used by eslint-find-rules will correspond to a real run of eslint only sometimes, accidentally.

   (Also, eslint supports TypeScript, and the plan is to eventually have it replace tslint as the go-to linter for TypeScript code. (See this announcement.) Searching for **/*.js won't work for some users of eslint. They'd want to search for **/*.ts.)

[ljharb]

That's a pretty compelling argument for that approach. Let me pull in your commit.