Upgrade workspace and fixtures to TypeScript 6#137
Conversation
wyattjoh
left a comment
There was a problem hiding this comment.
Code Review — PR #137
Reviewed with Opus + Codex second-opinion validation.
Blocker
B1. bun run format:check fails on packages/cli-core/src/lib/help.ts:105-115 (codex confirmed)
The terms.map arrow body was converted from an expression to a block but the indent/wrapping parens around helper.formatItem(...) were not re-formatted. CI format:check will reject. Run bun run format and re-push.
Major
M1. Branch is very stale (codex partial)
Merge-base is 85f7cb3b (Nov 2025); main is at 1d746017. ~15 PRs merged since, including changeset enforcement, Homebrew distribution, NonEmptyArray helpers, PKCE rejection sampling, skill infrastructure, and removal of log.ts / runners.ts / installer.ts. Reviewing this PR without rebasing cannot predict post-rebase behavior; the TS 6 sweep will almost certainly conflict with help.ts / spinner.ts / constants.ts. Rebase onto current main, re-run bun run format:check / lint / typecheck / test, then re-open for review.
M2. Next 14 fixture opts out of noUncheckedSideEffectImports rather than fixing the side-effect import (codex confirmed)
test/e2e/fixtures/nextjs-app-router-next14/tsconfig.json:7 sets "noUncheckedSideEffectImports": false. TS 6 has this on by default, and the fixture is meant to represent what downstream users of clerk init would get. Either fix the offending side-effect import in the fixture (preferred; matches user reality), or update the init scaffolder to inject the opt-out into generated Next 14 tsconfigs.
Minor
- m1. Vite-based framework test fixtures (
react.test.ts:19,react-router.test.ts:18,tanstack-start.test.ts:18,vue.test.ts:18) setframework.envFile = ".env", but canonicalFrameworkInfoinlib/framework.ts:45-77declares.env.localfor these frameworks. Tests pass because nothing asserts on this field today. - m2. Root
typecheckscript (package.json:10) covers only@clerk/cli-core;scripts/**/*.tsand e2e fixtures are not validated under TS 6. - m3. No changeset. Touches runtime cli-core files (
auth-server.ts,constants.ts,dotenv.ts,help.ts,spinner.ts,heuristics.ts,transformations.ts,react-router.ts). Post-rebase, theEnforce Changesetworkflow will require one; add a patch-level changeset for@clerk/cli-core. - m4.
ProjectContext.envFile: stringis wider thanFrameworkInfo.envFile: ".env" | ".env.local"(frameworks/types.ts:11). Tightening to the literal union would turn m1 into a compile error.
Nits
FRAMES[0]!fallback inspinner.ts:99is defensive cruft for a non-empty tuple; typingFRAMES as constremoves the need.env-paths{suffix: ""}inconstants.ts:15is behaviorally equivalent to the old{suffix: false}(both falsy); no migration concern, noted for the reviewer's benefit.
Positives
Switching from expect(arr[0].foo).toBe(...) to destructure + toBeDefined() + optional chaining is the right pattern for noUncheckedIndexedAccess. FrameworkTemplateName = Exclude<TemplateName, "generic"> is a correct fix: the prior Exclude<TemplateName, "generic" | "generic-fallback"> was wrong because "generic-fallback" is actually referenced as a framework template in FRAMEWORK_PROMPTS.vite.template. TS 6 forced the discovery of a latent type bug. plapi.test.ts was rewritten to use getPlapiBaseUrl() rather than hardcoding api.clerk.com, making the suite portable across environments. heuristics.ts added an explicit "Invalid package manager install command" guard rather than silently propagating undefined.
rafa-thayto
left a comment
There was a problem hiding this comment.
Code Review -- PR #137
I reviewed the full diff against main. wyattjoh's earlier review is thorough and accurate; I'll confirm the blocker/majors and add one finding that wasn't called out.
Blocker
B1. help.ts formatting -- confirmed. The terms.map rewrite (lines 105-112) has misaligned closing parens. bun run format will fix it, but CI format:check will reject as-is.
B2. Stale branch -- confirmed. The merge-base is months behind main. A rebase is needed before this can land cleanly. Several touched files (help.ts, spinner.ts, constants.ts, prompts) have diverged.
New finding
init/index.test.ts -- fwOverride.envFile uses ".env" for Next.js, but FRAMEWORK_MAP defines Next.js as ".env"... that's correct. However, the existingCtx object on the same file (around line 243 in the diff) sets publishableKeyEnv to "x" in the old code -- the PR replaces this with proper envVar/envFile fields, but sets envFile: ".env" on the ProjectContext level while the framework also gets envFile: ".env". This is fine for Next.js. But FAKE_CTX (the React fixture) has framework.envFile: ".env" when the canonical FRAMEWORK_MAP entry for React is .env.local. This is pre-existing on main, but since this PR adds an explicit ProjectContext type annotation to FAKE_CTX, TS6 will happily accept it (both are valid literals in the union). Worth fixing while you're here to keep test fixtures honest -- change it to ".env.local" to match production. Confidence: 82/100.
Confirming wyattjoh's review
All the points from the prior review hold up:
- M2 (noUncheckedSideEffectImports opt-out): Agreed. The Next 14 fixture disabling it is a workaround, not a fix. Worth investigating the offending import.
- m3 (missing changeset): Confirmed. The
Enforce Changesetworkflow requires one since runtime files underpackages/cli-core/src/are touched. An empty changeset (bun changeset --empty) is the right call here since this is an internal toolchain upgrade. - Positives: The
FrameworkTemplateNamefix,getPlapiBaseUrl()in tests,setFetchMockhelper, and safe index access patterns are all solid improvements.
No additional blockers found. The PR is well-structured and the TS6 fixes are correct. Just needs the rebase, format pass, changeset, and the minor fixture fixes before it's ready to merge.
c9816dd to
9d42281
Compare
🦋 Changeset detectedLatest commit: 0f88d29 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
9d42281 to
f4e28ab
Compare
- Remove duplicate `types` key in root and cli-core tsconfig.json (rebase artifact) - Remove duplicate `typecheck` script key in root package.json (rebase artifact) - Remove unnecessary `noUncheckedSideEffectImports: false` from next14 fixture - Add patch changeset for the TypeScript 6 upgrade - Regenerate bun.lock after rebase
- Fix FAKE_CTX framework.envFile from ".env" to ".env.local" to match the canonical React FrameworkInfo definition - Fix changeset to target @clerk/cli-core (the package where TS6 changes live) instead of the wrapper clerk package
TypeScript 6 enables noUncheckedSideEffectImports by default, which causes `import "./globals.css"` to fail without a type declaration. Add a css.d.ts file to declare the module, matching what Next.js 16 provides built-in.
39091a2 to
0f88d29
Compare
Summary
typescript@6.0.2typecheckentrypoints for TS6cli-coreTS6 diagnostics and align tests and fixtures with the new compiler behaviorCommits
build: upgrade workspace to TypeScript 6.0.2fix: resolve TypeScript 6 diagnostics in cli-coretest: align cli-core tests with TypeScript 6chore: upgrade e2e fixtures to TypeScript 6Verification
bun installbun run typecheckbun run buildbun run testcd packages/cli-core/mocks && bun install && bunx tsc --noEmitnextjs-app-routernextjs-pages-routernextjs-app-router-next14react-routerreactvuetanstack-startNotes
bun run test:e2ewas not run because this environment does not have the required Clerk e2e secrets (CLERK_PLATFORM_API_KEY,CLERK_CLI_TEST_APP_ID, etc.)