-
Notifications
You must be signed in to change notification settings - Fork 10
migrated eslint from v8 to v9 and updated dependencies version related #59
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
base: main
Are you sure you want to change the base?
Conversation
WalkthroughAdds a new flat ESLint config Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Nitpick comments (2)
eslint.config.cjs (2)
26-26: Remove redundant emptyparserOptions.The empty
parserOptions: {}objects on lines 26 and 47 serve no purpose and can be removed for cleaner code.Apply this diff:
parser: tsParser, ecmaVersion: "latest", sourceType: "module", - parserOptions: {}, },And:
}, sourceType: "script", - parserOptions: {}, },Also applies to: 47-47
16-52: Consider adding ignore patterns.The configuration lacks ignore patterns for common directories that should not be linted, such as
node_modules,dist,build, or coverage directories.Add an ignores configuration:
module.exports = defineConfig([ { ignores: ["node_modules/", "dist/", "build/", "coverage/", "*.config.js"], }, // ... rest of configs ]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package.jsonis excluded by!**/*.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!**/*.yaml
📒 Files selected for processing (1)
eslint.config.cjs(1 hunks)
| @@ -0,0 +1,52 @@ | |||
| const { defineConfig } = require("eslint/config"); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix the import path for defineConfig.
In ESLint v9, defineConfig is exported from "eslint" directly, not from "eslint/config". This will cause a module resolution error.
Apply this diff:
-const { defineConfig } = require("eslint/config");
+const { defineConfig } = require("eslint");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const { defineConfig } = require("eslint/config"); | |
| const { defineConfig } = require("eslint"); |
🤖 Prompt for AI Agents
In eslint.config.cjs around line 1, the require currently imports defineConfig
from "eslint/config" which causes module resolution errors in ESLint v9; update
the import to require defineConfig from "eslint" instead (replace
require("eslint/config") with require("eslint")) so defineConfig is loaded from
the correct module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's is not correct in eslint v9
we should keep this line.
const { defineConfig } = require("eslint/config");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🌐 Web query:
ESLint v9 defineConfig import path eslint/config
💡 Result:
Use the flat-config helper exported from "eslint/config". Example:
import { defineConfig } from "eslint/config";
export default defineConfig([
{ files: ["**/*.js"], rules: { semi: "error" } },
]);
(ESLint docs show defineConfig imported from "eslint/config".) [1][2]
Sources:
[1] ESLint — Configuration files.
[2] ESLint — Configuration migration guide.
@p-stam001, you're absolutely right, and I apologize for the incorrect suggestion. In ESLint v9, defineConfig is indeed exported from "eslint/config", not from "eslint" directly. The original line in your PR is correct:
const { defineConfig } = require("eslint/config");According to the ESLint v9 documentation, the flat-config helper defineConfig should be imported from "eslint/config". Thank you for the correction!
✏️ Learnings added
Learnt from: p-stam001
PR: kinde-oss/kinde-remix-sdk#59
File: eslint.config.cjs:1-1
Timestamp: 2025-10-22T05:59:01.328Z
Learning: In ESLint v9, the `defineConfig` helper is exported from `"eslint/config"`, not from `"eslint"` directly. The correct import is: `const { defineConfig } = require("eslint/config");` for CommonJS or `import { defineConfig } from "eslint/config";` for ES modules.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
@p-stam001 I think we can safely go ahead and delete the old .eslintrc.cjs here |
yea, then I will commit after delete the old .eslintrc.cjs |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This PR updates ESLint from v8 to v9 and migrates the config to a flat ES5/CommonJS format.
And it updates all dependencies related also.
TypeScript support via @typescript-eslint/parser and plugin is preserved.
Browser and Node globals are included.
Overrides for ESLint config files (.eslintrc.js/.cjs) added.
Removed the need for --legacy-config in pnpm lint.
Linting passes locally and in CI. No runtime or functional changes.
Also this PR includes the changes of these PRs #56 #51 #52 #53