-
Notifications
You must be signed in to change notification settings - Fork 1
feat(eslint): Add support for flat eslint v9 #204
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
Conversation
24e2624
to
3854a40
Compare
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.
First of all, thank you for your PR and introducing the migration to ESLint v9.
Now to the review and firstly to commits.
Multiple changes are not atomically separated into the commits.
For example:
- there is a modification of the workflow that should be in a separate commit
- there is an update of the prettier dependency + modification of other packages, which should be in a separate commit
- there are changes in 3 packages, each should be committed with its scope (should be enforced by commitlint 🤔)
Secondly, the API/structure.
I prefer to clean and extend the configs. For example:
@lmc-eu/esling-config-base/flat
@lmc-eu/esling-config-base/flat/optional
- etc...
Keep in mind that these are the features, because you are only adding a new config.
Thirdly, the breaking changes and back-compatible changes.
Some changes, like changing the Node.js version, are breaking.
However, you are adding support for flat config as a feature, so I am not sure how this will work together.
I have no problem with these two scenarios:
- we are replacing legacy config with the flat one, thus it is ok replace the content of the files and change the Node.js version
- we are adding flat config as a feature, thus it should not be breaking, and when I upgrade, everything should work as before
I have not checked the files themselves. These are the main architectural problems I see here. Please, address them first. I should be able to assist.
9762280
to
d20980e
Compare
There are places where you are allowing Node.js version 16, 18, 20, and somewhere only 20. Shouldn't it be the same everywhere? |
I am not sure if I should mess with all the packages, but if you want to increase node version I will do it |
After all changes and fixes are done, I will prettify commits |
Could you please reference this issue #191 in the commits? |
8abf7ee
to
f901a20
Compare
befa9a9
to
43e6a14
Compare
43e6a14
to
b2d7d13
Compare
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.
Good job! 👏 Thanks for moving this library forward and resolving this big step. I do not see anything that should be addressed now.
I think this is ready to be merged. After that, I will publish the prerelease, and we can test it anywhere we want. Thanks a lot :-)
No description provided.