Migrate to ESLint flat config #583
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #583 +/- ##
==========================================
+ Coverage 84.14% 84.19% +0.05%
==========================================
Files 214 214
Lines 5777 5772 -5
Branches 587 586 -1
==========================================
- Hits 4861 4860 -1
+ Misses 916 912 -4 ☔ View full report in Codecov by Sentry. |
|
I don't think enabling the hooks check is going to be realistic in this project 😬 |
sergei-maertens
commented
Jan 3, 2025
There was a problem hiding this comment.
So, conclusions of this POC:
- IMO it's viable to switch to flat config
- I see quite some opportunities to expose the configs from a centralized
@maykin/eslint-configpackage with sufficient room for overrides - in combination with (unmaintained) CRA you end up with some duplicated dependencies/eslint plugins because CRA itself pulls in older versions. I'm afraid the long-term solution for this is migrating from CRA to ViteJS if the node_modules size is a problem.
- eslint isn't the fastest, might be an option to enable the cache (it's disabled by default)
frontend/src/components/DestructionListAuditLog/DestructionListAuditLogDetails.stories.tsx
Show resolved
Hide resolved
sergei-maertens
force-pushed
the
experiment/582-eslint-flat-config
branch
from
b67b7b4 to
7f1515b
Compare
frontend/package.json
Outdated
Comment on lines
25
to
31
| "start": "DISABLE_ESLINT_PLUGIN=true react-scripts start", | ||
| "build": "DISABLE_ESLINT_PLUGIN=true react-scripts build", | ||
| "test": "react-scripts test", | ||
| "test:coverage": "npm test -- --coverage --watchAll=false", | ||
| "eject": "react-scripts eject", | ||
| "storybook": "storybook dev -p 6006", | ||
| "build-storybook": "storybook build", | ||
| "storybook": "DISABLE_ESLINT_PLUGIN=true storybook dev -p 6006", | ||
| "build-storybook": "DISABLE_ESLINT_PLUGIN=true storybook build", |
There was a problem hiding this comment.
I think so, I don't really like "stuffing" package.json with envars like this. The .env fie is already use to control environment settings on development and I see a .env.production.template so we might need to align this to get this working properly without breaking stuff.
Can you align with @SilviaAmAm
I suggeset enabling it then comment it out, I might want to have a go at this. |
Nice work
Lets roll
The overrides are a requirements so cool. One thing too may attention to is that these packages should remains thin. We don't want to end up with a default project that contains lots of stuff we might have used 8 years ago but not anymore. Lets keep it bare bones only.
Yes, out of scope for now I would like to migrate in the future.
Lets keep it disabled as I see various caching issues in various tools/softwares quite regularly which are incredible difficult to debug if you don't know where to look (removing node_modules/.cache/) typically works. |
sergei-maertens
force-pushed
the
experiment/582-eslint-flat-config
branch
4 times, most recently
from
7fd21a1 to
7e80c8a
Compare
sergei-maertens
changed the title
sergei-maertens
commented
Jan 6, 2025
CRA ships with a default ESLint config, which is overridden in the project to disable some bits. ESLint is moving towards flat config, and CRA (unmaintained) is not compatible with it, so we set up our own flows. The envvar disables the CRA integration in react-scripts, as it does not pick up the new config file. The pre-commit checks and CI pipeline ensure that code is properly linted before being committed. We also use the shared config from Maykin for consistency across projects.
Addressed the linter errors to minimize the impact on the project. Some lines are ignored because they are valid and consistent in the project, but ignoring the rule as a whole would allow other mistakes to fall through/go undetected.
sergei-maertens
force-pushed
the
experiment/582-eslint-flat-config
branch
from
7e80c8a to
729e216
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ESLint 8/9 flat config, with more plugins/strictness based on config from open-forms-sdk
TODO:
Check if we can enable hook checksnoperesolves #582