fix(tsconfig): silence TS 7.0 baseUrl + rootDir deprecation warnings#472
fix(tsconfig): silence TS 7.0 baseUrl + rootDir deprecation warnings#472roiizchak wants to merge 1 commit into
Conversation
57f10a4 to
32995b5
Compare
jrusso1020
left a comment
There was a problem hiding this comment.
Verdict
Approve. The change is functionally correct — both bun run --filter '@hyperframes/{cli,studio}' typecheck exit clean — and the ignoreDeprecations: "5.0" value is the canonical silencer for baseUrl on the repo's TypeScript 5.9.3.
That said, the PR description does not match the code, and the reasoning given for one of the changes is wrong. Both are worth fixing before merge so the description doesn't mislead future readers.
Key Concerns
(none blocking — see below)
Test Coverage
No test changes needed; this is a tsconfig metadata change with no runtime/emit effect (verified separately — see "Nits" item 2). The "verified clean" line in the test plan is what matters here, and typecheck confirms it.
Nits / Future
-
Description ↔ code mismatch on
ignoreDeprecationsvalue. The PR body's diff snippets show"ignoreDeprecations": "6.0", but bothpackages/cli/tsconfig.json:6andpackages/studio/tsconfig.json:6use"5.0". The code is right —node_modules/typescript/lib/typescript.js(TS 5.9.3) explicitly rejects every value other than"5.0":// typescript.js: getIgnoreDeprecationsVersion() if (ignoreDeprecations === "5.0") { return new Version(ignoreDeprecations); } reportInvalidIgnoreDeprecations(); // any other value, including "6.0", errors out
So anyone copy-pasting
"6.0"from the description would actually break their build. Recommend updating the PR body before merge so it doesn't become misleading historical documentation. -
rootDir: ".."reasoning in the description is incorrect, but the change is still safe — for a different reason. The description claimsrootDir: "..""only affects how TS computes error paths and does not alter emitted file locations when combined withinclude: [\"src\"]." That isn't how TypeScript works — an explicitrootDiroverrides the auto-computed common source directory and would shift emit paths fromdist/foo.tstodist/cli/src/foo.tsif tsc were doing the emit.The change is still safe because the CLI doesn't use tsc to emit (
packages/cli/package.json:18—"build": "bun run build:fonts && tsup && ..."), andtypecheckistsc --noEmit. Same for studio (vite build). SorootDirhere only feeds the typechecker, and the typechecker doesn't write files. Worth correcting the description so future readers don't generalize the (wrong) reasoning to packages that do usetsc -bfor emit (e.g. core, engine). -
(Future) Once
pathsis dropped in favor of bun workspace resolution (as the description hints), the wholebaseUrl+paths+ignoreDeprecationsblock on both packages can come out together. Tracking-only — out of scope here.
vanceingalls
left a comment
There was a problem hiding this comment.
Additive review at 32995b5a — @jrusso1020 already approved with detailed nits 6 days ago. The code is good; the PR-body framing has known issues James already flagged.
Audited
packages/cli/tsconfig.json(+ignoreDeprecations: "5.0",+rootDir: "..")packages/studio/tsconfig.json(+ignoreDeprecations: "5.0")- CI: all required green ✓
Strength
James covered the correctness end-to-end — verified bun run --filter '@hyperframes/{cli,studio}' typecheck exits clean, confirmed ignoreDeprecations: "5.0" is the only TS 5.9.3-accepted value ("6.0" errors out per getIgnoreDeprecationsVersion()), and explained why rootDir: ".." doesn't shift emit paths in this repo (CLI builds via tsup, studio via vite build; typecheck is tsc --noEmit).
Important — supersedes the tsconfig portion of #795
#795 (Jefsky) bundles the same tsconfig fix with three unrelated changes and a broken LayersPanel.tsx new-file addition. This PR is the single-purpose, already-approved version. Recommend Vance lands this and closes the tsconfig portion of #795 (or just close #795 entirely; the other three fixes also have approved single-purpose versions in #622, #625, #623).
Important — author should fix the PR-body framing before merge
James flagged two issues with the PR description that should be corrected before merge so it doesn't become misleading historical documentation:
- PR body shows
"ignoreDeprecations": "6.0"in the diff snippets, but the code uses"5.0". Anyone copy-pasting from the description would break their build. rootDir: ".."reasoning is incorrect — the description claimsrootDir"only affects error paths," which isn't how TypeScript emit works. The change is still safe (because tsc isn't doing emit here), but for the right reason, not the stated one.
Worth a 2-minute update before merge — but the code is correct, and James has the substantive review nailed.
Verdict
Verdict: COMMENT
Reasoning: Code is correct (James approved with full verification). External-contributor PR — Vance check before merging. Recommend the author fix the PR-body framing issues James flagged, then this can land. Supersedes the tsconfig portion of #795.
— Vai
vanceingalls
left a comment
There was a problem hiding this comment.
roiizchak's tsconfig deprecation silencing. @jrusso1020 already APPROVED on this SHA with detailed nits accepted. CLEAN merge state. Vance authorized.
Verdict: APPROVE
Reasoning: James verified, no functional change, CLEAN.
— Vai
Summary
TypeScript 7.0 deprecates the
baseUrlcompiler option unlessignoreDeprecationsis set, and also flags tsconfigs whose computed common source directory differs from an explicitrootDir. Both conditions currently trigger warnings inpackages/cli/tsconfig.jsonandpackages/studio/tsconfig.json:packages/cli/tsconfig.json—baseUrldeprecation and "common source directory is '..'" becausepathsmaps@hyperframes/producerto../producer/src/index.ts, pulling the compiler's root above./src.packages/studio/tsconfig.json—baseUrldeprecation only (rootDir is already"..").The other six package tsconfigs (
core,engine,player,player/tests/perf,producer,shader-transitions) setrootDirexplicitly and don't usebaseUrl, so they don't trip either warning.Changes
packages/cli/tsconfig.json— addignoreDeprecations: "5.0"androotDir: ".."(matches studio's pattern; covers bothcli/srcand the resolvedpathstarget underpackages/):packages/studio/tsconfig.json— addignoreDeprecations: "5.0"only:"moduleResolution": "bundler", + "ignoreDeprecations": "5.0", "baseUrl": ".", "paths": { "@hyperframes/player": ["../player/src/hyperframes-player.ts"] },Rationale
ignoreDeprecations: "5.0"is the only value TypeScript 5.x accepts — TS 5.9.3'sgetIgnoreDeprecationsVersion()explicitly rejects every other value (including"6.0") withInvalid value for '--ignoreDeprecations'. Newer editor LS versions suggest"6.0"in their warning text, but that fails typecheck on this repo's pinned compiler. It leavesbaseUrl/pathsbehavior unchanged while silencing the warning until a future refactor is ready to droppathsin favor of bun workspace resolution.rootDir: ".."on the CLI matches the existing pattern inpackages/studio/tsconfig.json, and keeps the resolved@hyperframes/producerfile underrootDirso no TS6059 is introduced.Test plan
bun install— clean.bun run build— builds successfully with no new errors.bunx oxlint packages/cli/tsconfig.json packages/studio/tsconfig.json— clean.bunx oxfmt --check packages/cli/tsconfig.json packages/studio/tsconfig.json— clean.No runtime changes. No emit-layout changes — the CLI builds via
tsup(packages/cli/package.json"build": "bun run build:fonts && tsup && ...") and studio viavite build; thetypecheckscript istsc --noEmit. An explicitrootDirwould shift emit paths if tsc were doing the emit (it overrides the auto-computed common source directory), but here it only feeds the typechecker, which doesn't write files. Packages that emit viatsc -b(e.g.core,engine) are untouched by this PR.