Skip to content

media/lock: drop the LockMap re-export - #1647

Merged
sergey-shandar merged 8 commits into
mainfrom
dont-rexport
Aug 20, 2026
Merged

media/lock: drop the LockMap re-export#1647
sergey-shandar merged 8 commits into
mainfrom
dont-rexport

Conversation

@sergey-shandar

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

Copy link
Copy Markdown
Contributor

Summary

  • Remove the LockMap re-export from fjs/media/lock/types.ts; consumers should import it directly from ../revision/types.ts
  • Update fjs/media/lock/proof.f.mjs to import LockMap from its source module

Test plan

  • N/A (type-only change)

Changelog:

  • BREAKING CHANGES: fjs/media/lock/types drops its LockMap re-export
    of fjs/media/revision's type — import it from fjs/media/revision/types.ts
    directly instead

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Aug 19, 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 f9264dc Commit Preview URL

Branch Preview URL
Aug 20 2026, 06:07 PM

@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.

The change is correct on its own terms and every gate is green here: npm test 3032 pass / 0 fail, npm run cov 100.00/100.00/100.00, ci-update and prepack clean, npx tsc exit 0 on a tree with the gitignored .d.mts and .d.ts deleted first (negative controls fired both ways). Nothing else imports LockMap from fjs/media/lock/types.ts, and fjs/cas/evo/module.f.mjs:55 already imports it straight from ../../media/revision/types.ts, so this brings lock/proof in line with existing practice. Three things are missing.

The docstring still describes the re-export. fjs/media/lock/types.ts:4-6 says the map type "is fjs/media/revision's LockMap, re-exported so a consumer of the shared form needs only one import, and so nothing can define a second, drifting spelling of it." That is no longer true of this file, and the comment is copied verbatim into the emitted fjs/media/lock/types.d.ts — so the published package would document an export it does not have.

This is a public-API removal, not an internal refactor. package.json has no exports field and files includes **/*.d.ts; npm pack --dry-run lists fjs/media/lock/types.d.ts in the tarball, and the published functionalscript@0.46.1 carries export type { LockMap } from '../revision/types.ts'; at exactly that path. Anyone importing LockMap from functionalscript/fjs/media/lock/types.ts breaks. Per AGENTS.md that needs changelog/unreleased/1647.md with a **BREAKING CHANGES:** prefix, and the body needs the matching Changelog: section holding the same list items. Right now there is no entry file and no Changelog: line at all — and the section is mandatory either way, so even on the internal-refactor reading Changelog: none would still be owed.

The title needs a topic. don't reexport has no <topic>: prefix; something like media/lock: don't re-export LockMap fits. Across the squash merges since the commit-message standard landed in #1561, 38 of 38 non-release titles carry a topic prefix, the only exceptions being the release PRs whose bare version the convention explicitly allows.

One observation, not a request: fjs/types/rtti/parse/types.ts:10 still re-exports Path and ValidationError from ../common/types.ts, the same shape this PR is removing.

@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.

Re-checked at the merged head. The merge broke nothing — tsc --noEmit clean, npm test 3032/3032, npm run cov exit 0 at 100.00% across the board, ci-update no diff. And the removal breaks no in-repo caller: grep -rn LockMap finds no remaining import from fjs/media/lock/types.ts.

The three findings from my earlier review still stand — the head moved only by merging main, so none of the two source files changed.

  1. Breaking public-API change, still no changelog. I checked the actual tarball rather than reasoning from package.json: npm pack functionalscript@0.46.1 ships package/fjs/media/lock/types.d.ts, and line 11 of it is export type { LockMap } from '../revision/types.ts'. So the export is live public API and removing it breaks external consumers. AGENTS.md:88-92 needs changelog/unreleased/1647.md with a **BREAKING CHANGES:** prefix — the directory doesn't exist on this head and the body has no Changelog: line. Precedent for this exact type: changelog/0.44.0.md:346.

    (To be clear, the substance is right — changelog/README.md:82-84 explicitly says not to preserve a stale re-export just to avoid churn. Only the entry is missing.)

  2. fjs/media/lock/types.ts:4-6 still says LockMap is "re-exported so a consumer of the shared form needs only one import", describing the export this PR removes. TypeScript copies that docstring into the emitted types.d.ts, so the contradiction ships to consumers — verified in the extracted 0.46.1 tarball.

  3. Title don't reexport has no <topic>: prefix (CONTRIBUTING.md:178, AGENTS.md:86); it becomes the squash commit. All 39 merged non-release PRs since #1606 carry one — e.g. media/lock: drop the LockMap re-export.

@sergey-shandar sergey-shandar changed the title don't reexport media/lock: drop the LockMap re-export Aug 20, 2026
Addresses PR review: the LockMap-re-export removal needs a
changelog/unreleased entry with the BREAKING CHANGES prefix, and the
types.ts module docstring still described the re-export it removed.

@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.

All three resolved. Verified at f9264dc8: tsc clean, 3068/3068, cov exit 0 at 100.00, ci-update no diff, and the negative control fires (pointing proof.f.mjs's @import back at ./types.ts gives TS2305: Module '"./types.ts"' has no exported member 'LockMap').

The docstring fix I checked where it matters — in the emitted declarations, since that's what ships. npx tsc --noEmit false --emitDeclarationOnly produces a fjs/media/lock/types.d.ts carrying the corrected text verbatim and declaring only Lock and LockError; LockMap is genuinely absent. The new prose is also true of the file as it stands.

The changelog entry uses the literal **BREAKING CHANGES:** marker matching released precedent (0.43.0.md:1, 0.40.0.md:1, and the 0.44.0.md:346 entry that moved this very type), and the body's Changelog: section is byte-identical to the file. Title is media/lock: drop the LockMap re-export — 46 chars with (#1647). No in-repo consumer imports LockMap from the old path; everything resolves to fjs/media/revision/types.ts:31.

Minor, not blocking: 8c4cf91f adds a blank JSDoc separator line at fjs/types/rtti/module.f.mjs:110, which is outside this PR's scope and makes that one @type block differ from the other four in the file.

Approving.

@sergey-shandar
sergey-shandar added this pull request to the merge queue Aug 20, 2026
Merged via the queue into main with commit 00892b1 Aug 20, 2026
19 checks passed
@sergey-shandar
sergey-shandar deleted the dont-rexport branch August 20, 2026 20:15
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