Migrate fjs/media/json/parser to .f.mjs - #1499
Conversation
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
functionalscript | 0da8576 | Commit Preview URL Branch Preview URL |
Aug 12 2026, 08:22 AM |
o2alexanderfedin
left a comment
There was a problem hiding this comment.
Reviewed at 46f7ddd88d43920c95d9cdf59d62a8b021b74cc5. Early draft — title "1", empty body —
so treat this as a first pass. One small thing to fix; the substance is right.
The one thing
fjs/media/json/parser/types.ts has no @module header (§4). It opens straight into
import type. Both new files in #1498 ended up with one, including its types.ts, so this is
the same gap that PR closed.
Same placement caution applies here as there, and it matters more in this file than most:
every import in it is import type, which is exactly the shape where a header written flush
against the first import gets folded into that import's leading trivia and disappears from the
emitted declaration. #1498's fjs/media/json/types.ts solved it with a blank line between the
header and the imports, and I confirmed there that @module survives emit.
What's right
The privacy handling is the part I'd have scrutinised hardest, given how the equivalent split
went on #1494, and it's correct here. All five types the new types.ts exports —
_JsonObject, _JsonArray, _JsonStack, _StateParse, _JsonState — carry the _ prefix,
and diffing the emitted type surface against main confirms there are no unprefixed
additions. _JsonStackElement, _StateResult and _StateError stay non-exported, which
matches how fjs/types/rtti/types.ts and #1498's types.ts handle their own internals.
Nothing public moved: const signatures are identical to main, and
fjs/media/json/parser/module.f.ts still exports exactly parse and proof. That's also why
I don't think this needs a CHANGELOG entry — unlike #1498, which moved genuinely public types
and forced importers to change specifiers, every name added here is private by the _
contract and no importer outside the module is affected. Flagging that I considered it rather
than that I want one.
npx tsc --noEmit exits 0, npm run prepack exits 0 with 0 errors from a clean tree,
npm test gives 2368/0 — identical to main, as a pure type relocation should be — and
broken markdown links are 144 on both sides.
One note in case anyone diffs this the way I first did: the branch is one commit behind
main (#1496), so a two-dot git diff main makes it look like AGENTS.md §5.8 is being
deleted. It isn't — the PR touches only the two parser files, and the merge will keep #1496's
section.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Every import in this file is import type, so a header without a blank line before the imports gets folded into the first import's leading trivia and dropped from the emitted declaration (per review on #1499, following the same fix applied in #1498). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Thanks for the review — addressed:
|
o2alexanderfedin
left a comment
There was a problem hiding this comment.
Reviewed at d0d322c89d43deaeaddef663378ac723501d2ddc. The @module header is in, the scope
has grown from a type split into the full .f.mjs migration, and everything checks out.
Approving (still marked draft — flip it when you're ready).
The header. Present on both parser/module.f.mjs and parser/types.ts, with the blank
line before the imports, and it survives declaration emit in both — @module shows up in
parser/module.f.d.mts and in parser/types.d.ts. That second one is the case that mattered:
every import in types.ts is import type, which is the shape that swallowed headers earlier
in this series.
The migration is a clean one. Normalising away comments, semicolons and specifier
extensions, this branch's authored parser/module.f.mjs is token-identical to main's
compiled parser/module.f.js. No extraction idiom was needed anywhere — no annotated locals
standing in for call-site type arguments, which is unusual for this series and worth noting
because it means there's nothing behavioural to argue about.
Privacy. Five types added to the emitted surface — _JsonObject, _JsonArray,
_JsonStack, _StateParse, _JsonState — all _-prefixed, no unprefixed additions.
_JsonStackElement, _StateResult and _StateError stay non-exported. Const signatures are
identical to main, so nothing public moved except the specifier.
CHANGELOG. One entry, correctly scoped, links only the PR, in the usual band. I checked
its stated reason rather than taking it at face value, since a stale version of exactly this
claim slipped through on #1494: proof.f.ts really does still import stringify from
../module.f.ts, which really is still authored TypeScript, so "stays TypeScript for now"
is accurate and §2 is satisfied.
Everything else. npx tsc --noEmit 0, npm run prepack 0 errors from a clean tree,
npm test 2368 pass / 0 fail — identical to main, as a behaviour-preserving migration should
be — broken markdown links 144 on both sides, and the branch is level with origin/main.
Summary
fjs/media/json/parserfrom authored TypeScript (.f.ts) to JSDoc-typed JavaScript (.f.mjs) under the stage-1 TypeScript-to-mjs migration_JsonObject,_JsonArray,_JsonStack,_StateParse, and_JsonStatetypes into a siblingtypes.tsproof.f.tsstays TypeScript for now — it still depends onfjs/media/json/module.f.ts, which hasn't been migratedfjs/media/json/module.f.ts,fjs/media/json/parser/proof.f.ts) to the.f.mjsspecifierBREAKING CHANGES: importers of
fjs/media/json/parser/module.f.tsmust switch to the.f.mjsspecifier for the runtime value and the newtypes.tsspecifier for the internal types.Test plan
proof.f.tssuite covers parser behavior (valid/invalid JSON, deep-nesting regression cases) and passes unchanged