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

Configuration Migration of ESLint configuration file from the eslintrc format to the new flat config format #1091

Merged
merged 8 commits into from
Oct 24, 2024

Conversation

insane-22
Copy link
Contributor

Problem

According to ESLint's official docs- "The flat config file format has been the default configuration file format since ESLint v9.0.0". Codebase isn't updated with this new config

Solution

Added new eslint.config.js file and migrated code from .eslintrc.js and .eslintignore to this new config file

insane-22 and others added 3 commits May 30, 2024 00:04
Something went wrong in e2ff10e , I suspect some manual modification that ended up breaking the lockfile with this error message:
"SyntaxError: Invalid value type 1540:0 in yarn.lock"
Copy link
Member

@MonkeyDo MonkeyDo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for starting this update!

Something was wrong with the yarn lockfile, I think maybe it got corrupted somehow, or you tried to manually correct something in there?
In any case I regenerated it so at least the tests can run.
This suggests to me that you did not try to run the linting locally, or at least not after the last commit. It is essential that you try running the linting command to make sure everythinng works as you expect it to.

That shows that at least the eslint.config.js file has a lot of linting issues, ironically :)
You can fix a lot of the issues automatically by running yarn lint eslint.config.js --fix.

More problematically, the previous configuration file eslintrc.js still exists in the repo, and I can't tell you which config is used...
I suppose it should probably be deleted in favor of the new file.
Also do a global search in the repo for the file name, to replace it with the new one where appropriate.

I also think this PR would be the perfect occasion to update ESLint to v9 and allow us to test the changes with the target version; without that, i'm not sure the PR actually tests that the config changes will work for v9.

eslint.config.js Outdated Show resolved Hide resolved
eslint.config.js Outdated Show resolved Hide resolved
eslint.config.js Outdated Show resolved Hide resolved
@insane-22
Copy link
Contributor Author

Thank you very much @MonkeyDo for again helping while solving the issue :)
I actually was trying a few things and that's why lock file error was there. I also tried the the upgrade to eslint v9 but there is a problem, "eslint-plugin-import" dependency does not have compatibility with Eslint v9 (import-js/eslint-plugin-import#2948).

@insane-22
Copy link
Contributor Author

but at the same time eslint.config.js doesnt work with versions prior to v9, hence it doesnt make sense to leave it this way.

One work-around I could find is import-js/eslint-plugin-import#2948 (comment)
or we can wait until the whole v9 ecosystem is ready to use. what in your opinion shall be the next steps?

@MonkeyDo
Copy link
Member

I read up ont he whole discussion, and it looks like v9 support in the plugin is going to take some time.
In the meantime, from what I read you should be able to use this drop-in replacement: https://github.com/un-ts/eslint-plugin-import-x
Please add a TODO comment note where you can, with a short explanation that we are using this package as a temporary replacement until v9 support is out, that it should be replaced as soon as available, and a link to the eslint-plugin-import issue.

@MonkeyDo
Copy link
Member

Bump @insane-22, do you have the time to look into the alternate plugin, or would you prefer I do?
Thanks again for looking at these config changes!

@insane-22
Copy link
Contributor Author

Hello @MonkeyDo, sorry for this delay, was caught up in a few things and thanks a lot for this reminder.
Please allow me some more time, I will be back with the changes :)

@insane-22
Copy link
Contributor Author

Hello hello, I'm back with new sets of errors :]

"@eslint/[email protected]: The engine "node" is incompatible with this module. Expected version "^18.18.0 || ^20.9.0 || >=21.1.0". Got "20.6.1" "
when i changed dockerfile as "FROM metabrainz/node:20 as bookbrainz-base"
it still has node v20.6.0, which is incompatible, adding any advanced versons shows
"failed to solve: metabrainz/node:20.9.0: failed to resolve source metadata for docker.io/metabrainz/node:20.9.0: docker.io/metabrainz/node:20.9.0: not found"

@MonkeyDo
Copy link
Member

Sorry for another long delay 😶‍🌫️

I've updated the docker image for the base node image, and updated eslint-plugin-import which now supports ESLint v9 (I was keeping an eye on the PR/issue).
Let's see how the tests fare now

Update import and export style, format the file according to its rules, and chasnge a couple of rules from errors into warnings (comma-dangle, quotes)
@MonkeyDo
Copy link
Member

Thanks a lot for working on this, and for coming back to it multiple times :)
I'll take over from here with the ESLint 9 upgrade, if you don't mind, as it gets pretty complicated from here onwards: 7000+ errors the first time i ran it, 4000+ after I managed to make module resolution work, 1000+ after a few tweaks, and now 100+ of various issues that need a style guidelines decision to be taken...

@MonkeyDo MonkeyDo merged commit a78d508 into metabrainz:master Oct 24, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants