Automate Linting with Husky and lint-staged to Prevent CI Failures #896
Conversation
- Ensured all sub-projects follow the unified ESLint rules from the root configuration. - Moved .eslintignore to the project root for centralized management. - Updated scripts to reference the root .eslintignore file directly. - Simplified path handling logic by removing unnecessary relative path calculations.
WalkthroughThis update introduces several configuration files and modifications aimed at enhancing the linting process in a JavaScript and TypeScript project. Key changes include the addition of Changes
Assessment against linked issues
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
Files selected for processing (16)
- .eslintignore (1 hunks)
- .eslintrc.js (1 hunks)
- .github/workflows/ci.yml (1 hunks)
- .husky/pre-commit (1 hunks)
- examples/nextjs-scheduler/.eslintrc.js (1 hunks)
- examples/nextjs-scheduler/package.json (1 hunks)
- lint-staged.config.js (1 hunks)
- package.json (2 hunks)
- packages/sdk/.eslintrc.js (1 hunks)
- packages/sdk/package.json (3 hunks)
- packages/sdk/src/document/crdt/tree.ts (1 hunks)
- packages/sdk/src/document/document.ts (3 hunks)
- packages/sdk/src/document/json/array.ts (4 hunks)
- packages/sdk/src/util/error.ts (1 hunks)
- packages/sdk/test/helper/helper.ts (1 hunks)
- packages/sdk/test/unit/document/crdt/counter_test.ts (1 hunks)
Files skipped from review due to trivial changes (10)
- .eslintignore
- .github/workflows/ci.yml
- .husky/pre-commit
- examples/nextjs-scheduler/package.json
- packages/sdk/package.json
- packages/sdk/src/document/crdt/tree.ts
- packages/sdk/src/document/json/array.ts
- packages/sdk/src/util/error.ts
- packages/sdk/test/helper/helper.ts
- packages/sdk/test/unit/document/crdt/counter_test.ts
Additional comments not posted (6)
examples/nextjs-scheduler/.eslintrc.js (2)
2-2: Approved: Extension of ESLint configuration.Extending from
../../.eslintrc.jsis a good practice for maintaining consistency across the project.
10-10: Verify the rationale for disabling '@next/next/no-html-link-for-pages'.Turning off this rule allows more flexibility but could lead to practices that are less optimized for Next.js. It's important to ensure that this change aligns with the project's goals.
packages/sdk/.eslintrc.js (1)
3-4: Approved: Simplified ESLint configuration with a suggestion to verify adequacy.Extending from
../../.eslintrc.jsand focusing on TypeScript documentation witheslint-plugin-tsdocis a streamlined approach. Ensure that this configuration adequately covers all SDK-specific linting requirements.package.json (1)
Line range hint
20-35: Approved: Addition of linting scripts and dependencies.The new
lintandpreparescripts, along with the added dependencies, are essential for the automated linting process. Verify that these configurations are correctly integrated and functioning as intended.lint-staged.config.js (1)
5-18: Well-implemented function for filtering ignored files.The
removeIgnoredFilesfunction correctly uses ESLint's API to filter out files that are specified in the.eslintignorefile. The use ofPromise.allfor handling multiple asynchronous operations is efficient and appropriate.packages/sdk/src/document/document.ts (1)
Line range hint
441-545: Formatting Improvements in Type Definitions: Enhanced ReadabilityThe changes to the indentation levels in the type definitions significantly improve the readability and clarity of the code. This is particularly important in a file with complex logic and numerous type interactions. Here are a few points to consider:
- Consistency: Ensure that similar formatting improvements are applied consistently across all type definitions in the project to maintain uniformity.
- Review: While the changes are beneficial, it's crucial to review them thoroughly to ensure that no syntactical errors have been introduced and that they align with the project's coding standards.
Overall, these formatting adjustments are a positive step towards better code maintainability and readability.
| module.exports = { | ||
| '**/*.ts': async (files) => { | ||
| const filesToLint = await removeIgnoredFiles(files); | ||
|
|
||
| if (filesToLint.length > 0) { | ||
| const fileArgs = filesToLint.join(' '); | ||
| const command = `pnpm exec eslint ${fileArgs} --fix --max-warnings=0 --ext .ts`; | ||
| try { | ||
| execSync(command, { stdio: 'inherit' }); | ||
| process.exit(0); | ||
| } catch (error) { | ||
| console.error('Linting failed. Commit will be aborted.'); | ||
| process.exit(1); | ||
| } | ||
| } else { | ||
| console.log('No eligible files to lint. Skipping lint-staged command.'); | ||
| process.exit(0); | ||
| } | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Robust linting configuration for TypeScript files.
The module export configuration for linting TypeScript files is well-implemented. The use of pnpm exec eslint with the --fix and --max-warnings=0 flags ensures that linting issues are automatically fixed and that no warnings are allowed.
Suggestion: Consider handling the process exit more gracefully by allowing other middleware in lint-staged to continue running if needed, rather than using process.exit() which stops all processes immediately.
| module.exports = { | ||
| root: true, | ||
| plugins: ['prettier', 'jsdoc', '@typescript-eslint'], | ||
| extends: [ | ||
| 'eslint:recommended', | ||
| 'plugin:@typescript-eslint/eslint-recommended', | ||
| 'plugin:@typescript-eslint/recommended', | ||
| ], | ||
| rules: { | ||
| 'prettier/prettier': 'error', | ||
| 'object-shorthand': ['error', 'always'], | ||
| 'no-unreachable': 'error', | ||
| }, | ||
| overrides: [ | ||
| { | ||
| files: ['**/*.ts', '**/*.tsx'], | ||
| parser: '@typescript-eslint/parser', | ||
| plugins: ['@typescript-eslint'], | ||
| extends: ['plugin:@typescript-eslint/recommended'], | ||
| rules: { | ||
| '@typescript-eslint/no-non-null-assertion': 'off', | ||
| '@typescript-eslint/no-explicit-any': 'off', | ||
| '@typescript-eslint/ban-ts-comment': 'off', | ||
| 'jsdoc/require-jsdoc': [ | ||
| 'error', | ||
| { | ||
| contexts: ['MethodDefinition:not([accessibility="private"])'], | ||
| require: { | ||
| ClassDeclaration: true, | ||
| }, | ||
| checkConstructors: false, | ||
| enableFixer: false, | ||
| }, | ||
| ], | ||
| '@typescript-eslint/naming-convention': [ | ||
| 'error', | ||
| { | ||
| selector: 'variable', | ||
| format: ['camelCase', 'PascalCase'], | ||
| leadingUnderscore: 'allowDouble', | ||
| trailingUnderscore: 'allowDouble', | ||
| }, | ||
| ], | ||
| '@typescript-eslint/ban-types': [ | ||
| 'error', | ||
| { | ||
| types: { null: 'Use undefined instead of null' }, | ||
| }, | ||
| ], | ||
| '@typescript-eslint/array-type': ['error', { default: 'generic' }], | ||
| '@typescript-eslint/no-this-alias': [ | ||
| 'error', | ||
| { | ||
| allowDestructuring: true, | ||
| allowedNames: ['node'], | ||
| }, | ||
| ], | ||
| }, | ||
| }, | ||
| ], | ||
| }; |
There was a problem hiding this comment.
Review of ESLint Configuration: Generally Solid with Suggestions for Improvement
The ESLint configuration is well-structured and covers a broad range of rules that are essential for maintaining code quality and consistency, especially in a TypeScript project. Here are some observations and suggestions:
- TypeScript Rules: The configuration disables some TypeScript rules (
@typescript-eslint/no-non-null-assertion,@typescript-eslint/no-explicit-any, and@typescript-eslint/ban-ts-comment). While this may offer flexibility, it could compromise type safety. Consider enabling these rules if the project aims for strict type safety. - JSDoc Rules: The inclusion of JSDoc rules (
jsdoc/require-jsdoc) is commendable as it enforces documentation, which is crucial for maintainability and clarity. - Prettier Integration: Enforcing
prettierrules as errors is a good practice to ensure consistent code formatting.
Overall, the setup is robust, but revisiting the disabled TypeScript rules could further enhance the project's code quality.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- .eslintignore (1 hunks)
- examples/nextjs-scheduler/.eslintrc.js (1 hunks)
Files skipped from review due to trivial changes (1)
- .eslintignore
Files skipped from review as they are similar to previous changes (1)
- examples/nextjs-scheduler/.eslintrc.js
What this PR does / why we need it?
This PR addresses the issue where linting was not being automatically executed before commits, despite
Huskybeing installed. The previous setup hadHuskyinstalled at v8, but properpre-commithooks were not in place to ensure that linting occurred consistently. SinceHuskyv5, the configuration and usage have changed. For more details, refer to the the documentation of Husky.Key changes include:
pre-commitconfiguration in the.huskyfolder, and ensureHuskyworks as expected to automate linting at thepre-commitstage.lint-stagedto only lint the files that arestagedfor commit, improving efficiency.With these updates, developers will no longer need to manually run lint checks, as the process will be automated and enforced during the commit phase.
Any background context you want to provide?
This PR brings several improvements to the linting setup, along with updates to the
Huskyandlint-stagedconfigurations.1. Relocate
lintCommand to RootAs part of the monorepo migration (issue #648), the
lintcommand was previously located in thesdkpackage, requiring developers to runpnpm sdk lint. However, this command is intended to lint the entire project, not just thesdk.To address this, I’ve moved the lint command to the root
package.json. Now, you can simply runpnpm lintto lint the whole project.2. Ignore files in
.eslintignoreinlint-stagedTo prevent unnecessary files from being linted, we use an
.eslintignorefile to specify which paths should be excluded. Sincelint-stageddoesn’t automatically respect.eslintignoreby default, I’ve added thelint-staged.config.jsfile to ensure that the ignore rules are applied. This allows us to avoid linting files specified in.eslintignoreduring thepre-commitstage.Alternatively, if we want to eliminate this step, we would need to upgrade
ESLintto version8.5.0or later (we are currently using8.19.0). For more information, refer to the lint-staged documentation.Additionally, I’ve moved the
.eslintignorefile from thesdkpackage to the root, so it now applies to the entire monorepo. If you need to exclude specific files in other sub-projects, you can add them to this file.3. Centralize ESLint Configuration in the Root
Since we are now working in a monorepo, it makes sense to centralize
ESLintrules in a single configuration file at the root. To achieve this, I’ve created a root.eslintrc.jsfile, and sub-projects likesdkcan extend from it. This ensures consistency across the entire codebase while allowing for project-specific overrides.Here’s an example of how the sdk package extends the root ESLint configuration:
What are the relevant tickets?
Fixes #888
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Style