Skip to content

Migrate fjs/media/json/parser to .f.mjs - #1499

Merged
sergey-shandar merged 8 commits into
mainfrom
next
Aug 12, 2026
Merged

Migrate fjs/media/json/parser to .f.mjs#1499
sergey-shandar merged 8 commits into
mainfrom
next

Conversation

@sergey-shandar

@sergey-shandar sergey-shandar commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Migrates fjs/media/json/parser from authored TypeScript (.f.ts) to JSDoc-typed JavaScript (.f.mjs) under the stage-1 TypeScript-to-mjs migration
  • Splits the internal _JsonObject, _JsonArray, _JsonStack, _StateParse, and _JsonState types into a sibling types.ts
  • proof.f.ts stays TypeScript for now — it still depends on fjs/media/json/module.f.ts, which hasn't been migrated
  • Updates the two importers (fjs/media/json/module.f.ts, fjs/media/json/parser/proof.f.ts) to the .f.mjs specifier

BREAKING CHANGES: importers of fjs/media/json/parser/module.f.ts must switch to the .f.mjs specifier for the runtime value and the new types.ts specifier for the internal types.

Test plan

  • Existing proof.f.ts suite covers parser behavior (valid/invalid JSON, deep-nesting regression cases) and passes unchanged

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

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 o2alexanderfedin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@sergey-shandar sergey-shandar changed the title 1 Migrate fjs/media/json/parser to .f.mjs Aug 12, 2026
sergey-shandar and others added 2 commits August 12, 2026 00:53
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>
@sergey-shandar

Copy link
Copy Markdown
Contributor Author

Thanks for the review — addressed:

  • Added the @module header to fjs/media/json/parser/types.ts, with a blank line before the import type lines so it isn't folded into the first import's leading trivia. Verified with npm run prepack that @module survives into the emitted types.d.ts. Re-ran npx tsc --noEmit and npm test (4168/0) — both clean.

  • Re: the CHANGELOG entry — you reviewed at 46f7ddd8 ("2"), which only split the types into types.ts and hadn't yet done the module.f.tsmodule.f.mjs rename. That rename landed in the next commit (5b4d3a52, "3") and is part of this PR's final diff: fjs/media/json/module.f.ts and parser/proof.f.ts now import ./parser/module.f.mjs instead of ./parser/module.f.ts. That specifier change is breaking for any external importer, same as the pattern in Migrate fjs/effects/node/virtual to .f.mjs #1494/Split JSON rtti schemas into .f.mjs and JSON types into types.ts #1498, so I kept the CHANGELOG entry — it documents the .f.mjs migration itself, not the private type split.

@o2alexanderfedin o2alexanderfedin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@sergey-shandar
sergey-shandar marked this pull request as ready for review August 12, 2026 08:22
@sergey-shandar
sergey-shandar added this pull request to the merge queue Aug 12, 2026
Merged via the queue into main with commit 6adc8d3 Aug 12, 2026
19 checks passed
@sergey-shandar
sergey-shandar deleted the next branch August 12, 2026 08:28
@o2alexanderfedin o2alexanderfedin mentioned this pull request Aug 12, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants