Document private JSDoc typedef convention - #1461

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 60a7015674

Approving. This is the standing decision I'd been asking for across the last four iterations, and it's a better answer than the one I suggested — a documented API contract with an upstream exit path, rather than a per-module judgment call.

The premise checks out, and it's worse than "a property of @typedef"

I tested @internal + stripInternal on TypeScript 7.0.2 before accepting the workaround, and the contrast is sharp:

// authored .ts
/** @internal */ export type TsSecret = { a: number }
  -> emitted .d.ts:   TsSecret is STRIPPED

// authored .mjs
/** @internal @typedef {{ a: number }} SecretType */
  -> emitted .d.mts:  export type SecretType = { a: number };   // still there

So stripInternal works fine — it just doesn't understand JSDoc typedefs. (The @internal tag even survives into the emitted declaration, with no effect.) That means this isn't only an inherent limitation of @typedef; it's a capability the codebase had while authored in TypeScript and loses at the moment each module migrates. Worth framing that way in the blocked TODO, because it makes the case for #46407 concrete: this is a migration-induced regression, not a feature request.

Every upstream citation is accurate, including the open/closed labels:

microsoft/TypeScript#46407   open    Support stripInternal for types in JSDoc
microsoft/TypeScript#62453   open    JSDoc comments emitted a second time...
microsoft/typescript-go#4363 open    Comment above @typedef does not become comment...
microsoft/typescript-go#4235 closed  (labeled closed in the doc ✓)
microsoft/typescript-go#4011 closed  (labeled closed in the doc ✓)

Labeling the two closed ones as closed, and calling them "adjacent declaration-emitter bugs, not substitutes", is exactly right — it would have been easy to pad the blocker list with them.

The convention itself is well-drawn: _ as an API contract rather than declaration visibility, with the explicit statement that changing or removing a _-prefixed typedef needs no **BREAKING CHANGES:** entry. That's the part that makes it enforceable in review.

One gap: the backlog it was created for isn't scheduled

The convention governs future migrations, but nothing applies it to the types already promoted. All ten are publicly importable from main today — I verified by importing them:

fjs/types/function   Fn
fjs/types/list       NotLazy   Empty   Concat
fjs/types/bit_vec    Norm   NormOp   Base   UnpackConcat   ListToVecState   ListToVecOp

(That's every one I flagged in #1453/#1458/#1460; the X from #1454 isn't in the list because you deleted it outright.)

Without a rename task these keep their public, un-prefixed names, so the convention can't be read off the source — someone looking at Base or Empty in six months has no way to tell it was private pre-migration. The information needed to fix it is cheapest right now, while the pre-migration .ts is a short git show away.

A single task in migrate-typescript-to-mjs.md — rename those ten with the _ prefix, no changelog entry needed by this PR's own rule — would close it. Not blocking; the convention is worth landing either way.

Still approving at ee549f70. One commit since my last look, and it's the right call.

Removing the duplicated policy from AGENTS.md matches this repo's own discipline — AGENTS.md points at authoritative documents rather than restating them, and it already links fjs/fsc/README.md twice (lines 154 and 184) for exactly these extension and migration conventions. So the policy stays discoverable through the existing pointer, and there's no second copy to drift. AGENTS.md ends up byte-identical to main, which is the correct net result.

The assignability caveat you added is load-bearing

This wasn't in my suggestion and it's the sharper half of the rule:

_-prefixed typedef names are private even when declarations emit them as exports, but still require **BREAKING CHANGES:** whenever a change to one alters the assignability of a public declaration.

I checked whether that can actually bite, and it can — the emitted declaration references the private alias by name rather than inlining it:

// authored m.mjs
/** @typedef {{ a: number }} _Internal */
/** @type {(x: _Internal) => number} */
export const pub = x => x.a

// emitted m.d.mts
export type _Internal = { a: number; };
export declare const pub: (x: _Internal) => number;   // <-- by name

So a consumer's use of pub structurally depends on _Internal's shape. Without that caveat, the _ convention would have licensed silently breaking changes to public signatures — the name is private, but the shape is reachable through every public declaration that mentions it. Good catch; the rule is materially safer with it than the version I described.

Still open (repeating briefly, not re-arguing)

No task yet covers the ten typedefs already promoted to public in iterations 3–6 — the tasks all read "when converting", which is forward-looking only. The list is in my previous review. Non-blocking, and cheapest to do while the pre-migration .ts is one git show away.

CI green, 19/19.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: ee549f702b

Still approving at cdaeaa43. Both new commits are good, and one of them closes a hole I'd only half-identified.

cdaeaa43 — the compatibility examples are correct. I checked them rather than eyeballing:

_Internal = number ; Public = readonly [_Internal]
  -> readonly [number]           Equal => true    not breaking  ✓
  -> _Internal = string          Equal => false   breaking      ✓

Exactly one TS2344, on the case the doc calls breaking. The "expanded public contract" framing is the right way to state it — it's the distinction between removing a private alias (safe, the contract survives inlining) and redefining one (unsafe, the contract changes underneath a public name).

63b557f5 — task 4 is the one that matters:

Refactor public declarations that refer to private typedef names so they remain self-contained and preserve the same public assignability contract before those private typedefs are stripped.

That's the sequencing hazard I demonstrated last round, turned into a prerequisite. Worth being explicit about why it's load-bearing: since the emitted declaration references _Internal by name, enabling stripInternal without doing this first would delete a type that public declarations still point at — trading a visibility leak for broken declarations. Ordering it ahead of the strip is what makes the eventual cleanup safe.

The one remaining gap, stated once and then I'll drop it

The ten typedefs promoted in iterations 3–6 are still public and un-prefixed on this branch (re-verified by importing them). I've raised this twice as a tidiness point; there's a sharper reason to care that I hadn't articulated:

The cleanup you just wrote depends on the prefix. Its tasks are "mark implementation-only JSDoc typedefs with @internal" and "remove leading _ where the prefix exists only as the workaround" — both of which need to identify which typedefs are implementation-only. For everything migrated from here on, the _ prefix carries that intent in the source. For those ten, nothing does. Whoever runs the cleanup will have no way to tell that Base, Empty or Norm were ever private, short of archaeology against pre-migration .ts files.

So the prefix isn't cosmetic — it's the record the future task reads. A one-line task to apply it to the existing ten would make the cleanup mechanical instead of investigative. Still not blocking, and this PR is clearly worth landing as-is.

CI green, 19/19.