Repository navigation
[No QA] Consolidate TypeScript projects under a root project with subproject references - #100553
Conversation
Make the root `tsconfig.json` a solution-style config that references every project instead of being the app program itself. Splitting the Jest project out of the root config (#100124) left files under `tests/` and `jest/` outside every project, so editors resolved them to an inferred project with no `paths` and `@libs`/`@src` stopped resolving. A solution-style root fixes that: the language service finds this config, sees it owns no files, and walks `references` to the project that does. - Move the app program to `tsconfig.app.json` and leave `tsconfig.json` with `files: []` plus `references`. - Point `npm run typecheck` at those references, so a project joins CI by being referenced and nowhere else, and run `tsc --build` for the build-mode up-to-date check. - Drop the dead includes phase 3 called for: `website` (does not exist), `docs` (already excluded), `.claude` (was pulling in unrelated `.mjs` files) and `**/*.nitro/*` (already covered by `modules`). - List the files the app import graph already reached (`wdyr.ts`, `config/proxyConfig.ts`, `config/rsbuild`, `__mocks__/reportData`) and move `prompts` to the Bun program, which is what actually consumes it.
|
@codex review |
|
@MelvinBot review |
|
|
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@ikevin127 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
roryabraham
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 793d63e366
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ".github", | ||
| "web/proxy.ts", | ||
| "config" | ||
| "files": [], |
There was a problem hiding this comment.
With files: [], running the deliberately installed bare npx tsc now exits successfully without checking any referenced project; I confirmed tsc -p tsconfig.json --listFiles reports no files. This contradicts scripts/postInstall.sh:34-36, which explicitly makes bare npx tsc match npm run typecheck, and can give developers a false clean result. The compiler help confirms that only --build “build[s] one or more projects and their dependencies,” while normal project mode compiles just the selected project, so references are not traversed here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Legit, and the citation is accurate — fixed in 6884ceb.
Confirmed both halves:
tsc -p tsconfig.json --listFilesreports 0 files, and baretsc -p tsconfig.jsonexits 0 with no output. Project mode does not followreferences; only--builddoes.scripts/postInstall.shreally did promisenpx tsc"matchesnpm run typecheck", so that comment was stale the moment the root became a solution.
I also checked whether "include": [] would at least make it fail loudly instead of silently. It does not — same silent exit 0. So there is no config-level fix; the promise itself had to go.
What I did not do, and why: I tested keeping the app program on the root config alongside references, which would have preserved bare tsc. tsserver still solution-searches in that shape — tests/unit/ActiveSpansTest.ts still resolved to tsconfig.jest.json — so it would have worked for the IDE fix too. I went with the solution-style root anyway because that shape gives one file two jobs: scripts/typecheck.ts could no longer derive its project list from references alone (the root's own program is not in its own references), and #99495 splits the app program into web + native next, which would have to dismantle it again. Happy to revisit if you'd rather keep bare tsc meaningful.
The symlink still earns its keep — it forces TypeScript 7 over the tsc bin @typescript/old ships — so I kept it and corrected the claim around it:
# Force the TypeScript 7 native compiler, so `npx tsc` is the compiler `npm run typecheck` runs.
#
# It is not the same *command*: the root tsconfig.json is a solution that lists projects and owns no
# files, and project mode does not follow `references`. `npx tsc` at the repo root therefore checks
# nothing and exits 0. Use `npx tsc --build`, which walks the references, or `npm run typecheck`.For what it's worth npx tsc --build with no project argument does the right thing at the root: it walks all five references, and I verified it exits 1 on a planted error. Nothing in the repo invokes bare tsc (only scripts/typecheck.ts, by explicit path, with --build), and CLAUDE.md:72 already points at npm run typecheck, so the exposure was a human at a terminal — which is exactly the case the comment now covers.
The root config is a solution now, and project mode does not follow `references`, so `npx tsc` at the repo root checks nothing and exits 0. The symlink still matters (it forces TypeScript 7 over the `tsc` bin that `@typescript/old` ships), but it no longer makes bare `tsc` equivalent to `npm run typecheck`. Point at `tsc --build` instead.
| // Not app directories, but the app program reaches into them: `src/App.tsx` imports | ||
| // `wdyr`, `src/libs/ApiUtils` imports `config/proxyConfig`, `.storybook` imports the | ||
| // Rsbuild config, and the Storybook stories import the shared report fixtures. They were | ||
| // already being type-checked here via those imports; listing them makes that visible. |
There was a problem hiding this comment.
NAB: this appears red since comments aren't valid in JSON. Not sure if it matters of if there's a workaround.
There was a problem hiding this comment.
tsconfig is jsonc (even though it doesn't have that extension). So it allows comments and trailing commas.
|
🚧 roryabraham has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/roryabraham in version: 9.4.74-0 🚀
|
|
Deploy Blocker #100698 was identified to be related to this PR. |
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.4.74-2 🚀
|
Explanation of Change
Consolidate the per-runtime TypeScript projects under the root
tsconfig.jsonusing Project References.The bug this fixes
Splitting the Jest project out of the root config in #100124 removed
tests,jestand__mocks__from the roottsconfig.json'sinclude. A language service resolves a file to the closesttsconfig.json, so those files still landed on the root config, did not appear in it, and fell back to an inferred project — which has nopaths. That is why@libsstopped resolving intests/.Reproduced by driving
tsserverdirectly, before and after. The fix is a root project that owns no files, only project references. So the language service walksreferencesto the project that does own the file, and type-checks it with that project's options.Changes
npm run typechecktakes its project list from thosereferences. It still runs each project in parallel for performance (tsc by default is sequential).website(does not exist),docs(already excluded),.claude(was pulling two unrelated.mjsfiles into the app program),**/*.nitro/*(already covered bymodules).wdyr.ts(fromsrc/App.tsx),config/proxyConfig.ts(fromsrc/libs/ApiUtils.ts),config/rsbuild(from.storybook),__mocks__/reportData(from the Storybook stories).promptsout of the app and Jest programs into the Bun program. Nothing insrc/imports it;scripts/,evals/andtests/tooling/do.tsBuildInfoFile.modules/ExpensifyNitroUtilsnow extendstsconfig.app.jsonand sets its own, so it cannot clobber the app's.Timings
Cold is a wash; warm is ~4× faster because build mode skips a project whose build info is current instead of rebuilding its program.
Fixed Issues
$ #99291
PROPOSAL: N/A
Tests
Automated tests / CI only.
Offline tests
Not applicable: tooling-only change.
QA Steps
n/a - tooling-only change.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionScreenshots/Videos
Not applicable: tooling-only change.