-
Notifications
You must be signed in to change notification settings - Fork 806
Update eslint configuration to use new flat config instead of rushstack #12198 #13440
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: develop
Are you sure you want to change the base?
Conversation
Build Artifacts
|
Why has the file been renamed to an mjs file? This is causing issues for non-ESM compliant code in the codebase, so if there's not a compelling reason to do this rename, it might be easier to keep it as a .js file. |
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.
A little bit more cleanup required - some for sure, some I have questions about.
eslint.config.js
Outdated
import { modernModuleResolution } from '@rushstack/eslint-patch/modern-module-resolution'; | ||
import kolibriConfig from 'kolibri-format/eslint.config.mjs'; | ||
|
||
modernModuleResolution(); |
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.
Is this still necessary? We had originally introduced rushstack so as not to update to the flat config just yet, so I was hoping that switching would remove this dependency.
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.
Yeah this is not needed.
@@ -1,5 +1,6 @@ | |||
{ |
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.
I can't comment on the file directly, as it is too large, but you should not commit the package-lock.json.
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.
Yeah maybe due to some bug or error it got commited
eslint.config.js
Outdated
@@ -0,0 +1,8 @@ | |||
import { modernModuleResolution } from '@rushstack/eslint-patch/modern-module-resolution'; | |||
import kolibriConfig from 'kolibri-format/eslint.config.mjs'; |
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.
I don't see a file called eslint.config.mjs
in the kolibri-format package any more, so I suspect this may not be working?
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.
Yeah i changed it.
@@ -1,5 +1,6 @@ | |||
{ | |||
"name": "kolibri-root", | |||
"type": "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.
Is this update still needed if we have no .mjs files in the package?
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.
We need to keep "type": "module" because we're using ESM syntax (e.g., import/export) in .js files. Since we're renaming the config to eslint.config.js and it's written using ESM, this setting is required for Node to interpret it correctly.
@@ -0,0 +1,368 @@ | |||
import js from '@eslint/js'; |
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.
I assume the rename is a convention change for the flat 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.
yes
try { | ||
esLintConfig = require(`${hostProjectDir}/.eslintrc`); |
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.
Did using a dynamic import()
statement in place of the require not work here?
Summary
I have upgraded the old ESLint configuration to the new Flat Config format. The configuration file has been renamed to eslint.config.mjs, and the loading logic in index.js has been updated accordingly. I removed the extends property and replaced overrides with individual entries in the config array, as required by the Flat Config format. The setup has been tested using npx eslint . --debug to ensure it works correctly.
References
Issue #12198