TS => MJS. Iteration 5 - #1458
Conversation
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
functionalscript | 219bda3 | Commit Preview URL Branch Preview URL |
Aug 08 2026, 11:57 PM |
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
o2alexanderfedin
left a comment
There was a problem hiding this comment.
Reviewed at 783e8683. Flagging one thing before it goes further — it's a draft, so this may already be on your list.
fjs/effects/module.f.mjs is a dead duplicate
The migration is half-applied for effects. Both files exist on this branch:
fjs/effects/module.f.mjs 591 lines (added)
fjs/effects/module.f.ts 491 lines (still here; only its imports were updated)
and nothing uses the new one:
imports of effects/module.f.ts → 34
imports of effects/module.f.mjs → 0
So the 591-line .mjs is unreferenced, and the .ts is still the live module for all 34 importers. That's why the suite is green and tsc is clean — the new file simply isn't reachable, so it proves nothing yet.
Two side effects worth knowing while it sits there: shouldLoad matches .f.mjs (fjs/dev/module.f.ts:53), so the loader does discover it, and files includes **/*.mjs, so it would ship in the npm archive as a second copy of the module.
Contrast with list, result, and monoid in this same PR, which are done properly — .mjs added, .ts removed, importers rewritten. effects looks like it stopped after step one.
CHANGELOG is missing fjs/common/monoid
The entry names only fjs/types/list and fjs/types/result, but fjs/common/monoid migrates here too (.f.ts on main → .f.mjs on this branch), so its importers need the new specifier as well. Same omission shape as fjs/types/ts in #1454. effects will need a line too, once it's actually migrated.
Type visibility, again
fjs/types/list had three types declared without export; as JSDoc typedefs they're now public. Confirmed both directions:
- on
main:TS2459: Module … declares 'NotLazy' locally, but it is not exported, plus TS2724 forEmptyandConcat - on this branch: all three import cleanly from
module.f.mjs
Empty is the one I'd think twice about — it's a very general name to add to the package's public type surface, and it sits next to the existing exported value empty. Same class as Fn in #1453 and X in #1454; at this point it's a predictable consequence of every @typedef, so a standing decision in the migration doc would probably serve better than case-by-case.
Verified good
The part I most wanted to check holds: Cont and Do keep @template {Operation} out O, and the variance is real, not decorative —
Effect<ReadOp> → Effect<ReadOp | WriteOp> accepted (widening, the point of `out`)
Effect<ReadOp | WriteOp> → Effect<ReadOp> TS2322 (narrowing, correctly refused)
That's the Cont<out O, T> / Do<out O, T> contract from #1449 surviving translation in the real module rather than in my probe, which was the open question when the blocker was dropped.
Also: npx tsc clean, npm test → pass: 2354, fail: 0, total: 2354, and CI is now fully green (19/19).
o2alexanderfedin
left a comment
There was a problem hiding this comment.
Re-checked at fc3b5847.
CHANGELOG fixed — fjs/common/monoid is in the entry now, alongside list and result. Thanks.
The other two from my last review are unchanged, which is fine for a draft — just noting them so they don't get lost:
fjs/effectsis still half-migrated. Both files remain, and the split is unchanged: 34 importers onmodule.f.ts, 0 on the new 591-linemodule.f.mjs. Until an importer moves, the new file is unreachable and the green suite says nothing about it. It'll also need its own CHANGELOG line once it actually lands.NotLazy/Empty/Concatare still private-on-main, public here.
CI is green (19/19) and main hasn't moved under you.
o2alexanderfedin
left a comment
There was a problem hiding this comment.
Reviewed at 777fd2c3. This PR left draft since my last pass, which changes the weight of the one open item — it's now mergeable.
fjs/effects is still half-migrated. Unchanged from two passes ago: 34 importers on module.f.ts, 0 on the 591-line module.f.mjs. The file statuses show it more clearly than any description:
R069 fjs/common/monoid/module.f.ts -> module.f.mjs
R064 fjs/types/bigint/module.f.ts -> module.f.mjs
R052 fjs/types/result/module.f.ts -> module.f.mjs
A fjs/types/list/module.f.mjs
D fjs/types/list/module.f.ts
A fjs/effects/module.f.mjs <-- no matching D
Every other module is a rename, or an add paired with a delete. effects is the only add with nothing removed. While it stays that way the new file is unreachable, so the green suite and clean tsc say nothing about it — and merging ships a second, dead copy of the module in the package (files includes **/*.mjs).
Since it's mergeable now, I'd either finish it (switch the 34 importers, delete the .ts, add its CHANGELOG line) or drop fjs/effects/module.f.mjs from this PR and land it whole in Iteration 6. Either is fine; shipping it half-applied is the option I'd avoid.
Verified on this head
fjs/types/bigint (new since my last pass) is a clean conversion. Export surface is byte-identical — 22 names, no additions or losses — and the types are genuinely enforced, not collapsed to any: abs(5) gives TS2345: Argument of type 'number' is not assignable to parameter of type 'bigint', and assigning abs(-5n) to string gives TS2322. It also has no unexported type declarations, so the visibility issue doesn't arise for it.
One thing I checked and want to state plainly so it isn't mistaken for a finding: sign returns Sign (-1 | 0 | 1, numbers) rather than bigint. That surprised me, but it's identical on main — I ran the same assignment against both and got the same TS2322 either way. No change here.
CHANGELOG kept up — fjs/types/bigint was added along with the migration, without being asked. It'll still need fjs/effects if that lands here.
Still open: NotLazy / Empty / Concat in fjs/types/list remain private on main and public here.
npx tsc clean; npm test → pass: 2354, fail: 0, total: 2354.
o2alexanderfedin
left a comment
There was a problem hiding this comment.
Approving. The half-migrated fjs/effects is resolved — you took the cleaner option and dropped module.f.mjs from this PR, and the removal is clean: no reference to it survives anywhere in the tree.
What's left is uniform, which is the point:
R069 fjs/common/monoid/module.f.ts -> module.f.mjs
R064 fjs/types/bigint/module.f.ts -> module.f.mjs
R052 fjs/types/result/module.f.ts -> module.f.mjs
A/D fjs/types/list/module.f.{mjs,ts}
A/D fjs/types/nominal/module.f.{mjs,ts}
Every module is now a rename or a matched add/delete — no stray A without its D. The CHANGELOG lists all five, including nominal, which you added without being asked.
fjs/types/nominal checks out: exports are Nominal / asNominal / asBase before and after, Nominal was already exported so nothing changes visibility, and the @template {string} N constraints carry over intact.
npx tsc clean, npm test → pass: 2354, fail: 0, total: 2354, CI 19/19.
The one standing item
NotLazy / Empty / Concat in fjs/types/list are still private on main and public here (re-confirmed on this head). I'm not blocking on it — it's the same non-blocking observation I made for Fn in #1453 and X in #1454, and it's an unavoidable property of @typedef rather than anything wrong with this PR.
But that's three iterations in a row, and Empty is the least comfortable one yet given how general the name is. It's probably worth one line in the migration doc — either "unexported type aliases become public on migration; accepted" or "audit and re-privatize after stage 1" — so the remaining iterations have a rule to apply instead of each one being a judgment call.
Continues the
.ts=>.mjsmigration.Converted modules
fjs/types/list/module.f.ts=>fjs/types/list/module.f.mjsfjs/types/result/module.f.ts=>fjs/types/result/module.f.mjsTypeScript type declarations are replaced by JSDoc (
@typedef,@template,@import), and all importers across the repo are updated to point at the new.mjspaths.Also adds
fjs/effects/module.f.mjsas a converted copy offjs/effects/module.f.ts. The.tsversion is still present and is what everything imports, so this file is currently unused groundwork for the next iteration.🤖 Generated with Claude Code