Automate Linting with Husky and lint-staged to Prevent CI Failures #895
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.
Removing unused TypeScript versions causing conflicts
WalkthroughThis update introduces several changes to the project's linting and formatting configurations. A new 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: 1
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 (21)
- .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)
- examples/react-tldraw/package.json (1 hunks)
- examples/react-todomvc/package.json (1 hunks)
- examples/vanilla-codemirror6/package.json (1 hunks)
- examples/vanilla-quill/package.json (1 hunks)
- lint-staged.config.js (1 hunks)
- package.json (2 hunks)
- packages/devtools/package.json (1 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 (15)
- .eslintignore
- .github/workflows/ci.yml
- .husky/pre-commit
- examples/nextjs-scheduler/package.json
- examples/react-tldraw/package.json
- examples/react-todomvc/package.json
- examples/vanilla-codemirror6/package.json
- examples/vanilla-quill/package.json
- packages/sdk/package.json
- packages/sdk/src/document/crdt/tree.ts
- packages/sdk/src/document/document.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 (7)
examples/nextjs-scheduler/.eslintrc.js (1)
10-10: Consider the implications of disabling '@next/next/no-html-link-for-pages'.Disabling this rule allows for the use of standard HTML
<a>tags instead of Next.js'sLinkcomponent. While this increases flexibility, it could potentially lead to less optimized client-side navigation experiences, as<a>tags do not benefit from Next.js's prefetching capabilities. Ensure that this change aligns with the project's performance and user experience goals.packages/sdk/.eslintrc.js (1)
3-3: Approve the centralization of ESLint configuration but ensure necessary rules are maintained.The move to extend
../../.eslintrc.jshelps in maintaining consistency across sub-projects. However, ensure that any specific rules that are necessary for the SDK's unique requirements are not lost in this transition. The focus on TSDoc is commendable for emphasizing documentation quality.Also applies to: 4-4
packages/devtools/package.json (1)
30-30: Approve the removal of TypeScript from local dependencies.Standardizing TypeScript at the project root level is a strategic move to maintain consistency and avoid version conflicts across sub-projects. This change should help streamline development processes and ensure that all sub-projects are using the same TypeScript version.
lint-staged.config.js (1)
5-18: Well-implemented function for filtering ignored files.The use of
Promise.allto handle multiple asynchronous operations concurrently is appropriate and efficient.package.json (2)
19-21: Scripts for linting and Husky setup are correctly defined.The
lintscript enforces a strict linting policy by treating warnings as errors, which is beneficial for maintaining code quality.
30-43: Dependencies and TypeScript version override are appropriately configured.The new dependencies support the project's enhanced linting capabilities and Husky integration. The TypeScript version override is a good practice to maintain consistency and avoid potential conflicts.
.eslintrc.js (1)
1-61: ESLint configuration is comprehensive and well-structured.The configuration effectively addresses common coding issues and adheres to best practices with the inclusion of plugins like
prettierandjsdoc. This setup should significantly improve code quality and consistency across the project.
| 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.log('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.
Consider improvements in error handling and execution method.
The use of execSync might block the Node.js event loop. Consider using exec for a non-blocking approach. Additionally, improve error handling by logging the error stack or message for better debugging.
|
Previously, we had multiple unused versions of TypeScript in the project, one of which caused conflicts with |
|
@hackerwins |
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:
4. (Added) Relocated TypeScript to Root
In our monorepo setup, we've standardized the TypeScript version to
5.3.3by relocating TypeScript to the rootpackage.json. This change simplifies the management of the TypeScript version across all sub-projects.What are the relevant tickets?
Fixes #888
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores