Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛 ESLint edge case: flatCompat.extends in FlatConfig files #818

Open
6 tasks done
kachkaev opened this issue Oct 29, 2024 · 3 comments
Open
6 tasks done

🐛 ESLint edge case: flatCompat.extends in FlatConfig files #818

kachkaev opened this issue Oct 29, 2024 · 3 comments
Labels
bug Something isn't working

Comments

@kachkaev
Copy link
Contributor

kachkaev commented Oct 29, 2024

Prerequisites

Reproduction url

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

Reproduction access

  • 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.

@kachkaev kachkaev added the bug Something isn't working label Oct 29, 2024
@webpro
Copy link
Collaborator

webpro commented Oct 29, 2024

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:

'@rushstack/eslint-config/patch/modern-module-resolution': empty,
'@rushstack/eslint-patch/modern-module-resolution': empty,

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
Copy link
Contributor Author

kachkaev commented Oct 29, 2024

@webpro
Copy link
Collaborator

webpro commented Oct 30, 2024

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

apepper added a commit to Scrivito/scrivito-portal-app that referenced this issue Oct 31, 2024
Workaround needed until webpro-nl/knip#818 is fixed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants