🐛 ESLint edge case: `flatCompat.extends` in FlatConfig files

kachkaev commented 1 week ago

Prerequisites

[X] I'm using the latest version
[X] I've read the relevant documentation
[X] I've searched for existing issues
[X] I've checked the list of known issues
[X] I've read the issue reproduction guide

Reproduction url

https://codesandbox.io/p/devbox/yjgd5f

Reproduction access

[X] I've made sure the reproduction is publicly accessible

Description of the issue

With a recent end-of-life for ESLint v8, more people are migrating to FlatConfig. New eslint.config.js file is pretty standard, so most imports can be tracked without any special tricks. However, if this file refers to a config that is still relying on an old config system, Knip fails to detect that a project dependency is used.

Here is an example for eslint-config-next:

`.eslintrc.json` before conversion

{
  "extends": "next/core-web-vitals"
}

Knip is smart enough to know that next/core-web-vitals comes from eslint-config-next, so does not mark this dependency as unused.

`eslint.config.js` after conversion

npx @eslint/migrate-config .eslintrc.json

↓

import path from "node:path";
import { fileURLToPath } from "node:url";
import js from "@eslint/js";
import { FlatCompat } from "@eslint/eslintrc";

const __filename = fileURLToPath(import.meta.url);
const __dirname = path.dirname(__filename);
const compat = new FlatCompat({
  baseDirectory: __dirname,
  recommendedConfig: js.configs.recommended,
  allConfig: js.configs.all,
});

export default [...compat.extends("next/core-web-vitals")];

Knip does not know about compat.extends("next/core-web-vitals"), so marks eslint-config-next as unused in package.json.

Note that in the real world scenario the call may also look like this:

...flatCompat.extends('next/core-web-vitals', 'next/typescript')

In the long term most popular community plugins and configs will be converted to FlatConfig so will be imported natively using ESM. But it would be great if Knip could handle compat.extends in the meantime to help folks with migration.

webpro commented 1 week ago

In the long term most popular community plugins and configs will be converted to FlatConfig so will be imported natively using ESM.

My assumption was indeed that Knip didn't need extra work here. But I've already started in #806 to actually load eslint.config.* files and start resolving dependencies. Was surprised to see this is necessary though :(

When I try that version of Knip in this CSB I'm getting this:

ERROR: Error loading /project/workspace/eslint.config.mjs
Reason: Cannot read config file: /project/workspace/node_modules/.pnpm/eslint-config-next@15.0.1_eslint@8.57.0_typescript@5.4.5/node_modules/eslint-config-next/index.js
Error: Failed to patch ESLint because the calling module was not recognized.

This happens because... it's complicated. But here's the gist.

The reason is an ESLint config/plugin loads some rushstack stuff (which throws the error):

https://github.com/vercel/next.js/blob/2e28c965279de90ce4bfca673196c27dd6117027/packages/eslint-config-next/index.js#L53

Work-around in Knip:

https://github.com/webpro-nl/knip/blob/c6d5c10e895ad6f006c79d3f78eb451c7931b552/packages/knip/src/util/jiti.ts#L9-L10

This fix works fine so far. Apparently we're having a new scenario here in which the rushstack alias isn't active and I don't fancy jumping into this rabbit hole again.

Maybe the fix is easy, idk. This also just made me reconsider the work in #806 for now.

kachkaev commented 1 week ago

Ah yeah rushstack. It’s causing problems in other tools, at least in ESLint config expector:

webpro commented 1 week ago

Thanks for sharing that! Will follow those issues for sure.

webpro-nl / knip