[Draft] | Support flat eslint configuration format for nimble packages#2718
[Draft] | Support flat eslint configuration format for nimble packages#2718gokulprasanth-ni wants to merge 8 commits intomainfrom
Conversation
Signed-off-by: gokulprasanth-ni <131153319+gokulprasanth-ni@users.noreply.github.com>
Signed-off-by: gokulprasanth-ni <131153319+gokulprasanth-ni@users.noreply.github.com>
Signed-off-by: gokulprasanth-ni <131153319+gokulprasanth-ni@users.noreply.github.com>
Signed-off-by: gokulprasanth-ni <131153319+gokulprasanth-ni@users.noreply.github.com>
|
|
||
| attr({ attribute: 'fractional-width', converter: nullableNumberConverter })( | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-argument | ||
|
|
There was a problem hiding this comment.
This applies to all instances where we leave a line space where you removed the lint suppression.
gokulprasanth-ni
requested review from
TrevorKarjanis,
atmgrifter00,
fredvisser,
jattasNI,
msmithNI,
munteannatan and
rajsite
as code owners
| @@ -0,0 +1,7 @@ | |||
| { | |||
There was a problem hiding this comment.
Moved the PR to draft because like described in this comment lets do the nimble transition after AzDo. We should iterate in AzDo first and we don't have the time to review the change in nimble right now.
There was a problem hiding this comment.
Milan, I want to ensure I understood correctly. Are you suggesting that we update the SLE apps first, followed by the Nimble update? Additionally, in your other comment, you mentioned updating the Azdo pipeline—could you please clarify what specific updates are required there?
There was a problem hiding this comment.
Are you suggesting that we update the SLE apps first
yep!
mentioned updating the Azdo pipeline—could you please clarify what specific updates are required there?
sorry wasn't being clear, the SLE apps is what I meant. Not aware of any azdo pipeline changes.
There was a problem hiding this comment.
fyi @gokulprasanth-ni as part of regular dependency updates we pulled in the the stylistic eslint changes (from pre v9) into nimble. We also made some intentional rule tweaks which are now merged in main. Just an fyi when the eslint v9 upgrade is revisited.
Hopefully now there will be far fewer changes in the PRs as the stylistic update was merged separately
There was a problem hiding this comment.
The goal of splitting the prettier update to a separate PR was to reduce the number of formatting changes in this PR. The assumption was that prettier is what caused so many changes but there are still over 100 files changed in this PR many with just formatting changes. Does splitting the prettier update into a separate PR actually make a difference?
There was a problem hiding this comment.
@rajsite @jattasNI I tried updating the prettier-eslint and prettier-eslint-cli packages version in the Nimble monorepo to make them compatible with ESLint v9. Following this upgrade, we tried enabling ESLint and Prettier for linting across several packages in the workspace.
Problem:
During this update, they were conflicts between ESLint and Prettier that create a circular fixing problem:
- ESLint runs first and reports certain formatting/style violations
- Prettier runs subsequently and reports different or conflicting style issues
- When we fix the ESLint errors, those changes are flagged as violations by Prettier
- Conversely, when we fix Prettier issues, ESLint reports new violations
- This creates an endless loop where fixing one tool's issues breaks the other tool's rules
Proposed Solutions:
We're considering the following approaches to resolve this conflict:
-
Disable ESLint style rules: Turn off all style/formatting-related rules in ESLint (such as indent, quotes, semi, comma-dangle, etc.) and rely exclusively on Prettier for code formatting. This would allow ESLint to focus on code quality and potential bugs while Prettier handles all stylistic concerns.
-
Use
eslint-plugin-prettier: Integrate Prettier directly into ESLint by usingeslint-plugin-prettieralong witheslint-config-prettier. This approach:- Runs Prettier as an ESLint rule
- Automatically disables conflicting ESLint formatting rules via eslint-config-prettier
- Reports Prettier formatting issues as ESLint errors
- Provides a single, unified linting experience with no conflicts
But we are still not sure whether these approaches works fine or not. We would like to get your thoughts on this and also any alternate solutions available to fix this issue.
There was a problem hiding this comment.
Looking at it again I'm not sure why we call both prettier-eslint and eslint via the cli. I think we can just prettier-eslint which is aware of the eslint configuration already.
There was a problem hiding this comment.
Closing this PR as it will be handled in the below PR
…nt-v9 Signed-off-by: gokulprasanth-ni <131153319+gokulprasanth-ni@users.noreply.github.com>
Signed-off-by: gokulprasanth-ni <131153319+gokulprasanth-ni@users.noreply.github.com>
Signed-off-by: gokulprasanth-ni <131153319+gokulprasanth-ni@users.noreply.github.com>
# Pull Request ## 🤨 Rationale Adopts eslint v9 🎉 ## 👩💻 Implementation - Switches to ni eslint config packages using flat config - Removes `prettier-eslint` which is in [alpha builds](https://github.com/prettier/prettier-eslint/releases?q=v17&expanded=true) for eslint v9 support, significantly hurt lint performance, and had [major conflicts with eslint](#2718 (comment)). Now that eslint libraries like [stylistic](https://eslint.style/) are dedicated to style rules and eslint supports more formats such as [markdown syntax](https://github.com/eslint/markdown) and [markdown style](https://github.com/ota-meshi/eslint-plugin-markdown-preferences?tab=readme-ov-file) it's possible we don't need to rely on prettier long term. Either way we remove prettier for now to unblock eslint upgrades and can reconsider in the future - Linting performance was significantly improved so simplified the `validate` concurrency scripts to run all lint sequentially in parallel to tests. Now `validate` completes ~100s faster. It's also possible we will get another speed bump enabling [multithreaded eslint](https://eslint.org/blog/2025/08/multithread-linting/) but will save for a follow-up PR. - In the nimble private `eslint-config-nimble` package: - Created a shared `lintNimbleConfig` configuration that is used in each eslint config to make sure the eslint config file itself is linted and to give a spot for very general shared config in follow-up PRs (like markdown or json linting) - Created an angular lint similar to our typescript / component configs as shared config for the angular projects. This resolves issues where extending from a base config in the angular workspace is technically crossing a library boundary and triggered lint errors about reaching across package boundaries. Now we explicitly depend on a shared package. - Angular workspace changes: - The angular workspace root config is now for the config file in root, not the angular projects. - Each angular project uses the shared `angularTypescriptNimbleConfig` and `angularTemplateNimbleConfig` - Root workspace changes: - Added a top-level eslint config file for the config in root (the config file itself and beachball) but could also be used for linting markdown, etc. in the future. - Having a root eslint config also prevents the eslint vscode plugin from getting confused (errors in extension output logs) searching for the eslint config for files in root. ## 🧪 Testing Note, see #2774 for CI results and source changes resulting from this config change. ## ✅ Checklist - [x] I have updated the project documentation to reflect my changes or determined no changes are needed. **A follow-up PR will remove prettier references and docs** --------- Signed-off-by: gokulprasanth-ni <131153319+gokulprasanth-ni@users.noreply.github.com> Co-authored-by: gokulprasanth-ni <131153319+gokulprasanth-ni@users.noreply.github.com> Co-authored-by: rajsite <1588923+rajsite@users.noreply.github.com>
3 participants
Pull Request
🤨 Rationale
👩💻 Implementation
javascript-styleguidepackages.🧪 Testing
✅ Checklist