Investigate: Config file changes and how they affect this package

savetheclocktower commented 3 months ago

I figured this package would need revisiting once the new “flat” config file became more ubiquitous, but I'm pleasantly surprised at how well the package handles this currently.

Testing scenario

I created a test project that uses a from-scratch eslint.config.mjs file:

import globals from "globals";
import pluginJs from "@eslint/js";

export default [
  {
    ignores: ["ignored.js"]
  },
  {languageOptions: { globals: globals.browser }},
  pluginJs.configs.recommended,
  {
    rules: {
      'no-unused-vars': 'warn'
    }
  }
];

I then created two files — index.js and ignore.js — with very similar content. The index.js file got

let foo = "bar";

console.log(zort);

which exhibited two violations: an unused variable (foo) and an unrecognized variable (zort). I did the same for ignored.js, but changed the variable names so I wouldn't get the two files mixed up.

Findings

I was amazed at how well this package worked without our having made any changes to specifically support the new config file format. I chalk this up to the fact that we rely on ESLint itself to find the right config file; we don't try to substitute our own logic.

Able to change rules and have them reflected on next lint

I was able to change no-unused-vars back to error, return to index.js, save the file, and see the warning turn back into an error.

Able to change ignored pattern and have it reflected on next lint

I was able to change the ignored.js entry in ignores to ignoredx.js, save the config file, switch to ignored.js, save the file, and see linting messages appear (since the file is no longer ignored).

Changes we need to make

I've found two changes that we should make: one easy and one a bit trickier.

First: we need to broaden our “detect changes in config files” logic to include all files of the format eslint.config.(mc)?(j|t)s for safety's sake. Before I did this, the changes I made to ESLint config didn't take effect until I reloaded the window.

Second: we need to handle ignored files better:

We use the presence of an .eslintignore as a signal that we should use a given directory as a cwd for running ESLint. We might want to enhance this logic to look for eslint.config.js files as well; since ESLint config is now flat, this should be a safe change to make.
On older ESLint versions, presence of a file in .eslintignore seems to trigger the exact behavior we want: when that file gets linted, we should get an empty array as a result. On newer ESLint versions that define ignores in eslint.config.js, I instead get this:

There's an option to suppress this — warnIgnored: false passed into the ESLint constructor — but older ESLints complain about options they don't recognize, so we can't add it indiscriminately. Might have to do a version sniff. (This is really annoying behavior on ESLint's part.)

Testing

Testing will be hard! For the first time we'll have to have different fixtures using different versions of ESLint. I hope we can figure out a good way to do this for running tests locally, but if not we might just have to increase the number of CI-only tests.

Caveats

I didn't test any oddball ESLint plugins, or even mainstream ones like typescript-eslint, with the new config file syntax. I might try to upgrade ESLint on my work project in order to use it as a guinea pig.

scagood commented 3 months ago

The version checking thing is what I started to do here: https://github.com/pulsar-linter/linter-eslint-node/pull/2

I may have remade the repo over at scagood/pulsar-eslint.

You can see different code for different eslint versions here: https://github.com/scagood/pulsar-eslint/tree/main/lib/workers

I also updated the root config finding to match looking only for the eslint.config.js

scagood commented 3 months ago

In terms of plugin tests, I have been using these for most of my projects:

pulsar-linter / linter-eslint-node