Migrate fjs/dev/update to .f.mjs - #1491
Conversation
Leaf module with no local types to split — reuses Mkdir/NodeProgram/ ReadFile/WriteFile/Effect from effects/node and effects. proof.f.ts stays TypeScript: it has a runtime dependency on effects/node/virtual/module.f.ts, still unmigrated.
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
functionalscript | fac21a6 | Commit Preview URL Branch Preview URL |
Aug 11 2026, 04:53 PM |
Leaf module with no local types and no dependents. proof.f.ts stays TypeScript: it has a runtime dependency on effects/node/virtual/module.f.ts, still unmigrated. Updates the stale line-number citation in effects' map-step-combinator todo doc.
dev-update and index-html hardcoded ./fjs/dev/update/module.f.ts and ./fjs/website/module.f.ts, both migrated to .f.mjs earlier in this PR. This broke the Cloudflare Workers Build, which runs index-html to generate the deployed site. Verified both scripts run clean and produce unchanged output.
o2alexanderfedin
left a comment
o2alexanderfedin
left a comment
There was a problem hiding this comment.
Approving. Reviewed at a0bab54 against main (dff17b0). Two modules — fjs/dev/update and fjs/website. CI green, nothing to flag.
The package.json fix is the interesting part
dev-update and index-html pointed at module.f.ts paths this PR renames. That is a class neither tsc nor the test suite can catch — the scripts are only exercised when someone runs them — so it would have sat broken until it bit somebody. Both run now:
$ npm run dev-update → OK
$ npm run index-html → OKI swept the rest for the same shape and found none remaining: every .f.ts/.f.mjs path in package.json and the other config files resolves, and .github has no such references at all.
Everything else
npx tscclean; 2357 pass / 0 fail, matchingmain.- Both modules are token-identical to
main's compiled output. - Public API completely untouched — 0 removed / 0 added / 0 changed for exported type aliases and exported const signatures. That is what a leaf migration with no local types should look like, and it matches the summary's claim that no
types.tscompanion was needed. - No
anyas a type in any emitted declaration. - Links 144/144, none added, 0 label/target mismatches, and no stale
.f.tsreferences to either migrated module outside the CHANGELOG entries themselves. - CHANGELOG covers both.
The proof.f.ts rationale is accurate this time, which I checked rather than assumed given it went stale on #1488: both proofs import virtual and emptyState from effects/node/virtual/module.f.ts, and that module is still TypeScript on disk. So §2 genuinely blocks proof.f.mjs for both, and the summary says so correctly.
proof.f.ts stays TypeScript for now: it has a runtime dependency on effects/node/virtual/module.f.ts, still unmigrated. Updates the cas/cli and top-level module.f.ts dependents, plus stale module.f.ts doc references (including line-number citations) across README.md and five todo docs.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4770078e7d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| import type { Effect } from '../effects/types.ts' | ||
|
|
||
| /** @internal */ | ||
| export type _Handler<O extends NodeOp> = (options: NodeProgramOptions) => Effect<O, number> |
There was a problem hiding this comment.
_Handler private
This migration changes the previously module-private Handler alias into an exported _Handler; because stripInternal is disabled, the generated types.d.ts retains that export and package consumers can import it despite the @internal tag. That unnecessarily expands the public API and creates a compatibility obligation for an implementation detail. Keep the alias non-exported, as it was before the migration. The repository requires migrations to preserve public assignability and declaration-emission behavior.
AGENTS.md reference: AGENTS.md:L449-L454
Useful? React with 👍 / 👎.
types.ts is real TypeScript, not JSDoc-constrained, so a non-exported type can be referenced by an exported type in the same file without issue — the underscore-export convention only applies where a JSDoc @typedef forces the export. Handler was never imported from module.f.mjs, so exporting it needlessly widened the declared surface.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3098a083d3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| * | ||
| * @module | ||
| */ | ||
| import { errorExit, log } from '../effects/node/module.f.mjs' |
There was a problem hiding this comment.
When declaration files are emitted, placing the first import immediately after the closing @module block causes TypeScript to drop the module documentation from the generated .d.mts. The same adjacency occurs in fjs/dev/update/module.f.mjs:6 and fjs/website/module.f.mjs:6, so all three newly migrated modules lose their module-level documentation in the published declarations; insert a blank line after each header.
Useful? React with 👍 / 👎.
| fjs/module.ts — Node.js entry point (runs main via the node runner) | ||
| fjs/module.f.ts — FunctionalScript command dispatcher (Commands list + dispatch) | ||
| fjs/cli/module.f.ts — generic Command/Commands/dispatch primitives | ||
| fjs/cli/module.f.mjs — generic Command/Commands/dispatch primitives |
There was a problem hiding this comment.
This rename updates the architecture reference, but fjs/cas/README.md:13 still links to cli/module.f.ts, which no longer exists after this change. Readers following the documented CAS CLI link therefore land on a missing file; update it to cli/module.f.mjs as part of the rename.
Useful? React with 👍 / 👎.
Without it, TypeScript's declaration emit drops the module doc comment entirely from the generated .d.mts — verified against the actual emitted output before and after.
|
Checked both findings.
Generated by Claude Code |
|
@codex review |
o2alexanderfedin
left a comment
o2alexanderfedin
left a comment
There was a problem hiding this comment.
Re-approving at d791a69. fjs/cli joins, and the two follow-up commits are both pre-empting things that came up earlier in this migration rather than waiting for review to raise them.
Handler stays private — and this is the better fix
3098a08 keeps Handler non-exported in fjs/cli/types.ts. Verified against main: it emits as a bare type Handler<O extends NodeOp> = … on both sides, no export, so the privacy is exactly preserved.
Worth naming why this is cleaner than the _Byte/_NodeList/_Type0 outcomes earlier: those types had to move into JSDoc @typedefs, which have no non-exported form, so the _ prefix was the only marker available. Handler moved into a types.ts — real TypeScript — where a plain non-exported type is expressible. Reaching for the stronger option when the file format allows it is the right instinct.
The @module blank lines are load-bearing
d791a69 adds a blank line after the @module block in fjs/cli/types.ts, fjs/dev/update/module.f.mjs, and fjs/website/module.f.mjs. All three have an import as the next statement, which is precisely the shape where TypeScript folds the header into the import's leading comment and drops it on elision. I confirmed the header survives emit in all three (types.js for cli, the .d.mts for the other two). Fixing that at authoring time rather than discovering it in a later sweep is the outcome I was hoping for after the 18-file case.
Everything re-verified at this head
npx tscclean; 2357 pass / 0 fail, matchingmain.- All three modules token-identical to
main's compiled output. - Public API untouched: 0 removed / 0 added / 0 changed, on both exported type aliases and exported const signatures.
- No
anyas a type in any emitted declaration. - Links 144/144, none added, 0 label/target mismatches.
- Every
.f.ts/.f.mjspath inpackage.jsonstill resolves — includingfjs/cli's arrival not stranding one. - CHANGELOG covers all three.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d791a69e1f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ### Problem | ||
|
|
||
| `Command` (`fjs/cli/module.f.ts`) hands every handler a raw | ||
| `Command` (`fjs/cli/module.f.mjs`) hands every handler a raw |
There was a problem hiding this comment.
After this migration, Command and Commands are exported only from fjs/cli/types.ts, but this updated reference—and the corresponding ownership claims in dispatch-help-rendering.md, positional-arity-check.md, fjs/todo/66g-fjs-run-commands.md, and fjs/README.md—still directs readers to module.f.mjs. Anyone implementing these issue designs would look for or import types from a runtime module that does not export them; update type-level references to types.ts while retaining module.f.mjs references for dispatch and runtime combinators.
AGENTS.md reference: AGENTS.md:L449-L454
Useful? React with 👍 / 👎.
Command and Commands are exported only from types.ts after the migration; dispatch stays correctly cited in module.f.mjs. Also refreshes dispatch's stale line-number citations (:18-49 -> :18-50).
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
#1491 migrated fjs/dev/update — the module this one is modelled on — so the writer's whole runtime closure is now .f.mjs and nothing blocks it. Shipping a new authored .f.ts implementation would work against the stage-1 migration. The proof stays .f.ts: it needs `virtual` from effects/node/virtual, which is still TypeScript. Same split fjs/dev/update has. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FyMg2KPgfJeQ5FXGXjyFmd
3 participants
Summary
fjs/dev/update/module.f.tsfrom authored TypeScript to JSDoc-typed.f.mjs, pertodo/migrate-typescript-to-mjs.md.Mkdir/NodeProgram/ReadFile/WriteFilefromeffects/node/types.tsandEffectfromeffects/types.ts, so notypes.tscompanion is needed.proof.f.tsstays TypeScript for now — it has a runtime dependency oneffects/node/virtual/module.f.ts, still unmigrated.dev/update.Test plan
npx tsc --noEmitcleannode ./fjs/module.ts t— 2357/2357 passGenerated by Claude Code