Added typecheck script and TypeScript configuration improvements#25354
Added typecheck script and TypeScript configuration improvements#25354
Conversation
WalkthroughThis pull request updates configuration and git-ignore files across multiple applications. It adds "types" to the .gitignore files in apps/activitypub, apps/posts, and apps/stats. It adds a new npm script Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Areas to check:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
eac498b to
2d498b1
Compare
jonatansberg
left a comment
There was a problem hiding this comment.
Overall approach looks sound. I have a few comments and questions I'd love feedback on before merging.
| "preview": "vite preview", | ||
| "test:unit": "vitest run" | ||
| "test:unit": "vitest run", | ||
| "typecheck": "tsc -b" |
There was a problem hiding this comment.
How do you feel about calling this check:types and then adding a separate check command that runs both the linting and the type check (and potentially future formatting checks)?
There was a problem hiding this comment.
Yes, or check: parallel(format, lint, typecheck) perhaps?
| "noUncheckedSideEffectImports": true | ||
| }, | ||
| "include": ["src"], | ||
| "include": ["src", "test-utils"], |
There was a problem hiding this comment.
You don't think we can put this in a separate TS config so we don't end up accidentally importing test things within the main app?
There was a problem hiding this comment.
I do think we can, I tried it but unfortunately things got messy, specifically around detecting when to use tsconfig.test.json and how to compose that with tsconfig.app.json and tsconfig.node.json but only under test. For example, tsc wouldn't pickup the tests when type-checking without resolving the config.
In the end I walked it back because it was getting too messy. I can try again if we think its worth the investment?
There was a problem hiding this comment.
We discussed this in our mob session and decided it was worth figuring out, but now isn't the time to do it so we'll loop back around once the whats-new work is complete to implement this.
ref https://linear.app/ghost/issue/BER-2610 - Added typecheck npm script for explicit type checking - Configured TypeScript path aliases (@/* and @test-utils/*) - Added .gitignore entries for generated types directory in admin apps
2d498b1 to
6bc6e19
Compare
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on December 3. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
jonatansberg
left a comment
There was a problem hiding this comment.
Reviewed and approved in mob session
…Ghost#25354) ref https://linear.app/ghost/issue/BER-2610 Adds test directory support to typescript configuration and typechecking command to admin.
ref https://linear.app/ghost/issue/BER-2610
Why are we making this change?
We wanted to add an independent
typecheckscript to validate TypeScript types in theapps/adminpackage and all new React code being added as part of the Ember → React migration.The Problem
Initially, we attempted to use
tsc --noEmitfor type checking, but this doesn't work with TypeScript project references. Theapps/adminpackage uses project references (composite TypeScript configuration) to depend on other packages:apps/admin-x-frameworkapps/postsapps/statsapps/activitypubWith project references, TypeScript needs to emit
.d.tsdeclaration files from referenced projects so the consuming project can type-check against them. This means we must usetsc -b(build mode) instead oftsc --noEmit.The Side Effect
When running
tsc -b, TypeScript builds all the referenced dependencies and generates their declaration files intotypes/directories (as configured in each package'stsconfig.declaration.json). These generated files were appearing as untracked in git.We found an established pattern in the codebase where packages like
admin-x-framework,admin-x-design-system, andshadealready havetypes/in their.gitignore. This PR extends that pattern to the remaining dependencies.What does this PR do?
Adds
.gitignoreentries for generatedtypes/directories in:apps/activitypubapps/postsapps/statsAdds
typechecknpm script toapps/admin/package.json:Configures TypeScript path aliases in
apps/admin/tsconfig.app.json:@/*→./src/*@test-utils/*→./test-utils/*Updates TypeScript includes to include the
test-utilsdirectoryNote
Adds a
typecheckscript, configures TS path aliases and includes in admin, and ignores generatedtypes/in related apps.typecheckscript (tsc -b) inapps/admin/package.json.apps/admin/tsconfig.app.json(@/*,@test-utils/*), includetest-utils, and add reference to../admin-x-framework/tsconfig.declaration.json.types/in.gitignoreforapps/activitypub,apps/posts, andapps/stats.Written by Cursor Bugbot for commit 2d498b1. This will update automatically on new commits. Configure here.