Migrating to eslint.config.js #2522
Conversation
WalkthroughThe pull request introduces several changes across multiple files, primarily focusing on enhancing the documentation and updating the ESLint configuration for improved code quality. A new "Videos" section is added to the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Our Pull Request Approval ProcessWe have these basic policies to make the approval process smoother for our volunteer team. Testing Your CodePlease make sure your code passes all tests. Our test code coverage system will fail if these conditions occur:
The process helps maintain the overall reliability of the code base and is a prerequisite for getting your PR approved. Assigned reviewers regularly review the PR queue and tend to focus on PRs that are passing. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
|
Please fix conflicts |
|
The conflicts occur due to some newly added packages which are necessary for the migration to happen, As I even mentioned in my previously closed PR #2483. |
|
We have a policy of unassigning contributors who close PRs without getting validation from our reviewer team. This is because:
Please be considerate of our volunteers' limited time and our desire to improve our code base. This policy is stated as a pinned post in all our Talawa repositories. Our YouTube videos explain why this practice is not acceptable to our Community. Unfortunately, if this continues we will have to close the offending PR and unassign you from the issue.
|
|
Please fix the conflicting files |
|
Thank you for bringing this to my attention. I understand the importance of following the established process for PR validation and the impact it has on both the review team and the quality of our codebase. I apologize for closing the PR prematurely without proper review. I assure you that I will adhere to the policy moving forward and ensure that all PRs remain open until they have been thoroughly reviewed by the team. I appreciate the time and effort the reviewers put into their work, and I will make sure to respect that process in the future. |
|
|
Now the only conflict that arises is due to the |
|
Please fix the conflicting files |
|
I would like to know how I should continue with this, because resolving the conflicts would require me to remove the |
|
Did you get any feedback from the #talawa-api slack channel? |
|
Okay, I will surely try to seek assistance from there. |
|
I got a suggestion on the slack channel to add the |
|
Make the changes to this PR. It is in scope of what needs to be done to do the migration. |
…lint.config.js file to handle the global variables
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
- eslint.config.mjs (1 hunks)
- package.json (2 hunks)
🧰 Additional context used
🔇 Additional comments (4)
package.json (4)
102-104: LGTM: New ESLint packages added to support migration.The addition of
@eslint/compat,@eslint/eslintrc, and@eslint/jspackages is appropriate for the migration from.eslintrc.jsontoeslint.config.js. These packages provide the necessary tools and APIs to work with the new ESLint configuration format.
Line range hint
102-139: Summary: Package updates align with PR objectives and best practices.The changes in
package.jsoneffectively support the migration toeslint.config.jsby adding necessary ESLint-related packages. The updates tographql-markdown,husky, andlint-stagedfollow best practices for keeping dependencies current. These changes align well with the PR objectives and contribute to maintaining a robust development environment.
138-139: LGTM: Development tools husky and lint-staged updated.The updates to
husky(^9.1.5) andlint-staged(^15.2.10) are patch version increments, which typically include bug fixes and minor improvements. These updates help maintain a robust development environment.It's a good practice to review the changelogs for these updates. You can use the following commands to check for any notable fixes or improvements:
#!/bin/bash # Description: Fetch the changelogs for husky and lint-staged updates echo "Husky changelog:" npm view husky@9.1.5 changelog echo "\nLint-staged changelog:" npm view lint-staged@15.2.10 changelog
137-137: LGTM: graphql-markdown updated to latest minor version.The update of
graphql-markdownfrom^7.0.0to^7.1.0is a good practice to keep dependencies current. This minor version update should include backwards-compatible changes or bug fixes.Please verify the changelog for
graphql-markdownv7.1.0 to ensure there are no breaking changes or significant updates that might affect the project. You can use the following command to check:
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
- eslint.config.mjs (1 hunks)
- package.json (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🔇 Additional comments (5)
eslint.config.mjs (5)
36-58: LGTM: Language options and general rules are well-configuredThe language options and general rules are appropriately set for a Node.js TypeScript project. The rules focus on code quality and TypeScript best practices, which will help maintain a consistent and robust codebase.
59-111: LGTM: Comprehensive TypeScript-specific configurationThe TypeScript-specific configuration is well-structured and comprehensive. It includes appropriate parser options and a detailed set of rules, particularly the naming conventions. This will ensure consistency and best practices across the TypeScript codebase.
112-125: LGTM: GraphQL configuration is properly set upThe configuration for GraphQL files is well-structured, using the appropriate processor and parser from @graphql-eslint. This setup will ensure proper linting of GraphQL schema files in the project.
126-138: LGTM: File-specific configurations are appropriateThe special configurations for test files and specific source files are reasonable. Disabling certain rules for these files can be necessary to accommodate their unique requirements.
However, it's important to be cautious when disabling rules for specific files to avoid introducing inconsistencies in the codebase. Regularly review these exceptions to ensure they remain necessary. Consider adding comments explaining why these rules are disabled for these specific files.
19-35:⚠️ Potential issueQuote the 'import' property name to avoid syntax errors
The 'import' property in the plugins configuration should be quoted to avoid potential syntax errors, as it's a reserved keyword in JavaScript.
Apply this change:
- "import": fixupPluginRules(_import), + "import": fixupPluginRules(_import),The rest of the global configuration, including ignored directories and extended configs, looks appropriate and follows good practices.
Likely invalid or redundant comment.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2522 +/- ##
========================================
Coverage 98.57% 98.57%
========================================
Files 355 355
Lines 17996 17996
Branches 2400 2400
========================================
Hits 17740 17740
Misses 253 253
Partials 3 3 ☔ View full report in Codecov by Sentry. |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
eslint.config.mjs (5)
20-43: LGTM: Base configuration is well-structuredThe base configuration is comprehensive and well-structured. It correctly sets up ignores, extends recommended configs, and configures plugins and language options appropriately for a TypeScript project in a Node.js environment.
Consider adjusting the indentation of the
envandparserproperties withinlanguageOptionsfor better readability:languageOptions: { - env: { - node: true, - }, - parser: tsParser, -} + env: { + node: true, + }, + parser: tsParser, + },
45-60: LGTM with suggestion: Global rules are comprehensiveThe global rules are well-defined and appropriate for maintaining code quality in a TypeScript project. However, the
no-restricted-importsrule might be too strict.Consider modifying the
no-restricted-importsrule to allow local imports:- "no-restricted-imports": ["error", { - patterns: ["**/src/**"], - }], + "no-restricted-imports": ["error", { + patterns: [{ + group: ["**/src/**"], + message: "Please use relative imports for local files." + }], + }],This change will still discourage absolute imports from
src, but with a more informative message, and won't break legitimate local imports.
61-113: LGTM with suggestion: TypeScript-specific configuration is robustThe TypeScript-specific configuration is comprehensive and follows best practices. The naming conventions are well-defined and should promote consistency across the codebase.
Consider relaxing the
@typescript-eslint/explicit-function-return-typerule to allow inference in some cases:- "@typescript-eslint/explicit-function-return-type": "error", + "@typescript-eslint/explicit-function-return-type": ["error", { + allowExpressions: true, + allowTypedFunctionExpressions: true, + }],This change will still enforce explicit return types in most cases but allow inference for simple expressions and typed function expressions, which can improve code readability in some situations.
114-126: LGTM with suggestion: GraphQL configuration is set up correctlyThe configuration for GraphQL files is correctly set up with the appropriate processor and plugin.
Consider adding some GraphQL-specific rules to enhance the linting of GraphQL files. For example:
languageOptions: { parser: parser, }, +rules: { + "@graphql-eslint/no-anonymous-operations": "error", + "@graphql-eslint/naming-convention": [ + "error", + { + OperationDefinition: { + style: "PascalCase", + forbiddenPrefixes: ["Query", "Mutation", "Subscription"], + forbiddenSuffixes: ["Query", "Mutation", "Subscription"], + }, + }, + ], +},These rules will enforce naming conventions and prevent anonymous operations in your GraphQL schemas.
127-132: LGTM with suggestion: Test file configuration could be expandedThe configuration correctly disables the
no-restricted-importsrule for test files, allowing necessary flexibility.Consider adding more test-specific configurations to enhance the linting of test files. For example:
files: ["tests/**/*"], rules: { "no-restricted-imports": "off", + "@typescript-eslint/no-explicit-any": "off", + "@typescript-eslint/no-non-null-assertion": "off", + "jest/valid-expect": "error", + "jest/no-disabled-tests": "warn", },These additional rules will provide more flexibility for test files while still maintaining good practices. Remember to install and configure the
eslint-plugin-jestif you decide to use Jest-specific rules.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- eslint.config.mjs (1 hunks)
🧰 Additional context used
🔇 Additional comments (3)
eslint.config.mjs (3)
1-18: LGTM: Imports and initial setup are correctThe imports and initial setup look good. All necessary plugins and utilities are imported, and the FlatCompat instance is correctly set up for backwards compatibility with older ESLint configs.
1-139: Overall: Excellent ESLint configuration with minor suggestions for improvementThis ESLint configuration is well-structured and comprehensive, covering TypeScript, GraphQL, and test files effectively. It sets up a robust set of rules that should maintain high code quality standards across the project.
Key strengths:
- Comprehensive ignore patterns
- Well-defined TypeScript-specific rules and naming conventions
- Proper setup for GraphQL linting
- Flexibility for test files
Suggestions for improvement:
- Consider relaxing the
no-restricted-importsrule slightly- Add more GraphQL-specific rules
- Expand test-specific configurations
- Clarify the need for specific file overrides
Overall, this configuration provides a solid foundation for maintaining code quality in your project. Implementing the suggested improvements will further enhance its effectiveness and flexibility.
133-139: Please clarify: Specific file overridesThe configuration includes rule overrides for specific files. While this may be necessary, it's important to understand and document the reasons for these exceptions.
Could you please provide more context on why these specific files need these rules turned off? This will help ensure that the exceptions are justified and maintain code quality across the project.
To help verify the necessity of these overrides, you can run the following script:
This script will help identify if these files contain functions that might trigger the disabled rules, justifying the overrides.
✅ Verification successful
Rule Overrides May Be Unnecessary
The verification script did not find any functions with implicit return types or empty functions in
src/index.tsandsrc/utilities/copyToClipboard.ts. Therefore, the ESLint rule overrides for these files may not be needed and can potentially be removed to maintain consistent code quality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of implicit return types and empty functions in specified files echo "Checking src/index.ts:" ast-grep --lang typescript --pattern 'function $FUNC_NAME($_) { $$$ }' src/index.ts echo "\nChecking src/utilities/copyToClipboard.ts:" ast-grep --lang typescript --pattern 'function $FUNC_NAME($_) { $$$ }' src/utilities/copyToClipboard.tsLength of output: 345
|
I have performed the migration, Kindly review |
What kind of change does this PR introduce?
Bug fix
Issue Number:
Fixes #2472
Did you add tests for your changes?
Not relevant
Snapshots/Videos:
If relevant, did you update the documentation?
Not relevant here.
Summary
This PR migrates the deprecated version of
eslintrc.jsontoeslint.config.js.Now eslint will correctly fix all the linting issues which was not the case earlier as mentioned in the screenshot of the issue raised. Now this PR is in reference to an earlier PR which was closed due to some dependency issues.
Does this PR introduce a breaking change?
Not sure if relevant here.
Other information
None
Have you read the contributing guide?
Yes
Summary by CodeRabbit
New Features
Bug Fixes
Chores