From 054b411c59b0e1807cbb9e46e5660d722b32ce17 Mon Sep 17 00:00:00 2001 From: Janpot <2109932+Janpot@users.noreply.github.com> Date: Tue, 19 May 2026 14:41:29 +0200 Subject: [PATCH 1/6] [code-infra] Add ts-package-migration skill --- .claude/skills/ts-package-migration/SKILL.md | 261 +++++++++++++++++++ 1 file changed, 261 insertions(+) create mode 100644 .claude/skills/ts-package-migration/SKILL.md diff --git a/.claude/skills/ts-package-migration/SKILL.md b/.claude/skills/ts-package-migration/SKILL.md new file mode 100644 index 00000000000000..424c82c71e9faa --- /dev/null +++ b/.claude/skills/ts-package-migration/SKILL.md @@ -0,0 +1,261 @@ +--- +name: ts-package-migration +description: >- + Migrate a MUI monorepo package from hand-written `.js` + hand-written `.d.ts` + source to true TypeScript (single `.ts`/`.tsx` source, declarations emitted by + `tsc`), like `@mui/utils`. Use when asked to "convert to TypeScript", + "true TS like @mui/utils", or to remove a package's `--skipTsc` build. Hard + requirement it always enforces: the package's exported type surface stays + 100% identical, verified by a bidirectional type-equivalence probe. +--- + +When this skill is invoked, follow this playbook to convert a MUI monorepo +package from hand-written `.js` + hand-written `.d.ts` to true TypeScript, +mirroring `@mui/utils`. Run it in the main conversation — it is interactive and +judgement-heavy: confirm destructive/outward actions, and surface trade-offs and +impossibilities to the user rather than guessing. Delegate only the noisy +read-only recon (step 0) to a throwaway `general-purpose`/`Explore` subagent and +keep just the findings. + +The build pipeline is `@mui/internal-code-infra`'s `code-infra build`: Babel +emits the JS, `tsc -p tsconfig.build.json` emits the `.d.ts`. With `--skipTsc` +it instead copies hand-written `.d.ts` verbatim. + +## The contract (in priority order) + +1. **Exported types stay 100% identical** — this is the real, non-negotiable + acceptance criterion. Prove it with a tsc probe (see Verification). +2. **Emitted JS stays byte-identical** modulo whitespace/comments/mangling. +3. `.d.ts` ideally identical; where true-TS emission forces a different *form*, + it is acceptable **only if the resolved exported types are identical** — + document each exact difference with its root cause. + +A single `.ts` source feeds both Babel and `tsc`. When the original package +deliberately or sloppily diverged its hand `.js` from its hand `.d.ts` (e.g. +the `.d.ts` re-exports a third-party default while the `.js` has its own +runtime default), **byte-identical JS ∧ byte-identical `.d.ts` ∧ identical +types is mathematically unsatisfiable** from one source. Do not loop on it: +prioritize (1), then (2), document (3), and say so plainly. + +## Procedure + +### 0. Isolate & baseline FIRST +- Work in a git worktree; `pnpm install --frozen-lockfile` (fast, shared store). +- Build the package **unmodified** and snapshot every + `build/**/*.{js,mjs,d.ts,d.mts}` to a scratch dir. This is the parity oracle. +- If sources are already edited, `git stash -u`, build, snapshot, `git stash pop`. + +### 1. Convert source (per `.js`+`.d.ts` pair → one `.ts`/`.tsx`) +- `.tsx` for files with JSX, else `.ts`. `git mv` the `.js` so history shows a + rename; fold the hand-written `.d.ts`'s types into the same file; delete the + `.d.ts`. (Barrels: `export { default } from './X'; export type * from './X';`) +- Port the hand-written type block **verbatim**; only adjust what's required to + compile. Add casts that Babel strips (`as any`, `as unknown as T`) rather than + restructuring runtime logic — runtime JS must not change. +- Convert test `.js` → `.ts`/`.tsx` too (required: `allowJs:false`). Typical + fixes: `let x;`→ explicit type; theme callbacks → `(theme: any)`; + `delete obj.prop` needs `prop?:` optional. + +### 2. Build config +- `package.json`: build script drop `--skipTsc`; `exports` `.js`→`.ts` + (`"." : "./src/index.ts"`, `"./*": "./src/*/index.ts"`); add + `@types/prop-types` (devDep, exact, e.g. `15.7.15`) if any file uses propTypes. +- `tsconfig.json`: `compilerOptions: { allowJs: false, skipLibCheck: true, + types: [ … ] }`. **A package must never pull in `node` types.** Do not + inherit the repo-root `types` (it contains `node`) and never list `"node"` — + set `types` *explicitly* to either `[]` or only what the package actually + needs. For a React package with Vitest tests that use `process.env`: + `"types": ["react", "vitest/globals", "@mui/internal-code-infra/build-env"]`. + `skipLibCheck: true` is **required**: trimming `types` exposes unrelated + errors inside third-party `.d.ts` (e.g. `@vitest/spy` references the + `Disposable` global `@types/node` used to supply) — mui-x sets + `skipLibCheck:true` for exactly this. `@mui/internal-code-infra/build-env` + makes `process.env` resolve without `node`. +- Add `tsconfig.build.json`: `extends: "./tsconfig.json"`, `composite, + declaration, emitDeclarationOnly, noEmit:false, outDir:"build", + rootDir:"./src"`, include `src/**/*.ts*`, exclude `*.test.ts*`/`*.spec.ts*`. + Add `references` only if it depends on another workspace package's types. Add + `stripInternal: true` (paired with `/** @internal *​/` on the relevant + declarations) when an existing runtime-only export was intentionally hidden + from the hand-written `.d.ts` — but treat this as a judgement call (see the + `@internal` finding below; the omission may also be a bug worth surfacing + rather than formalizing). **Also wire the reverse direction:** every *downstream* package that + builds via `tsc` and imports this one must add + `{ "path": "..//tsconfig.build.json" }` to *its* + `tsconfig.build.json` `references` (see Verification step 6 — skipping this is + a guaranteed CI failure). **`tsconfig.build.json` is mandatory** once + `--skipTsc` is gone or + the build throws. +- **`types` in `tsconfig.build.json` — never `node` for a browser package.** + Declaration emit still type-checks, so build-time `process.env.NODE_ENV` + must be typed; `types:["react"]` alone fails with `TS2591 Cannot find name + 'process'`, but pulling `@types/node` into a non-Node package is wrong. + The shared infra package already ships this ambient: reference + **`@mui/internal-code-infra/build-env`** (its `./build-env` export → + `src/build-env.d.ts`, which declares a minimal global `process.env`). No + custom file to create or maintain: + `"types": ["@mui/internal-code-infra/build-env", "react"]` + (drop `"react"` only if the source imports `React` explicitly everywhere; + keep it if any `.ts` uses the global `React.*` namespace without importing, + e.g. in a folded type block — adding `import * as React` instead would add a + namespace import to the emitted JS). `@mui/internal-code-infra` is a + repo-wide build dependency, so the subpath resolves from any package with + `moduleResolution: bundler`/`node16` (no per-package dependency needed). +- The same no-`node` rule applies to **both** configs, both pointing at + `@mui/internal-code-infra/build-env` for `process.env`. `tsconfig.build.json`'s + `types` (above) covers declaration emit; the package's `tsconfig.json` + (dev/test typecheck) additionally needs `vitest/globals` for the test files + but still **no `node`** — + `["react", "vitest/globals", "@mui/internal-code-infra/build-env"]` — plus + `skipLibCheck:true`. Neither config may inherit the repo-root `types` (it + contains `node`); both set `types` explicitly. + +### 3. Build, diff, fix; iterate until parity is at the proven optimum. + +### 4. Quality gates — run all four from the repo root and fix every issue +- **Build:** `pnpm -F build` +- **Prettier:** `pnpm exec prettier --check "packages//src/**/*.{ts,tsx}" --ignore-path .lintignore` + then `pnpm exec prettier --write …` to fix. (Prettier reformats source only — + it does not change emitted JS beyond whitespace; re-confirm with the diff.) +- **ESLint:** `pnpm exec eslint packages/ --report-unused-disable-directives --max-warnings 0` + (repo runs flat-config ESLint with `--max-warnings 0`; warnings fail CI). +- **Typecheck:** `pnpm -F typescript` (covers converted tests + `.spec.tsx`). +Re-run the type-equivalence probe after any lint/format fix that touches source. + +## Hard-won findings (apply directly — these are the traps) + +- **Babel (`@babel/preset-typescript`, default opts) elides type-only *named* + imports but NOT namespace `import * as X` nor `export *`/`export type *` + statements.** Consequences: + - Namespace type imports → use `import type * as X` (keeps JS clean; cost: + `.d.ts` shows `import type * as X` vs `import * as X` — type-identical). + - Named type imports → **always use an explicit `type` modifier** (inline + `import { Value, type Foo } from 'pkg';` or a separate + `import type { Foo } from 'pkg';`). Even though Babel elides type-only + names today, relying on silent elision is fragile across tooling (different + `verbatimModuleSyntax`/`onlyRemoveTypeImports` settings, other bundlers) + and Copilot will flag a bare `import { TypeName }` as a potential + "does not provide an export named" runtime hazard in ESM — exactly the + review comment that landed on the `@mui/styled-engine` PR for + `import { Global, Interpolation } from '@emotion/react'` (`Interpolation` + is type-only). Fix: `import { Global, type Interpolation } from + '@emotion/react'`. Inline `type` and separate `import type` are both + erased identically by Babel (verified: JS output byte-identical). + - Re-export of a type-only module (`export * from '@emotion/styled'`) → use + `export type *` so no runtime re-export line is added to the JS (cost: + `.d.ts` shows `export type *` — type-identical when the target's surface is + all `export type` + a default). +- **`'use client'` forbids `export *` — including `export type *`.** The repo's + `no-restricted-syntax` ESLint rule rejects any `ExportAllDeclaration` in a + module with the `'use client'` pragma (Next.js limitation). It fires on + `export type *` too (it is syntactically an `ExportAllDeclaration`, even + though it is runtime-erased). When the entry has `'use client'`, re-export the + third-party/barrel type surface with **explicit named** type re-exports + instead: `export type { A, B, C } from 'pkg';`. Do **not** name-re-export a + type you also locally declare (that is a duplicate-export error, whereas + `export *` silently lets the local shadow) — only re-export the names the + module does *not* redeclare. Verify the net surface is unchanged with the + type-equivalence probe (it checks the exported-name set both ways). +- **Runtime-wrapper-with-third-party-type default export** (e.g. local `styled` + wrapper whose public type must be emotion's `CreateStyled`): annotate + `export default localImpl as unknown as CreateStyled`. `tsc` emits + `declare const _default: CreateStyled; export default _default;` — the + resolved default type is identical to a `export { default } from '...'` + re-export. Unavoidable JS cost: `export default function f(){}` becomes + `function f(){}; export default f;` (same function, name, binding, semantics). +- **propTypes on a component creates a tsc expando** → emits + `declare namespace X { var propTypes: any }` and splits + `export default function X` into `declare function X; export default X;`. + Suppress it **and** keep the Babel production guard with: + `(X as any).propTypes /* remove-proptypes */ = { ... };` + The `/* remove-proptypes */` trailing comment on the assignment LHS triggers + `babel-plugin-transform-react-remove-prop-types` `forceRemoval` (keeps the + `process.env.NODE_ENV !== "production" ? … : void 0` wrap), while the + `(X as any)` LHS stops `tsc` from creating the expando — so the `.d.ts` + default export stays `export default function X`. (Plain + `X.propTypes = {} as any` keeps the guard but adds the expando; `(X as + any).propTypes = {}` without the comment drops the guard — JS regression.) +- **`stripInternal: true` + `/** @internal *​/`** on a declaration removes + runtime-only exports (e.g. `TEST_INTERNALS_DO_NOT_USE`) from emitted `.d.ts`. + **An export that exists at runtime but is missing from the hand-written + `.d.ts` is a judgement call, not a mechanical fix** — it can be either + (a) intentionally private (the most-faithful conversion is to formalize that + with `@internal` + `stripInternal`, keeping it out of the emitted `.d.ts`) or + (b) a bug in the hand-written `.d.ts` (the most-faithful conversion is to + let `tsc` emit the real declaration — an additive, non-breaking surface + change that fixes the omission). Use the signals — naming (`_internal_*`, + `*_DO_NOT_USE`, `TEST_*` strongly suggest private), prior intent in surrounding + comments, whether external consumers reasonably need it — and **surface the + call to the user with your recommendation before applying**; never silently + mark a runtime export `@internal`. Note: `@internal` on an *expando + assignment* does NOT propagate — only on the declaration itself. +- `tsc` always emits `declare` on function declarations in `.d.ts`; hand-written + baselines often omit it. `export function f` vs `export declare function f` + are identical in a declaration file — document, don't chase. +- A subpath component's props type may never be exported at the package root if + the baseline `index.d.ts` had no `export *` for it — verify, don't assume. + +## Verification (all must pass before claiming done) + +1. `pnpm -F build` succeeds. +2. Normalized diff every `build/**` file vs the baseline snapshot (strip + comments, collapse whitespace). Enumerate every residual diff with root cause. +3. **Type-equivalence probe** — the proof that matters. Stage two copies of the + built package (baseline snapshot, new build) with their `package.json` + `exports`; write `probe.ts`: + ```ts + type Equal = (()=>T extends X?1:2) extends (()=>T extends Y?1:2) ? true : false; + type Expect = T; + import * as Base from './base'; import * as New from './new'; + import BaseDefault from './base'; import NewDefault from './new'; + // strict Equal for every exported type (instantiate generics with a + // representative Props); bidirectional `const a: typeof Base.x = New.x` + // for value exports; Equal, never> + // both ways for the exported-name set; repeat for each subpath export. + ``` + `tsc -p` it with `strict`, `moduleResolution:bundler`. **Exit 0 = exported + types provably identical.** A failure pinpoints the exact divergence. +4. `pnpm -F typescript` (includes converted tests + `.spec.tsx`). +5. `pnpm -F test` (node + browser projects). +6. **Build every downstream consumer, not just typecheck it.** `pnpm -F + typescript` uses `tsconfig.json` and will *not* catch the failure + that matters here: once the converted package's `exports` point at + `./src/index.ts`, a consumer's *declaration build* + (`tsc -p /tsconfig.build.json --rootDir /src`) pulls the + converted package's `.ts` **source** into its program and fails with + `TS6059: File '…' is not under 'rootDir'` (previously it consumed the + hand-written `.d.ts` as an external/ambient file). Run the real build of the + full dependency chain in topological order, e.g. `pnpm -F @mui/types build + && pnpm -F @mui/utils build && pnpm -F build && pnpm -F + build`. CI (`release:build`) does this; a bare `typescript` check does not — + this is exactly how the `@mui/styled-engine` PR's first CI run failed + (`test-dev`, `pkg.pr.new`, `test_bundle_size_monitor`, `test_regressions`). + **Fix:** add the converted package as a project reference in *every* + downstream package that builds via `tsc` and imports it — add + `{ "path": "..//tsconfig.build.json" }` to that consumer's + `tsconfig.build.json` `references` array (the converted package's own + `tsconfig.build.json` must have `composite: true`, which it does). This is + the same wiring true-TS deps like `@mui/utils` already use; grep + `grep -rl '"@mui/"' packages/*/package.json` to find every consumer + that needs it. `TS6305 "Output file … has not been built"` when verifying + locally just means a referenced project's `build/` is stale — build deps + first; it is not a defect. +7. Remove all scratch dirs/probes before finishing. + +## Reporting + +State which contract clauses hold, and for every residual JS/`.d.ts` difference +give the exact before→after and its root cause (from Findings above). If the +spec demands byte-identical JS *and* `.d.ts` *and* no hand `.d.ts` for a file +whose original diverged, state the impossibility, prove it, deliver the +type-identical optimum, and stop — do not loop. + +## Reference: `@mui/styled-engine` migration (first application) + +All of the above was derived from converting `@mui/styled-engine` (PR #48544, +branch `code-infra/styled-engine-typescript`). Outcome: 9/10 emitted JS files + +`StyledEngineProvider` declarations byte-identical; all exported types proven +strictly identical (probe exit 0); only `index`'s default-export form and +type-only `import type`/`export type *`/`declare` `.d.ts` forms differ, each a +documented, type-preserving consequence of single-source true-TS. Downstream +`@mui/system` typechecked unchanged. From 212f065180e274324b09b21d4a5a0c3f1d525bad Mon Sep 17 00:00:00 2001 From: Janpot <2109932+Janpot@users.noreply.github.com> Date: Tue, 26 May 2026 16:49:18 +0200 Subject: [PATCH 2/6] [skill] Note: no /* remove-proptypes */ on the dev-only exactProp line MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reassignment inside if (NODE_ENV !== 'production') is already dead-code-eliminated in production, so the babel forceRemoval marker is redundant — matches mui-material's convention. Lesson from PR #48565. --- .claude/skills/ts-package-migration/SKILL.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/.claude/skills/ts-package-migration/SKILL.md b/.claude/skills/ts-package-migration/SKILL.md index 424c82c71e9faa..b81ca414fde263 100644 --- a/.claude/skills/ts-package-migration/SKILL.md +++ b/.claude/skills/ts-package-migration/SKILL.md @@ -175,6 +175,15 @@ Re-run the type-equivalence probe after any lint/format fix that touches source. default export stays `export default function X`. (Plain `X.propTypes = {} as any` keeps the guard but adds the expando; `(X as any).propTypes = {}` without the comment drops the guard — JS regression.) + **The companion dev-only `exactProp` reassignment inside + `if (process.env.NODE_ENV !== 'production') { … }` must NOT carry the + `/* remove-proptypes */` marker** — the env-guard already dead-code- + eliminates it in production builds, so the marker is redundant. Use: + `(X as any).propTypes = exactProp((X as any).propTypes);` — the `(X as any)` + cast is just to satisfy TS for the read/write; no LHS comment, no expando + (no static `.propTypes = {}` assignment on `X`'s declaration). Matches + `mui-material`'s convention (e.g. `Portal.tsx`, `FocusTrap.tsx`). Surfaced + on `@mui/private-theming` PR #48565 review. - **`stripInternal: true` + `/** @internal *​/`** on a declaration removes runtime-only exports (e.g. `TEST_INTERNALS_DO_NOT_USE`) from emitted `.d.ts`. **An export that exists at runtime but is missing from the hand-written From 3881ca3d901633d4c8c71870c2c23fde99997f53 Mon Sep 17 00:00:00 2001 From: Janpot <2109932+Janpot@users.noreply.github.com> Date: Wed, 27 May 2026 16:35:12 +0200 Subject: [PATCH 3/6] [skill] Add per-item leak triage heuristic and stripInternal cascade warning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two findings surfaced on @mui/system PR #48578 worth encoding: 1. Per-item triage on packages with many undeclared runtime exports — shape-of-the-export tells you which way the judgement goes (style- function siblings → promote; cross-submodule helpers → @internal), saving per-item deliberation on packages with 20+ such leaks. 2. stripInternal:true strips every @internal in the package, including pre-existing ones. If a pre-existing @internal is reachable through the public type surface (as Grid.unstable_level was via GridOwnerState), enabling the flag breaks downstream consumers' declaration builds — not the converted package's own build, so it slips past local steps that skip Verification 6. Audit + drop the lying tag before enabling. --- .claude/skills/ts-package-migration/SKILL.md | 27 ++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/.claude/skills/ts-package-migration/SKILL.md b/.claude/skills/ts-package-migration/SKILL.md index b81ca414fde263..a0c581f51992f9 100644 --- a/.claude/skills/ts-package-migration/SKILL.md +++ b/.claude/skills/ts-package-migration/SKILL.md @@ -198,6 +198,33 @@ Re-run the type-equivalence probe after any lint/format fix that touches source. call to the user with your recommendation before applying**; never silently mark a runtime export `@internal`. Note: `@internal` on an *expando assignment* does NOT propagate — only on the declaration itself. + + **Per-item triage for packages with many undeclared runtime exports.** On + packages with a dozen-plus such leaks (e.g. `@mui/system` had ~26 across 7 + dirs), the judgement scales by shape: a runtime export that mirrors the + *form* of declared siblings — `outline`/`outlineColor` next to `border`/etc., + or `displayPrint`/`displayRaw`/`overflow`/`textOverflow`/`visibility`/ + `whiteSpace` when the dir's compose-default treats them uniformly — is + almost certainly an oversight-(b) case; let `tsc` emit it and call out the + surface additions in the PR description. A runtime export that is a utility + helper consumed cross-submodule but never style-function-shaped — + `borderTransform`, `paletteTransform`, `sizingTransform`, + `marginKeys`/`paddingKeys`, `styleFunctionMapping`, breakpoint-internal + helpers like `createEmptyBreakpointObject` — is (a); annotate `@internal`. + Surfaced on `@mui/system` PR #48578. + + **`stripInternal: true` strips every `@internal`-marked declaration in the + package — including pre-existing ones you didn't add.** If a pre-existing + `@internal` is referenced transitively in another emitted type (e.g. as a + literal key in a `PartiallyRequired` constraint, or anywhere `keyof` + the stripped surface is consumed), turning on `stripInternal` will break the + downstream consumer's declaration build — *not* the converted package's own + build, which makes the failure easy to miss locally if Verification step 6 + is skipped. Before enabling: `git grep "@internal" packages//src/`, + and for each match check whether the symbol is reachable through the + public type surface anyway. If it is — as `Grid.unstable_level` was via + `GridOwnerState` in `@mui/system` — the pre-existing `@internal` was + lying; drop the tag rather than letting the strip break consumers. - `tsc` always emits `declare` on function declarations in `.d.ts`; hand-written baselines often omit it. `export function f` vs `export declare function f` are identical in a declaration file — document, don't chase. From d2c6925d6319102cdfb44c9b934a51b4229519b0 Mon Sep 17 00:00:00 2001 From: Janpot <2109932+Janpot@users.noreply.github.com> Date: Wed, 27 May 2026 19:41:34 +0200 Subject: [PATCH 4/6] [skill] Fix propTypes pattern; add wildcard-exports finding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two corrections from @mui/system PR #48578: - The (X as any).propTypes = {} pattern this skill recommended bricks typescript-to-proptypes (Expected type "Identifier", got "TSAsExpression"). It worked for styled-engine because that package has no propTypes-bearing components in the generator's input list — but failed immediately on @mui/system's Box.tsx and ThemeProvider.tsx. Default to the mui-material convention used by Portal.tsx / FocusTrap.tsx: `X.propTypes = { ... } as any;` for the main assignment, `(X as any)['propTypes' + ''] = exactProp((X as any).propTypes);` for the dev reassignment. This admits the tsc expando but keeps the Babel guard AND passes the generator. - Wildcard package.json `exports` (`./*: ./src/*/index.ts`) only resolve `.ts` files; any dir still on `.js` (partial conversion or mid-PR revert) needs an explicit entry, otherwise rolldown bundle-size fails to resolve the package subpath. --- .claude/skills/ts-package-migration/SKILL.md | 74 +++++++++++++++----- 1 file changed, 57 insertions(+), 17 deletions(-) diff --git a/.claude/skills/ts-package-migration/SKILL.md b/.claude/skills/ts-package-migration/SKILL.md index a0c581f51992f9..5079b86172e03d 100644 --- a/.claude/skills/ts-package-migration/SKILL.md +++ b/.claude/skills/ts-package-migration/SKILL.md @@ -166,24 +166,64 @@ Re-run the type-equivalence probe after any lint/format fix that touches source. - **propTypes on a component creates a tsc expando** → emits `declare namespace X { var propTypes: any }` and splits `export default function X` into `declare function X; export default X;`. - Suppress it **and** keep the Babel production guard with: - `(X as any).propTypes /* remove-proptypes */ = { ... };` - The `/* remove-proptypes */` trailing comment on the assignment LHS triggers + Two patterns exist; pick based on whether the file is processed by the + repo's `pnpm proptypes` (`typescript-to-proptypes`) generator — which is + **everything in `@mui/system`, `@mui/material`, `@mui/lab`, `@mui/joy` with + a React component**. + + **Pattern A — `mui-material` convention** (used by `Portal.tsx`, + `FocusTrap.tsx`; mandatory for any file in `typescript-to-proptypes`'s + input list, since the script asserts the propTypes-assignment LHS object + is an `Identifier` and rejects `TSAsExpression`): + ```ts + X.propTypes /* remove-proptypes */ = { + /* ... */ + } as any; + if (process.env.NODE_ENV !== 'production') { + // eslint-disable-next-line + (X as any)['propTypes' + ''] = exactProp((X as any).propTypes); + } + ``` + The `/* remove-proptypes */` trailing comment on the static LHS triggers `babel-plugin-transform-react-remove-prop-types` `forceRemoval` (keeps the - `process.env.NODE_ENV !== "production" ? … : void 0` wrap), while the - `(X as any)` LHS stops `tsc` from creating the expando — so the `.d.ts` - default export stays `export default function X`. (Plain - `X.propTypes = {} as any` keeps the guard but adds the expando; `(X as - any).propTypes = {}` without the comment drops the guard — JS regression.) - **The companion dev-only `exactProp` reassignment inside - `if (process.env.NODE_ENV !== 'production') { … }` must NOT carry the - `/* remove-proptypes */` marker** — the env-guard already dead-code- - eliminates it in production builds, so the marker is redundant. Use: - `(X as any).propTypes = exactProp((X as any).propTypes);` — the `(X as any)` - cast is just to satisfy TS for the read/write; no LHS comment, no expando - (no static `.propTypes = {}` assignment on `X`'s declaration). Matches - `mui-material`'s convention (e.g. `Portal.tsx`, `FocusTrap.tsx`). Surfaced - on `@mui/private-theming` PR #48565 review. + `process.env.NODE_ENV !== "production" ? … : void 0` wrap). The `as any` + on the RHS satisfies tsc — at the cost of allowing the expando + `declare namespace X { var propTypes: any }` in the emitted `.d.ts`, which + is benign and matches what `mui-material` ships. The dev-only `exactProp` + reassignment uses a computed-key `['propTypes' + '']` LHS so + `typescript-to-proptypes` doesn't try to inject into it; the marker + comment is omitted because the `NODE_ENV` env-guard already dead-code- + eliminates it in production. + + **Pattern B — `styled-engine` convention** (no expando in the emitted + `.d.ts`, but **breaks `typescript-to-proptypes`** with `Expected type + "Identifier", got "TSAsExpression"`): + `(X as any).propTypes /* remove-proptypes */ = { ... };` with + `(X as any).propTypes = exactProp((X as any).propTypes);` for the dev + reassignment. Use **only** when the file is not in `pnpm proptypes`'s + input list — e.g. a styling-only package with no React-component + propTypes (where Pattern A would emit a needless expando but the script + never runs anyway). The original skill commit (and `@mui/private-theming` + PR #48565 review) recommended Pattern B globally; that bricked + `test_static` (Generate PropTypes) on `@mui/system` PR #48578 for + `Box.tsx` and `ThemeProvider.tsx`. Default to Pattern A. + + (Plain `X.propTypes = {}` without `as any` — no cast at all — keeps the + guard but emits a propTypes type-mismatch error at the tsc step. `(X as + any).propTypes = {}` without the `/* remove-proptypes */` comment drops + the Babel guard — JS regression.) +- **Wildcard `package.json` `exports` resolve only the wildcard's literal + extension.** A catch-all like `"./*": "./src/*/index.ts"` only resolves + directories whose `index` file is `.ts`. If the conversion leaves any + directory on `index.js` — e.g. a partial conversion or a revert of one + dir mid-PR, as with `RtlProvider/` on `@mui/system` PR #48578 — that dir + needs an explicit entry: + `"./RtlProvider": "./src/RtlProvider/index.js"`. Otherwise rolldown (and + any strict ESM resolver) fails at bundle time with + `"./X" is not exported under the conditions [...]`. The package's own + build won't catch this — only downstream bundling does (CI surfaces it + as `test_bundle_size_monitor` failure). The set of explicit entries to + keep around therefore mirrors the set of dirs still on `.js`. - **`stripInternal: true` + `/** @internal *​/`** on a declaration removes runtime-only exports (e.g. `TEST_INTERNALS_DO_NOT_USE`) from emitted `.d.ts`. **An export that exists at runtime but is missing from the hand-written From 07cf748b2477b6ff10545e0a2630b9199212119a Mon Sep 17 00:00:00 2001 From: Janpot <2109932+Janpot@users.noreply.github.com> Date: Thu, 28 May 2026 18:02:53 +0200 Subject: [PATCH 5/6] [skill] Replace propTypes Pattern A/B with binding-shape fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both cast-based patterns documented before this commit had failure modes: - `X.propTypes = {…} as any` keeps the proptypes script happy but does not prevent tsc from synthesizing `declare namespace X { var propTypes: any }` in the emitted .d.ts (the `as any` is on the value, not on the property — tsc still augments). The skill previously claimed this expando was "benign and matches mui-material"; on @mui/system part-1 it was the trigger for the cleanup commit that broke `pnpm proptypes`. - `(X as any).propTypes = {…}` does prevent the namespace but trips typescript-to-proptypes with `Expected type "Identifier", got "TSAsExpression"` because the script asserts the LHS object is an Identifier. PR #48578 hit this after the first fix. The fix is the binding shape, not a cast. Either the receiver of `.propTypes` already has propTypes in its declared type (forwardRef/memo wrapping, via @types/react — Portal.tsx, mui-x/GridRow.tsx), or it's a const-bound function expression typed by an interface that lists `propTypes?: any` (the only correct shape for non-wrapped components like ThemeProvider/GlobalStyles/ DefaultPropsProvider whose original .js was a `function X(){}` declaration). With either binding shape, the assignment site needs no casts at all — and the namespace synthesis disappears because tsc no longer has an unaccounted-for property assignment to capture. Three concrete shapes documented: - wrapped (Shape 1) — `const X = forwardRef(function X(...){...})`, `X.propTypes = {…}` — clean .d.ts emits as `declare const X: ForwardRefExoticComponent<…>`. - generic / bare-function (Shape 2) — `const X: XType = function X(...){...}` with `interface XType { …; propTypes?: any }` — clean .d.ts emits as `declare const X: XType`. - generic + wrapped (Shape 3, mui-x DataGrid) — non-exported inner `const XRaw = function X(){...}`, exported wrapper cast to a propTypes-bearing interface — clean .d.ts emits the interface and the const. Critically: a same-name `interface X { propTypes?: any }` declaration-merged with `function X(){}` is NOT sufficient. Verified empirically — tsc still synthesizes the namespace even with the interface present. The const-binding (Shape 2) is load-bearing; the interface alone is not. Reference: mui-x/packages/x-data-grid/build/DataGrid/DataGrid.d.ts and .../GridRow.d.ts ship both Shape 1 and Shape 3 today, both with clean declarations. --- .claude/skills/ts-package-migration/SKILL.md | 242 ++++++++++++------- 1 file changed, 160 insertions(+), 82 deletions(-) diff --git a/.claude/skills/ts-package-migration/SKILL.md b/.claude/skills/ts-package-migration/SKILL.md index 5079b86172e03d..95b1fb9876c4a7 100644 --- a/.claude/skills/ts-package-migration/SKILL.md +++ b/.claude/skills/ts-package-migration/SKILL.md @@ -26,7 +26,7 @@ it instead copies hand-written `.d.ts` verbatim. 1. **Exported types stay 100% identical** — this is the real, non-negotiable acceptance criterion. Prove it with a tsc probe (see Verification). 2. **Emitted JS stays byte-identical** modulo whitespace/comments/mangling. -3. `.d.ts` ideally identical; where true-TS emission forces a different *form*, +3. `.d.ts` ideally identical; where true-TS emission forces a different _form_, it is acceptable **only if the resolved exported types are identical** — document each exact difference with its root cause. @@ -40,12 +40,14 @@ prioritize (1), then (2), document (3), and say so plainly. ## Procedure ### 0. Isolate & baseline FIRST + - Work in a git worktree; `pnpm install --frozen-lockfile` (fast, shared store). - Build the package **unmodified** and snapshot every `build/**/*.{js,mjs,d.ts,d.mts}` to a scratch dir. This is the parity oracle. - If sources are already edited, `git stash -u`, build, snapshot, `git stash pop`. ### 1. Convert source (per `.js`+`.d.ts` pair → one `.ts`/`.tsx`) + - `.tsx` for files with JSX, else `.ts`. `git mv` the `.js` so history shows a rename; fold the hand-written `.d.ts`'s types into the same file; delete the `.d.ts`. (Barrels: `export { default } from './X'; export type * from './X';`) @@ -57,13 +59,14 @@ prioritize (1), then (2), document (3), and say so plainly. `delete obj.prop` needs `prop?:` optional. ### 2. Build config + - `package.json`: build script drop `--skipTsc`; `exports` `.js`→`.ts` (`"." : "./src/index.ts"`, `"./*": "./src/*/index.ts"`); add `@types/prop-types` (devDep, exact, e.g. `15.7.15`) if any file uses propTypes. - `tsconfig.json`: `compilerOptions: { allowJs: false, skipLibCheck: true, - types: [ … ] }`. **A package must never pull in `node` types.** Do not +types: [ … ] }`. **A package must never pull in `node` types.** Do not inherit the repo-root `types` (it contains `node`) and never list `"node"` — - set `types` *explicitly* to either `[]` or only what the package actually + set `types` _explicitly_ to either `[]` or only what the package actually needs. For a React package with Vitest tests that use `process.env`: `"types": ["react", "vitest/globals", "@mui/internal-code-infra/build-env"]`. `skipLibCheck: true` is **required**: trimming `types` exposes unrelated @@ -72,16 +75,16 @@ prioritize (1), then (2), document (3), and say so plainly. `skipLibCheck:true` for exactly this. `@mui/internal-code-infra/build-env` makes `process.env` resolve without `node`. - Add `tsconfig.build.json`: `extends: "./tsconfig.json"`, `composite, - declaration, emitDeclarationOnly, noEmit:false, outDir:"build", - rootDir:"./src"`, include `src/**/*.ts*`, exclude `*.test.ts*`/`*.spec.ts*`. +declaration, emitDeclarationOnly, noEmit:false, outDir:"build", +rootDir:"./src"`, include `src/**/*.ts*`, exclude `*.test.ts*`/`*.spec.ts*`. Add `references` only if it depends on another workspace package's types. Add `stripInternal: true` (paired with `/** @internal *​/` on the relevant declarations) when an existing runtime-only export was intentionally hidden from the hand-written `.d.ts` — but treat this as a judgement call (see the `@internal` finding below; the omission may also be a bug worth surfacing - rather than formalizing). **Also wire the reverse direction:** every *downstream* package that + rather than formalizing). **Also wire the reverse direction:** every _downstream_ package that builds via `tsc` and imports this one must add - `{ "path": "..//tsconfig.build.json" }` to *its* + `{ "path": "..//tsconfig.build.json" }` to _its_ `tsconfig.build.json` `references` (see Verification step 6 — skipping this is a guaranteed CI failure). **`tsconfig.build.json` is mandatory** once `--skipTsc` is gone or @@ -89,7 +92,7 @@ prioritize (1), then (2), document (3), and say so plainly. - **`types` in `tsconfig.build.json` — never `node` for a browser package.** Declaration emit still type-checks, so build-time `process.env.NODE_ENV` must be typed; `types:["react"]` alone fails with `TS2591 Cannot find name - 'process'`, but pulling `@types/node` into a non-Node package is wrong. +'process'`, but pulling `@types/node` into a non-Node package is wrong. The shared infra package already ships this ambient: reference **`@mui/internal-code-infra/build-env`** (its `./build-env` export → `src/build-env.d.ts`, which declares a minimal global `process.env`). No @@ -113,6 +116,7 @@ prioritize (1), then (2), document (3), and say so plainly. ### 3. Build, diff, fix; iterate until parity is at the proven optimum. ### 4. Quality gates — run all four from the repo root and fix every issue + - **Build:** `pnpm -F build` - **Prettier:** `pnpm exec prettier --check "packages//src/**/*.{ts,tsx}" --ignore-path .lintignore` then `pnpm exec prettier --write …` to fix. (Prettier reformats source only — @@ -120,11 +124,11 @@ prioritize (1), then (2), document (3), and say so plainly. - **ESLint:** `pnpm exec eslint packages/ --report-unused-disable-directives --max-warnings 0` (repo runs flat-config ESLint with `--max-warnings 0`; warnings fail CI). - **Typecheck:** `pnpm -F typescript` (covers converted tests + `.spec.tsx`). -Re-run the type-equivalence probe after any lint/format fix that touches source. + Re-run the type-equivalence probe after any lint/format fix that touches source. ## Hard-won findings (apply directly — these are the traps) -- **Babel (`@babel/preset-typescript`, default opts) elides type-only *named* +- **Babel (`@babel/preset-typescript`, default opts) elides type-only _named_ imports but NOT namespace `import * as X` nor `export *`/`export type *` statements.** Consequences: - Namespace type imports → use `import type * as X` (keeps JS clean; cost: @@ -139,7 +143,7 @@ Re-run the type-equivalence probe after any lint/format fix that touches source. review comment that landed on the `@mui/styled-engine` PR for `import { Global, Interpolation } from '@emotion/react'` (`Interpolation` is type-only). Fix: `import { Global, type Interpolation } from - '@emotion/react'`. Inline `type` and separate `import type` are both +'@emotion/react'`. Inline `type` and separate `import type` are both erased identically by Babel (verified: JS output byte-identical). - Re-export of a type-only module (`export * from '@emotion/styled'`) → use `export type *` so no runtime re-export line is added to the JS (cost: @@ -154,7 +158,7 @@ Re-run the type-equivalence probe after any lint/format fix that touches source. instead: `export type { A, B, C } from 'pkg';`. Do **not** name-re-export a type you also locally declare (that is a duplicate-export error, whereas `export *` silently lets the local shadow) — only re-export the names the - module does *not* redeclare. Verify the net surface is unchanged with the + module does _not_ redeclare. Verify the net surface is unchanged with the type-equivalence probe (it checks the exported-name set both ways). - **Runtime-wrapper-with-third-party-type default export** (e.g. local `styled` wrapper whose public type must be emotion's `CreateStyled`): annotate @@ -163,55 +167,125 @@ Re-run the type-equivalence probe after any lint/format fix that touches source. resolved default type is identical to a `export { default } from '...'` re-export. Unavoidable JS cost: `export default function f(){}` becomes `function f(){}; export default f;` (same function, name, binding, semantics). -- **propTypes on a component creates a tsc expando** → emits - `declare namespace X { var propTypes: any }` and splits - `export default function X` into `declare function X; export default X;`. - Two patterns exist; pick based on whether the file is processed by the - repo's `pnpm proptypes` (`typescript-to-proptypes`) generator — which is - **everything in `@mui/system`, `@mui/material`, `@mui/lab`, `@mui/joy` with - a React component**. - - **Pattern A — `mui-material` convention** (used by `Portal.tsx`, - `FocusTrap.tsx`; mandatory for any file in `typescript-to-proptypes`'s - input list, since the script asserts the propTypes-assignment LHS object - is an `Identifier` and rejects `TSAsExpression`): +- **propTypes on a component → reach for the binding shape, not a cast.** + When tsc sees `X.propTypes = …` on a `function X(){}` declaration and `X`'s + declared type doesn't list `propTypes`, it synthesizes + `declare namespace X { var propTypes: any }` in the emitted `.d.ts` and + splits `export default function X` into `declare function X; export default +X;`. The two casts that suppress this both have failure modes: + `X.propTypes = {…} as any` keeps the script happy but **does not** prevent + the namespace (the `as any` is on the value, not the property — tsc still + synthesizes); `(X as any).propTypes = {…}` does prevent the namespace but + trips `typescript-to-proptypes` with + `Expected type "Identifier", got "TSAsExpression"` because the script + `assertIdentifier`s the LHS object. **PR #48578 hit both modes in sequence** + (commit `a8f4580fd0` then `7083171`) — the cast-based options are a false + dichotomy. + + **The fix is the binding shape: make the value receiving `.propTypes` either + (a) already-typed-as-having-propTypes (via `React.ForwardRefExoticComponent` + / `React.NamedExoticComponent`, supplied by `@types/react`), or + (b) a const-bound function expression typed by an interface that lists + `propTypes?: any`.** `function X(){}` declarations are the trap; `const X = +…` bindings are not. With (a) or (b) you can drop every `as any` from the + assignment site — no LHS cast, no RHS cast, no computed-key + `['propTypes' + '']` hack — and both the script and the declaration emit + are clean. + + **Shape 1 — wrapped (`forwardRef` / `React.memo`).** Used by + `mui-material/src/Portal/Portal.tsx`, `mui-x/packages/x-data-grid/src/components/GridRow.tsx`. + The wrapper's return type already declares `propTypes?` (see `@types/react`), + so plain assignment type-checks and tsc has no augmentation to capture: + ```ts - X.propTypes /* remove-proptypes */ = { - /* ... */ - } as any; + const Portal = React.forwardRef(function Portal(props, ref) { … }); + Portal.propTypes /* remove-proptypes */ = { … }; + ``` + + Emitted: `declare const Portal: React.ForwardRefExoticComponent<…>`. No + namespace. + + **Shape 2 — bare or generic function declaration in source → convert to + typed const-bound function expression.** The bare-function shape (what + hand-written `.js` for non-wrapped components like `ThemeProvider`, + `GlobalStyles`, `DefaultPropsProvider` typically looks like) is the only + one that forces an interface declaration. Declare the value-with-propTypes + type explicitly; annotate the const; assign without casts: + + ```ts + interface ThemeProviderType { + (props: ThemeProviderProps): React.ReactElement>; + propTypes?: any; + } + + const ThemeProvider: ThemeProviderType = function ThemeProvider( + props: ThemeProviderProps, + ): React.ReactElement> { … }; + + ThemeProvider.propTypes /* remove-proptypes */ = { … }; if (process.env.NODE_ENV !== 'production') { - // eslint-disable-next-line - (X as any)['propTypes' + ''] = exactProp((X as any).propTypes); + ThemeProvider.propTypes = exactProp(ThemeProvider.propTypes); + } + export default ThemeProvider; + ``` + + Emitted: `declare const ThemeProvider: ThemeProviderType`. No namespace. + JS impact: `function X(){}` → `const X = function X(){}` — same identity, + name, length, semantics; precedent already documented above for the + `export default function f(){}` → `function f(){}; export default f;` + case. Mirror the existing `.js` form: same `function X(…)` name in the + expression, same parameter destructuring, same body. + + **Note: a same-name `interface X { propTypes?: any }` declaration-merged + with `function X(){}` is NOT sufficient.** tsc still synthesizes the + namespace even with the interface present — verified empirically. The + const-binding is load-bearing; the interface alone is not. + + **Shape 3 — generic + wrapped (mui-x `DataGrid`).** `forwardRef`/`memo` + don't preserve generic call signatures, so the exported wrapper needs an + explicit interface, and `propTypes` lands on a non-exported inner const: + + ```ts + const DataGridRaw = function DataGrid(inProps, ref) { … }; + interface DataGridComponent { + (props): React.JSX.Element; + propTypes?: any; } + export const DataGrid = React.memo(forwardRef(DataGridRaw)) as DataGridComponent; + DataGridRaw.propTypes = { … }; ``` - The `/* remove-proptypes */` trailing comment on the static LHS triggers - `babel-plugin-transform-react-remove-prop-types` `forceRemoval` (keeps the - `process.env.NODE_ENV !== "production" ? … : void 0` wrap). The `as any` - on the RHS satisfies tsc — at the cost of allowing the expando - `declare namespace X { var propTypes: any }` in the emitted `.d.ts`, which - is benign and matches what `mui-material` ships. The dev-only `exactProp` - reassignment uses a computed-key `['propTypes' + '']` LHS so - `typescript-to-proptypes` doesn't try to inject into it; the marker - comment is omitted because the `NODE_ENV` env-guard already dead-code- - eliminates it in production. - - **Pattern B — `styled-engine` convention** (no expando in the emitted - `.d.ts`, but **breaks `typescript-to-proptypes`** with `Expected type - "Identifier", got "TSAsExpression"`): - `(X as any).propTypes /* remove-proptypes */ = { ... };` with - `(X as any).propTypes = exactProp((X as any).propTypes);` for the dev - reassignment. Use **only** when the file is not in `pnpm proptypes`'s - input list — e.g. a styling-only package with no React-component - propTypes (where Pattern A would emit a needless expando but the script - never runs anyway). The original skill commit (and `@mui/private-theming` - PR #48565 review) recommended Pattern B globally; that bricked - `test_static` (Generate PropTypes) on `@mui/system` PR #48578 for - `Box.tsx` and `ThemeProvider.tsx`. Default to Pattern A. - - (Plain `X.propTypes = {}` without `as any` — no cast at all — keeps the - guard but emits a propTypes type-mismatch error at the tsc step. `(X as - any).propTypes = {}` without the `/* remove-proptypes */` comment drops - the Babel guard — JS regression.) + + Emitted: `interface DataGridComponent { …; propTypes?: any; }` and + `export declare const DataGrid: DataGridComponent`. No namespace — + `DataGridRaw` is not exported, so any synthesis on it never reaches the + `.d.ts`. Cite `mui-x/packages/x-data-grid/build/DataGrid/DataGrid.d.ts` as + the live reference. + + **Trailing `/* remove-proptypes */` comment is always required** — + `babel-plugin-transform-react-remove-prop-types` `forceRemoval` keys on it + to keep the `process.env.NODE_ENV !== 'production' ? … : void 0` runtime + wrap. The dev-only `exactProp` reassignment doesn't need the comment + (the `NODE_ENV` env-guard already dead-code-eliminates it in production) + and doesn't need any hack — once the binding is shape 1/2/3, plain + `X.propTypes = exactProp(X.propTypes)` type-checks and the script ignores + the second assignment (it picks up the first one it sees per component). + + **Verification:** after conversion, + `grep -l "declare namespace" packages//build/**/*.d.ts` must return + empty for converted files, and `pnpm proptypes --pattern "/src/()"` + must exit 0 with no working-tree changes. + + **Legacy patterns still present in the tree** (mui-material, pre-PR-48578 + mui-system) used `X.propTypes = {…} as any` (script-compatible, but ships + a `declare namespace` expando in the `.d.ts`) or `(X as any).propTypes = +{…}` (clean `.d.ts`, but script-incompatible). The latter pattern + appeared in earlier revisions of this skill as the recommended default — + it bricks `test_static` and should be replaced with the shape-based fix + above whenever the file is touched. (Plain `X.propTypes = {}` without + any cast on a `function X(){}` declaration emits a propTypes + type-mismatch error at tsc; `(X as any).propTypes = {}` without the + `/* remove-proptypes */` comment drops the Babel guard — JS regression.) + - **Wildcard `package.json` `exports` resolve only the wildcard's literal extension.** A catch-all like `"./*": "./src/*/index.ts"` only resolves directories whose `index` file is `.ts`. If the conversion leaves any @@ -224,25 +298,25 @@ Re-run the type-equivalence probe after any lint/format fix that touches source. build won't catch this — only downstream bundling does (CI surfaces it as `test_bundle_size_monitor` failure). The set of explicit entries to keep around therefore mirrors the set of dirs still on `.js`. -- **`stripInternal: true` + `/** @internal *​/`** on a declaration removes - runtime-only exports (e.g. `TEST_INTERNALS_DO_NOT_USE`) from emitted `.d.ts`. - **An export that exists at runtime but is missing from the hand-written - `.d.ts` is a judgement call, not a mechanical fix** — it can be either - (a) intentionally private (the most-faithful conversion is to formalize that - with `@internal` + `stripInternal`, keeping it out of the emitted `.d.ts`) or - (b) a bug in the hand-written `.d.ts` (the most-faithful conversion is to - let `tsc` emit the real declaration — an additive, non-breaking surface - change that fixes the omission). Use the signals — naming (`_internal_*`, - `*_DO_NOT_USE`, `TEST_*` strongly suggest private), prior intent in surrounding - comments, whether external consumers reasonably need it — and **surface the - call to the user with your recommendation before applying**; never silently - mark a runtime export `@internal`. Note: `@internal` on an *expando - assignment* does NOT propagate — only on the declaration itself. +- **`stripInternal: true` + `/** @internal _​/`** on a declaration removes +runtime-only exports (e.g. `TEST*INTERNALS_DO_NOT_USE`) from emitted `.d.ts`. +**An export that exists at runtime but is missing from the hand-written +`.d.ts`is a judgement call, not a mechanical fix** — it can be either +(a) intentionally private (the most-faithful conversion is to formalize that +with`@internal`+`stripInternal`, keeping it out of the emitted `.d.ts`) or +(b) a bug in the hand-written `.d.ts`(the most-faithful conversion is to +let`tsc` emit the real declaration — an additive, non-breaking surface +change that fixes the omission). Use the signals — naming (`\_internal*_`, +`_*DO_NOT_USE`, `TEST*_`strongly suggest private), prior intent in surrounding +comments, whether external consumers reasonably need it — and **surface the +call to the user with your recommendation before applying**; never silently +mark a runtime export`@internal`. Note: `@internal` on an _expando + assignment_ does NOT propagate — only on the declaration itself. **Per-item triage for packages with many undeclared runtime exports.** On packages with a dozen-plus such leaks (e.g. `@mui/system` had ~26 across 7 dirs), the judgement scales by shape: a runtime export that mirrors the - *form* of declared siblings — `outline`/`outlineColor` next to `border`/etc., + _form_ of declared siblings — `outline`/`outlineColor` next to `border`/etc., or `displayPrint`/`displayRaw`/`overflow`/`textOverflow`/`visibility`/ `whiteSpace` when the dir's compose-default treats them uniformly — is almost certainly an oversight-(b) case; let `tsc` emit it and call out the @@ -258,13 +332,14 @@ Re-run the type-equivalence probe after any lint/format fix that touches source. `@internal` is referenced transitively in another emitted type (e.g. as a literal key in a `PartiallyRequired` constraint, or anywhere `keyof` the stripped surface is consumed), turning on `stripInternal` will break the - downstream consumer's declaration build — *not* the converted package's own + downstream consumer's declaration build — _not_ the converted package's own build, which makes the failure easy to miss locally if Verification step 6 is skipped. Before enabling: `git grep "@internal" packages//src/`, and for each match check whether the symbol is reachable through the public type surface anyway. If it is — as `Grid.unstable_level` was via `GridOwnerState` in `@mui/system` — the pre-existing `@internal` was lying; drop the tag rather than letting the strip break consumers. + - `tsc` always emits `declare` on function declarations in `.d.ts`; hand-written baselines often omit it. `export function f` vs `export declare function f` are identical in a declaration file — document, don't chase. @@ -280,10 +355,13 @@ Re-run the type-equivalence probe after any lint/format fix that touches source. built package (baseline snapshot, new build) with their `package.json` `exports`; write `probe.ts`: ```ts - type Equal = (()=>T extends X?1:2) extends (()=>T extends Y?1:2) ? true : false; + type Equal = + (() => T extends X ? 1 : 2) extends () => T extends Y ? 1 : 2 ? true : false; type Expect = T; - import * as Base from './base'; import * as New from './new'; - import BaseDefault from './base'; import NewDefault from './new'; + import * as Base from './base'; + import * as New from './new'; + import BaseDefault from './base'; + import NewDefault from './new'; // strict Equal for every exported type (instantiate generics with a // representative Props); bidirectional `const a: typeof Base.x = New.x` // for value exports; Equal, never> @@ -294,19 +372,19 @@ Re-run the type-equivalence probe after any lint/format fix that touches source. 4. `pnpm -F typescript` (includes converted tests + `.spec.tsx`). 5. `pnpm -F test` (node + browser projects). 6. **Build every downstream consumer, not just typecheck it.** `pnpm -F - typescript` uses `tsconfig.json` and will *not* catch the failure + typescript` uses `tsconfig.json` and will _not_ catch the failure that matters here: once the converted package's `exports` point at - `./src/index.ts`, a consumer's *declaration build* + `./src/index.ts`, a consumer's _declaration build_ (`tsc -p /tsconfig.build.json --rootDir /src`) pulls the converted package's `.ts` **source** into its program and fails with `TS6059: File '…' is not under 'rootDir'` (previously it consumed the hand-written `.d.ts` as an external/ambient file). Run the real build of the full dependency chain in topological order, e.g. `pnpm -F @mui/types build - && pnpm -F @mui/utils build && pnpm -F build && pnpm -F - build`. CI (`release:build`) does this; a bare `typescript` check does not — +&& pnpm -F @mui/utils build && pnpm -F build && pnpm -F +build`. CI (`release:build`) does this; a bare `typescript` check does not — this is exactly how the `@mui/styled-engine` PR's first CI run failed (`test-dev`, `pkg.pr.new`, `test_bundle_size_monitor`, `test_regressions`). - **Fix:** add the converted package as a project reference in *every* + **Fix:** add the converted package as a project reference in _every_ downstream package that builds via `tsc` and imports it — add `{ "path": "..//tsconfig.build.json" }` to that consumer's `tsconfig.build.json` `references` array (the converted package's own @@ -322,7 +400,7 @@ Re-run the type-equivalence probe after any lint/format fix that touches source. State which contract clauses hold, and for every residual JS/`.d.ts` difference give the exact before→after and its root cause (from Findings above). If the -spec demands byte-identical JS *and* `.d.ts` *and* no hand `.d.ts` for a file +spec demands byte-identical JS _and_ `.d.ts` _and_ no hand `.d.ts` for a file whose original diverged, state the impossibility, prove it, deliver the type-identical optimum, and stop — do not loop. From e95da77d2c9643507131aca5c6ac45b7db992294 Mon Sep 17 00:00:00 2001 From: Janpot <2109932+Janpot@users.noreply.github.com> Date: Fri, 29 May 2026 10:32:13 +0200 Subject: [PATCH 6/6] Revert "[skill] Replace propTypes Pattern A/B with binding-shape fix" This reverts commit 07cf748b2477b6ff10545e0a2630b9199212119a. --- .claude/skills/ts-package-migration/SKILL.md | 242 +++++++------------ 1 file changed, 82 insertions(+), 160 deletions(-) diff --git a/.claude/skills/ts-package-migration/SKILL.md b/.claude/skills/ts-package-migration/SKILL.md index 95b1fb9876c4a7..5079b86172e03d 100644 --- a/.claude/skills/ts-package-migration/SKILL.md +++ b/.claude/skills/ts-package-migration/SKILL.md @@ -26,7 +26,7 @@ it instead copies hand-written `.d.ts` verbatim. 1. **Exported types stay 100% identical** — this is the real, non-negotiable acceptance criterion. Prove it with a tsc probe (see Verification). 2. **Emitted JS stays byte-identical** modulo whitespace/comments/mangling. -3. `.d.ts` ideally identical; where true-TS emission forces a different _form_, +3. `.d.ts` ideally identical; where true-TS emission forces a different *form*, it is acceptable **only if the resolved exported types are identical** — document each exact difference with its root cause. @@ -40,14 +40,12 @@ prioritize (1), then (2), document (3), and say so plainly. ## Procedure ### 0. Isolate & baseline FIRST - - Work in a git worktree; `pnpm install --frozen-lockfile` (fast, shared store). - Build the package **unmodified** and snapshot every `build/**/*.{js,mjs,d.ts,d.mts}` to a scratch dir. This is the parity oracle. - If sources are already edited, `git stash -u`, build, snapshot, `git stash pop`. ### 1. Convert source (per `.js`+`.d.ts` pair → one `.ts`/`.tsx`) - - `.tsx` for files with JSX, else `.ts`. `git mv` the `.js` so history shows a rename; fold the hand-written `.d.ts`'s types into the same file; delete the `.d.ts`. (Barrels: `export { default } from './X'; export type * from './X';`) @@ -59,14 +57,13 @@ prioritize (1), then (2), document (3), and say so plainly. `delete obj.prop` needs `prop?:` optional. ### 2. Build config - - `package.json`: build script drop `--skipTsc`; `exports` `.js`→`.ts` (`"." : "./src/index.ts"`, `"./*": "./src/*/index.ts"`); add `@types/prop-types` (devDep, exact, e.g. `15.7.15`) if any file uses propTypes. - `tsconfig.json`: `compilerOptions: { allowJs: false, skipLibCheck: true, -types: [ … ] }`. **A package must never pull in `node` types.** Do not + types: [ … ] }`. **A package must never pull in `node` types.** Do not inherit the repo-root `types` (it contains `node`) and never list `"node"` — - set `types` _explicitly_ to either `[]` or only what the package actually + set `types` *explicitly* to either `[]` or only what the package actually needs. For a React package with Vitest tests that use `process.env`: `"types": ["react", "vitest/globals", "@mui/internal-code-infra/build-env"]`. `skipLibCheck: true` is **required**: trimming `types` exposes unrelated @@ -75,16 +72,16 @@ types: [ … ] }`. **A package must never pull in `node` types.** Do not `skipLibCheck:true` for exactly this. `@mui/internal-code-infra/build-env` makes `process.env` resolve without `node`. - Add `tsconfig.build.json`: `extends: "./tsconfig.json"`, `composite, -declaration, emitDeclarationOnly, noEmit:false, outDir:"build", -rootDir:"./src"`, include `src/**/*.ts*`, exclude `*.test.ts*`/`*.spec.ts*`. + declaration, emitDeclarationOnly, noEmit:false, outDir:"build", + rootDir:"./src"`, include `src/**/*.ts*`, exclude `*.test.ts*`/`*.spec.ts*`. Add `references` only if it depends on another workspace package's types. Add `stripInternal: true` (paired with `/** @internal *​/` on the relevant declarations) when an existing runtime-only export was intentionally hidden from the hand-written `.d.ts` — but treat this as a judgement call (see the `@internal` finding below; the omission may also be a bug worth surfacing - rather than formalizing). **Also wire the reverse direction:** every _downstream_ package that + rather than formalizing). **Also wire the reverse direction:** every *downstream* package that builds via `tsc` and imports this one must add - `{ "path": "..//tsconfig.build.json" }` to _its_ + `{ "path": "..//tsconfig.build.json" }` to *its* `tsconfig.build.json` `references` (see Verification step 6 — skipping this is a guaranteed CI failure). **`tsconfig.build.json` is mandatory** once `--skipTsc` is gone or @@ -92,7 +89,7 @@ rootDir:"./src"`, include `src/**/*.ts*`, exclude `*.test.ts*`/`*.spec.ts*`. - **`types` in `tsconfig.build.json` — never `node` for a browser package.** Declaration emit still type-checks, so build-time `process.env.NODE_ENV` must be typed; `types:["react"]` alone fails with `TS2591 Cannot find name -'process'`, but pulling `@types/node` into a non-Node package is wrong. + 'process'`, but pulling `@types/node` into a non-Node package is wrong. The shared infra package already ships this ambient: reference **`@mui/internal-code-infra/build-env`** (its `./build-env` export → `src/build-env.d.ts`, which declares a minimal global `process.env`). No @@ -116,7 +113,6 @@ rootDir:"./src"`, include `src/**/*.ts*`, exclude `*.test.ts*`/`*.spec.ts*`. ### 3. Build, diff, fix; iterate until parity is at the proven optimum. ### 4. Quality gates — run all four from the repo root and fix every issue - - **Build:** `pnpm -F build` - **Prettier:** `pnpm exec prettier --check "packages//src/**/*.{ts,tsx}" --ignore-path .lintignore` then `pnpm exec prettier --write …` to fix. (Prettier reformats source only — @@ -124,11 +120,11 @@ rootDir:"./src"`, include `src/**/*.ts*`, exclude `*.test.ts*`/`*.spec.ts*`. - **ESLint:** `pnpm exec eslint packages/ --report-unused-disable-directives --max-warnings 0` (repo runs flat-config ESLint with `--max-warnings 0`; warnings fail CI). - **Typecheck:** `pnpm -F typescript` (covers converted tests + `.spec.tsx`). - Re-run the type-equivalence probe after any lint/format fix that touches source. +Re-run the type-equivalence probe after any lint/format fix that touches source. ## Hard-won findings (apply directly — these are the traps) -- **Babel (`@babel/preset-typescript`, default opts) elides type-only _named_ +- **Babel (`@babel/preset-typescript`, default opts) elides type-only *named* imports but NOT namespace `import * as X` nor `export *`/`export type *` statements.** Consequences: - Namespace type imports → use `import type * as X` (keeps JS clean; cost: @@ -143,7 +139,7 @@ rootDir:"./src"`, include `src/**/*.ts*`, exclude `*.test.ts*`/`*.spec.ts*`. review comment that landed on the `@mui/styled-engine` PR for `import { Global, Interpolation } from '@emotion/react'` (`Interpolation` is type-only). Fix: `import { Global, type Interpolation } from -'@emotion/react'`. Inline `type` and separate `import type` are both + '@emotion/react'`. Inline `type` and separate `import type` are both erased identically by Babel (verified: JS output byte-identical). - Re-export of a type-only module (`export * from '@emotion/styled'`) → use `export type *` so no runtime re-export line is added to the JS (cost: @@ -158,7 +154,7 @@ rootDir:"./src"`, include `src/**/*.ts*`, exclude `*.test.ts*`/`*.spec.ts*`. instead: `export type { A, B, C } from 'pkg';`. Do **not** name-re-export a type you also locally declare (that is a duplicate-export error, whereas `export *` silently lets the local shadow) — only re-export the names the - module does _not_ redeclare. Verify the net surface is unchanged with the + module does *not* redeclare. Verify the net surface is unchanged with the type-equivalence probe (it checks the exported-name set both ways). - **Runtime-wrapper-with-third-party-type default export** (e.g. local `styled` wrapper whose public type must be emotion's `CreateStyled`): annotate @@ -167,125 +163,55 @@ rootDir:"./src"`, include `src/**/*.ts*`, exclude `*.test.ts*`/`*.spec.ts*`. resolved default type is identical to a `export { default } from '...'` re-export. Unavoidable JS cost: `export default function f(){}` becomes `function f(){}; export default f;` (same function, name, binding, semantics). -- **propTypes on a component → reach for the binding shape, not a cast.** - When tsc sees `X.propTypes = …` on a `function X(){}` declaration and `X`'s - declared type doesn't list `propTypes`, it synthesizes - `declare namespace X { var propTypes: any }` in the emitted `.d.ts` and - splits `export default function X` into `declare function X; export default -X;`. The two casts that suppress this both have failure modes: - `X.propTypes = {…} as any` keeps the script happy but **does not** prevent - the namespace (the `as any` is on the value, not the property — tsc still - synthesizes); `(X as any).propTypes = {…}` does prevent the namespace but - trips `typescript-to-proptypes` with - `Expected type "Identifier", got "TSAsExpression"` because the script - `assertIdentifier`s the LHS object. **PR #48578 hit both modes in sequence** - (commit `a8f4580fd0` then `7083171`) — the cast-based options are a false - dichotomy. - - **The fix is the binding shape: make the value receiving `.propTypes` either - (a) already-typed-as-having-propTypes (via `React.ForwardRefExoticComponent` - / `React.NamedExoticComponent`, supplied by `@types/react`), or - (b) a const-bound function expression typed by an interface that lists - `propTypes?: any`.** `function X(){}` declarations are the trap; `const X = -…` bindings are not. With (a) or (b) you can drop every `as any` from the - assignment site — no LHS cast, no RHS cast, no computed-key - `['propTypes' + '']` hack — and both the script and the declaration emit - are clean. - - **Shape 1 — wrapped (`forwardRef` / `React.memo`).** Used by - `mui-material/src/Portal/Portal.tsx`, `mui-x/packages/x-data-grid/src/components/GridRow.tsx`. - The wrapper's return type already declares `propTypes?` (see `@types/react`), - so plain assignment type-checks and tsc has no augmentation to capture: - +- **propTypes on a component creates a tsc expando** → emits + `declare namespace X { var propTypes: any }` and splits + `export default function X` into `declare function X; export default X;`. + Two patterns exist; pick based on whether the file is processed by the + repo's `pnpm proptypes` (`typescript-to-proptypes`) generator — which is + **everything in `@mui/system`, `@mui/material`, `@mui/lab`, `@mui/joy` with + a React component**. + + **Pattern A — `mui-material` convention** (used by `Portal.tsx`, + `FocusTrap.tsx`; mandatory for any file in `typescript-to-proptypes`'s + input list, since the script asserts the propTypes-assignment LHS object + is an `Identifier` and rejects `TSAsExpression`): ```ts - const Portal = React.forwardRef(function Portal(props, ref) { … }); - Portal.propTypes /* remove-proptypes */ = { … }; - ``` - - Emitted: `declare const Portal: React.ForwardRefExoticComponent<…>`. No - namespace. - - **Shape 2 — bare or generic function declaration in source → convert to - typed const-bound function expression.** The bare-function shape (what - hand-written `.js` for non-wrapped components like `ThemeProvider`, - `GlobalStyles`, `DefaultPropsProvider` typically looks like) is the only - one that forces an interface declaration. Declare the value-with-propTypes - type explicitly; annotate the const; assign without casts: - - ```ts - interface ThemeProviderType { - (props: ThemeProviderProps): React.ReactElement>; - propTypes?: any; - } - - const ThemeProvider: ThemeProviderType = function ThemeProvider( - props: ThemeProviderProps, - ): React.ReactElement> { … }; - - ThemeProvider.propTypes /* remove-proptypes */ = { … }; + X.propTypes /* remove-proptypes */ = { + /* ... */ + } as any; if (process.env.NODE_ENV !== 'production') { - ThemeProvider.propTypes = exactProp(ThemeProvider.propTypes); - } - export default ThemeProvider; - ``` - - Emitted: `declare const ThemeProvider: ThemeProviderType`. No namespace. - JS impact: `function X(){}` → `const X = function X(){}` — same identity, - name, length, semantics; precedent already documented above for the - `export default function f(){}` → `function f(){}; export default f;` - case. Mirror the existing `.js` form: same `function X(…)` name in the - expression, same parameter destructuring, same body. - - **Note: a same-name `interface X { propTypes?: any }` declaration-merged - with `function X(){}` is NOT sufficient.** tsc still synthesizes the - namespace even with the interface present — verified empirically. The - const-binding is load-bearing; the interface alone is not. - - **Shape 3 — generic + wrapped (mui-x `DataGrid`).** `forwardRef`/`memo` - don't preserve generic call signatures, so the exported wrapper needs an - explicit interface, and `propTypes` lands on a non-exported inner const: - - ```ts - const DataGridRaw = function DataGrid(inProps, ref) { … }; - interface DataGridComponent { - (props): React.JSX.Element; - propTypes?: any; + // eslint-disable-next-line + (X as any)['propTypes' + ''] = exactProp((X as any).propTypes); } - export const DataGrid = React.memo(forwardRef(DataGridRaw)) as DataGridComponent; - DataGridRaw.propTypes = { … }; ``` - - Emitted: `interface DataGridComponent { …; propTypes?: any; }` and - `export declare const DataGrid: DataGridComponent`. No namespace — - `DataGridRaw` is not exported, so any synthesis on it never reaches the - `.d.ts`. Cite `mui-x/packages/x-data-grid/build/DataGrid/DataGrid.d.ts` as - the live reference. - - **Trailing `/* remove-proptypes */` comment is always required** — - `babel-plugin-transform-react-remove-prop-types` `forceRemoval` keys on it - to keep the `process.env.NODE_ENV !== 'production' ? … : void 0` runtime - wrap. The dev-only `exactProp` reassignment doesn't need the comment - (the `NODE_ENV` env-guard already dead-code-eliminates it in production) - and doesn't need any hack — once the binding is shape 1/2/3, plain - `X.propTypes = exactProp(X.propTypes)` type-checks and the script ignores - the second assignment (it picks up the first one it sees per component). - - **Verification:** after conversion, - `grep -l "declare namespace" packages//build/**/*.d.ts` must return - empty for converted files, and `pnpm proptypes --pattern "/src/()"` - must exit 0 with no working-tree changes. - - **Legacy patterns still present in the tree** (mui-material, pre-PR-48578 - mui-system) used `X.propTypes = {…} as any` (script-compatible, but ships - a `declare namespace` expando in the `.d.ts`) or `(X as any).propTypes = -{…}` (clean `.d.ts`, but script-incompatible). The latter pattern - appeared in earlier revisions of this skill as the recommended default — - it bricks `test_static` and should be replaced with the shape-based fix - above whenever the file is touched. (Plain `X.propTypes = {}` without - any cast on a `function X(){}` declaration emits a propTypes - type-mismatch error at tsc; `(X as any).propTypes = {}` without the - `/* remove-proptypes */` comment drops the Babel guard — JS regression.) - + The `/* remove-proptypes */` trailing comment on the static LHS triggers + `babel-plugin-transform-react-remove-prop-types` `forceRemoval` (keeps the + `process.env.NODE_ENV !== "production" ? … : void 0` wrap). The `as any` + on the RHS satisfies tsc — at the cost of allowing the expando + `declare namespace X { var propTypes: any }` in the emitted `.d.ts`, which + is benign and matches what `mui-material` ships. The dev-only `exactProp` + reassignment uses a computed-key `['propTypes' + '']` LHS so + `typescript-to-proptypes` doesn't try to inject into it; the marker + comment is omitted because the `NODE_ENV` env-guard already dead-code- + eliminates it in production. + + **Pattern B — `styled-engine` convention** (no expando in the emitted + `.d.ts`, but **breaks `typescript-to-proptypes`** with `Expected type + "Identifier", got "TSAsExpression"`): + `(X as any).propTypes /* remove-proptypes */ = { ... };` with + `(X as any).propTypes = exactProp((X as any).propTypes);` for the dev + reassignment. Use **only** when the file is not in `pnpm proptypes`'s + input list — e.g. a styling-only package with no React-component + propTypes (where Pattern A would emit a needless expando but the script + never runs anyway). The original skill commit (and `@mui/private-theming` + PR #48565 review) recommended Pattern B globally; that bricked + `test_static` (Generate PropTypes) on `@mui/system` PR #48578 for + `Box.tsx` and `ThemeProvider.tsx`. Default to Pattern A. + + (Plain `X.propTypes = {}` without `as any` — no cast at all — keeps the + guard but emits a propTypes type-mismatch error at the tsc step. `(X as + any).propTypes = {}` without the `/* remove-proptypes */` comment drops + the Babel guard — JS regression.) - **Wildcard `package.json` `exports` resolve only the wildcard's literal extension.** A catch-all like `"./*": "./src/*/index.ts"` only resolves directories whose `index` file is `.ts`. If the conversion leaves any @@ -298,25 +224,25 @@ X;`. The two casts that suppress this both have failure modes: build won't catch this — only downstream bundling does (CI surfaces it as `test_bundle_size_monitor` failure). The set of explicit entries to keep around therefore mirrors the set of dirs still on `.js`. -- **`stripInternal: true` + `/** @internal _​/`** on a declaration removes -runtime-only exports (e.g. `TEST*INTERNALS_DO_NOT_USE`) from emitted `.d.ts`. -**An export that exists at runtime but is missing from the hand-written -`.d.ts`is a judgement call, not a mechanical fix** — it can be either -(a) intentionally private (the most-faithful conversion is to formalize that -with`@internal`+`stripInternal`, keeping it out of the emitted `.d.ts`) or -(b) a bug in the hand-written `.d.ts`(the most-faithful conversion is to -let`tsc` emit the real declaration — an additive, non-breaking surface -change that fixes the omission). Use the signals — naming (`\_internal*_`, -`_*DO_NOT_USE`, `TEST*_`strongly suggest private), prior intent in surrounding -comments, whether external consumers reasonably need it — and **surface the -call to the user with your recommendation before applying**; never silently -mark a runtime export`@internal`. Note: `@internal` on an _expando - assignment_ does NOT propagate — only on the declaration itself. +- **`stripInternal: true` + `/** @internal *​/`** on a declaration removes + runtime-only exports (e.g. `TEST_INTERNALS_DO_NOT_USE`) from emitted `.d.ts`. + **An export that exists at runtime but is missing from the hand-written + `.d.ts` is a judgement call, not a mechanical fix** — it can be either + (a) intentionally private (the most-faithful conversion is to formalize that + with `@internal` + `stripInternal`, keeping it out of the emitted `.d.ts`) or + (b) a bug in the hand-written `.d.ts` (the most-faithful conversion is to + let `tsc` emit the real declaration — an additive, non-breaking surface + change that fixes the omission). Use the signals — naming (`_internal_*`, + `*_DO_NOT_USE`, `TEST_*` strongly suggest private), prior intent in surrounding + comments, whether external consumers reasonably need it — and **surface the + call to the user with your recommendation before applying**; never silently + mark a runtime export `@internal`. Note: `@internal` on an *expando + assignment* does NOT propagate — only on the declaration itself. **Per-item triage for packages with many undeclared runtime exports.** On packages with a dozen-plus such leaks (e.g. `@mui/system` had ~26 across 7 dirs), the judgement scales by shape: a runtime export that mirrors the - _form_ of declared siblings — `outline`/`outlineColor` next to `border`/etc., + *form* of declared siblings — `outline`/`outlineColor` next to `border`/etc., or `displayPrint`/`displayRaw`/`overflow`/`textOverflow`/`visibility`/ `whiteSpace` when the dir's compose-default treats them uniformly — is almost certainly an oversight-(b) case; let `tsc` emit it and call out the @@ -332,14 +258,13 @@ mark a runtime export`@internal`. Note: `@internal` on an _expando `@internal` is referenced transitively in another emitted type (e.g. as a literal key in a `PartiallyRequired` constraint, or anywhere `keyof` the stripped surface is consumed), turning on `stripInternal` will break the - downstream consumer's declaration build — _not_ the converted package's own + downstream consumer's declaration build — *not* the converted package's own build, which makes the failure easy to miss locally if Verification step 6 is skipped. Before enabling: `git grep "@internal" packages//src/`, and for each match check whether the symbol is reachable through the public type surface anyway. If it is — as `Grid.unstable_level` was via `GridOwnerState` in `@mui/system` — the pre-existing `@internal` was lying; drop the tag rather than letting the strip break consumers. - - `tsc` always emits `declare` on function declarations in `.d.ts`; hand-written baselines often omit it. `export function f` vs `export declare function f` are identical in a declaration file — document, don't chase. @@ -355,13 +280,10 @@ mark a runtime export`@internal`. Note: `@internal` on an _expando built package (baseline snapshot, new build) with their `package.json` `exports`; write `probe.ts`: ```ts - type Equal = - (() => T extends X ? 1 : 2) extends () => T extends Y ? 1 : 2 ? true : false; + type Equal = (()=>T extends X?1:2) extends (()=>T extends Y?1:2) ? true : false; type Expect = T; - import * as Base from './base'; - import * as New from './new'; - import BaseDefault from './base'; - import NewDefault from './new'; + import * as Base from './base'; import * as New from './new'; + import BaseDefault from './base'; import NewDefault from './new'; // strict Equal for every exported type (instantiate generics with a // representative Props); bidirectional `const a: typeof Base.x = New.x` // for value exports; Equal, never> @@ -372,19 +294,19 @@ mark a runtime export`@internal`. Note: `@internal` on an _expando 4. `pnpm -F typescript` (includes converted tests + `.spec.tsx`). 5. `pnpm -F test` (node + browser projects). 6. **Build every downstream consumer, not just typecheck it.** `pnpm -F - typescript` uses `tsconfig.json` and will _not_ catch the failure + typescript` uses `tsconfig.json` and will *not* catch the failure that matters here: once the converted package's `exports` point at - `./src/index.ts`, a consumer's _declaration build_ + `./src/index.ts`, a consumer's *declaration build* (`tsc -p /tsconfig.build.json --rootDir /src`) pulls the converted package's `.ts` **source** into its program and fails with `TS6059: File '…' is not under 'rootDir'` (previously it consumed the hand-written `.d.ts` as an external/ambient file). Run the real build of the full dependency chain in topological order, e.g. `pnpm -F @mui/types build -&& pnpm -F @mui/utils build && pnpm -F build && pnpm -F -build`. CI (`release:build`) does this; a bare `typescript` check does not — + && pnpm -F @mui/utils build && pnpm -F build && pnpm -F + build`. CI (`release:build`) does this; a bare `typescript` check does not — this is exactly how the `@mui/styled-engine` PR's first CI run failed (`test-dev`, `pkg.pr.new`, `test_bundle_size_monitor`, `test_regressions`). - **Fix:** add the converted package as a project reference in _every_ + **Fix:** add the converted package as a project reference in *every* downstream package that builds via `tsc` and imports it — add `{ "path": "..//tsconfig.build.json" }` to that consumer's `tsconfig.build.json` `references` array (the converted package's own @@ -400,7 +322,7 @@ build`. CI (`release:build`) does this; a bare `typescript` check does not — State which contract clauses hold, and for every residual JS/`.d.ts` difference give the exact before→after and its root cause (from Findings above). If the -spec demands byte-identical JS _and_ `.d.ts` _and_ no hand `.d.ts` for a file +spec demands byte-identical JS *and* `.d.ts` *and* no hand `.d.ts` for a file whose original diverged, state the impossibility, prove it, deliver the type-identical optimum, and stop — do not loop.