Update to eslint 9

[avatus]

e udates https://github.com/gravitational/teleport.e/pull/5672

[ravicious]

pnpm i --resolution-only was returning warnings about unmet peer deps, so I updated typescript-eslint and eslint-plugin-testing-library.

There's no breaking changes in eslint-plugin-testing-library that would affect us. https://github.com/testing-library/eslint-plugin-testing-library/releases/tag/v7.0.0

Output of pnpm i --resolution-only

$ pnpm i --resolution-only
Scope: all 7 workspace projects
 WARN  `node_modules` is present. Lockfile only installation will make it out-of-date
 WARN  12 deprecated subdependencies found: @npmcli/[email protected], @types/[email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Progress: resolved 1459, reused 0, downloaded 0, added 0, done
 WARN  Issues with peer dependencies found
web/packages/build
└─┬ eslint-plugin-testing-library 6.3.0
  ├── ✕ unmet peer eslint@"^7.5.0 || ^8.0.0": found 9.16.0
  └─┬ @typescript-eslint/utils 5.62.0
    └── ✕ unmet peer eslint@"^6.0.0 || ^7.0.0 || ^8.0.0": found 9.16.0

[ravicious]

There used to be a babel plugin, does v9 no longer need it?

[gzdunek]

We didn't have it in extends/rules so it didn't actually work.

[ravicious]

I like that you changed numeric values to 'off', 'error', etc. (it was always super confusing for me). But I think some of the old comments and the order of rules were important, so I brought back the original order and the comments. Let me know if you disagree.

[ravicious]

I cleaned up some more rules where a rule was turned off but removing it did not create any new issues. It might be that someone put them in there with the intention of turning them on later, but if so, those should be captured in a GitHub issue if possible.

I tried to see if there's a way to preserve git history. However, the old rules were indented at 4 spaces, while the new are at 6… So there's just no way to do this unless we go way out of our way.

Everything seems fine and ESLint seems to catch issues in OSS and teleport.e. Too bad I have to update some plugins in my editor to make it work again. ;f

[avatus]

How do we feel about backporting this one? I think it'd be best to get it back as far as we can

[ravicious]

I don't think we need to backport this. We haven't kept ESLint deps on v17 up to date, so that would be the main problem.

As long as we don't make master less strict than release branches, everything should be fine. Otherwise we could run into a situation where you have code that's valid on master that you cannot backport to a release branch because of ESLint issues.

[gzdunek]

These two plugins can be removed, we don't use them.

[gzdunek]

We miss this in the new config so these plugins don't work. Having a plugin in plugins doesn't automatically activate it.

[avatus]

https://github.com/gravitational/teleport/blob/avatus/eslint/web/packages/build/eslint.config.mjs#L128-L151

is this not that? it seems to work for me. if i for example change this rule to error

'jest/consistent-test-it': 'error',

pnpm lint throws a bunch of a new errors

[gzdunek]

Yeah, but it did it because you explicitly enabled this rule. If you remove it completely, you will not get any warnings/errors, which means it is disabled by default.

[avatus]

I see what you mean. What is your recommendation? If these are "off" but don't affect any defaults, I'm unsure what would be required of us. Should we turn on at least one?

[ravicious]

I think what Grzegorz meant is that the new eslint.config.mjs does not use any configs from jest, jest-dom and testing-library/react plugins. So we'd need to add them in the override and also keep the existing rules that are in the override.

[avatus]

I'm not following. in the new config schema, they suggest to do it this way
https://www.npmjs.com/package/eslint-plugin-jest

With flat configuration, just import the plugin and away you go:

with examples of using override for the old eslint. but in the new one, they say that adding in rules/plugins to the object is just fine.

  {
    // update this to match your test files
    files: ['**/*.spec.js', '**/*.test.js'],
    plugins: { jest: pluginJest },
    languageOptions: {
      globals: pluginJest.environments.globals.globals,
    },
    rules: {
      'jest/no-disabled-tests': 'warn',
      'jest/no-focused-tests': 'error',
      'jest/no-identical-title': 'error',
      'jest/prefer-to-have-length': 'warn',
      'jest/valid-expect': 'error',
    },
  },

or are you suggesting we'd have to do something liek this

  {
    ...jestPlugin['configs'],
    ...testingLibraryPlugin.configs,
    ...jestDomPlugin.configs,
    files: ['**/*.test.{ts,tsx,js,jsx}'],
    languageOptions: {
      globals: {
        ...globals.jest,
      },
    },
    plugins: {
      jest: jestPlugin,
      'testing-library': testingLibraryPlugin,
      'jest-dom': jestDomPlugin,
    },
    rules: {
      'jest/prefer-called-with': 'off',

[gzdunek]

Uhhh I have it working. At first the new syntax was confusing to me, but actually it's quite simple.
If possible, we should use the entire recommended config from a plugin (like eslint.configs.recommended) which will merge everything automatically. If we need to apply a few plugins for a single file pattern, we need to this manually, like:

files: ['**/*.test.{ts,tsx,js,jsx}'],
plugins: {
  jest: jestPlugin,
  'testing-library': testingLibraryPlugin,
},
rules: [
  ...jestPlugin.configs['flat/recommended'].rules,
  ...testingLibraryPlugin.configs['flat/react'].rules,
]

or are you suggesting we'd have to do something liek this

Almost ;)
In your code, the last rules block would overwrite ...jestPlugin['configs'], ...testingLibraryPlugin.configs etc, since it's a regular JS object.

I enabled all recommended configs for our plugins. Previously we didn't do it for the react plugin, so I had to disable a few rules (I made them warnings).

[avatus]

Ok that makes sense now. sorry for the back and forth. @gzdunek @ravicious i rebased (to prepare for merge). let me know when you think its ready to go.

Also, i don't think i'll need the e branch for this anymore. After the changes to this PR, i ran the linter again against e/master and no changes happened (which makes sense). I think i'll close that PR

[ravicious]

If it all passes I think let's do it!

[gzdunek]

Let's gooo