-
Notifications
You must be signed in to change notification settings - Fork 17
build: fix pre-commit TypeScript type checking #191
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The pre-commit hook previously ran `yarn tsc` which does nothing meaningful because the root tsconfig.json has `"files": []` with only project references. This meant TypeScript errors weren't being caught locally but would fail in CI. Changes: - Use `yarn tsc --build` to properly build all project references - When WASM modules aren't built, check independent projects (core, website, engine2, xmutil-js) that don't depend on WASM - Fix rspress.config.ts type error by migrating from deprecated `analytics` config to `builderPlugins` with rsbuild-plugin-google- analytics The rspress 1.44.0 no longer supports the `analytics` top-level config property. Google Analytics must now be configured via the rsbuild-plugin-google-analytics plugin through the builderPlugins config.
Add gitignore patterns for files generated by `yarn tsc --build`: - *.tsbuildinfo files - Generated .js, .js.map, .d.ts in src/engine/ and src/importer/ - Generated .js files in website/ These artifacts are created when running type checking with the --build flag and should not be committed.
Code Review for PR #191: build: fix pre-commit TypeScript type checkingSummaryThis PR addresses a critical issue where TypeScript type checking was not actually running in the pre-commit hook. The root cause was using yarn tsc instead of yarn tsc --build when the root tsconfig.json only contains project references. Strengths
Observations and Suggestions1. Pre-commit script logic (scripts/pre-commit:80-147)Good: The script checks for WASM availability and handles both cases appropriately. Minor concern: The script checks 4 independent projects (core, website, engine2, xmutil-js) when WASM is not built, but it is unclear if this covers all possible TypeScript errors.
Recommendation: Consider adding a comment in the script explaining which projects depend on WASM and why they are skipped, to help future maintainers understand the trade-off. 2. Gitignore patterns (.gitignore:94-103)Good: Comprehensive patterns for build artifacts. Question: The pattern /website/docs/**/*.js suggests JavaScript files might be generated in the docs directory. Is this expected? If docs should only contain markdown/static content, this might indicate an unusual build configuration worth investigating. 3. Error handling consistencyObservation: The script uses a TSC_FAILED flag and continues checking all 4 projects even after one fails, which is good for developer experience (shows all errors at once). However, the temporary file handling could be slightly more robust by using separate temp files for each project check to avoid potential race conditions if the checks were ever parallelized in the future. 4. Package dependency (website/package.json)Good: Adding rsbuild-plugin-google-analytics is the correct migration path. Minor style note: The new dependency is listed before @emotion/styled in the diff, which suggests it might not be alphabetically sorted. This is very minor and depends on your project conventions. Testing RecommendationsConsider adding these scenarios to your testing:
Code Quality Assessment
VerdictLGTM with minor suggestions This is a solid fix that addresses a real problem. The fallback logic for missing WASM is pragmatic. The only substantive concern is documenting which projects cannot be fully type-checked without WASM, but this does not block merging since it is an improvement over the current state (no type checking at all). The commit messages follow the project style guide perfectly, and the changes align well with the development workflow described in CLAUDE.md. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #191 +/- ##
==========================================
+ Coverage 74.33% 76.31% +1.97%
==========================================
Files 46 45 -1
Lines 15262 15176 -86
==========================================
+ Hits 11345 11581 +236
+ Misses 3917 3595 -322 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The pre-commit hook previously ran
yarn tscwhich does nothingmeaningful because the root tsconfig.json has
"files": []with onlyproject references. This meant TypeScript errors weren't being caught
locally but would fail in CI.
Changes:
yarn tsc --buildto properly build all project referenceswebsite, engine2, xmutil-js) that don't depend on WASM
analyticsconfig tobuilderPluginswith rsbuild-plugin-google-analytics
The rspress 1.44.0 no longer supports the
analyticstop-level configproperty. Google Analytics must now be configured via the
rsbuild-plugin-google-analytics plugin through the builderPlugins
config.