Skip to content

Allow .mjs files as sources. - #1451

Merged
sergey-shandar merged 6 commits into
mainfrom
first
Aug 7, 2026
Merged

Allow .mjs files as sources.#1451
sergey-shandar merged 6 commits into
mainfrom
first

Conversation

@sergey-shandar

Copy link
Copy Markdown
Contributor

No description provided.

@cloudflare-workers-and-pages

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

Branch Preview URL
Aug 07 2026, 08:53 PM

@sergey-shandar
sergey-shandar marked this pull request as draft August 7, 2026 15:44

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

Checked this out and exercised it locally. The allowJs / checkJs flip and the two-pass prepack both hold up — see the verification notes at the bottom. One thing to fix.

**/*.mjs in files packs a non-package file

Adding **/*.mjs pulls the dev benchmark into the tarball. On this branch:

$ npm pack --dry-run
npm notice 4.8kB fjs/types/bigint/benchmark.mjs
npm notice total files: 513

fjs/types/bigint/benchmark.mjs is the only .mjs in the tree right now, so **/*.mjs currently means "ship the benchmark". That contradicts the rule the repo already states in fjs/ci/todo/publishing-packages.md:79, which #1449 keeps verbatim:

NPM must include both runtime extensions and both declaration extensions. Non-package .mjs files must remain excluded from the packed archive.

A negation entry is enough — I verified it:

"files": [
  "**/*.js",
  "**/*.d.ts",
  "**/*.mjs",
  "**/*.d.mts",
  "!**/benchmark.mjs"
]

→ benchmark gone, total files: 512.

Verified good

  • npx tsc is clean with allowJs / checkJs enabled, so benchmark.mjs already passes validation — that resolves the "make the benchmark pass or delete it" prerequisite in publishing-packages.md.
  • I specifically checked the case enabling allowJs could plausibly break: running npx tsc in a tree that already contains the 254 generated .js files from an earlier prepack. Still clean, so repeated local builds don't regress.
  • The two-pass prepack + npm pack completes.
  • Fixing the pre-existing --NoEmit--noEmit casing along the way is a nice catch.

sergey-shandar commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Decision: keep benchmark.mjs in the NPM package for now. It has no public API, so including it is harmless, and we can remove the file itself later. The temporary !**/benchmark.mjs exclusion has been reverted; the two-pass prepack remains unchanged.

@sergey-shandar
sergey-shandar marked this pull request as ready for review August 7, 2026 20:50

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

Approving at 3d0b03f8.

My packaging concern is resolved, and resolved the better way — you corrected the rule rather than the code. fjs/ci/todo/f-mjs-package-support.md on main now says it explicitly ("Do not add special exclusions merely for non-public authored .mjs files", "Non-public authored .mjs files do not require special package exclusions"), and publishing-packages.md:88 records why packing benchmark.mjs is harmless. That's a real answer with a rationale attached, not a workaround, and it removes the contradiction I was pointing at. No objection.

Re-verified on this head:

  • npx tsc clean.
  • npm test in a clean worktree: pass: 2354, fail: 0, total: 2354.
  • npm pack succeeds; the archive carries benchmark.mjs plus benchmark.d.mts, and that declaration is just export {}; — no references to omitted files, which satisfies the "Verify emitted .d.mts contains no references to omitted package files" checklist item.

One pre-existing footgun, not caused by this PR

Worth a todo/ note rather than a change here. Running npm test in a tree that already holds generated output runs every proof twice:

clean tree:              pass: 2354
after a local prepack:   pass: 4708

I confirmed this reproduces identically on main at 18af44b9, so it predates this PR. The cause is that shouldLoad matches both .f.ts and .f.js, and a prepack emits 111 proof.f.js beside the 111 authored proof.f.ts. Because .gitignore:131 ignores **/*.js, those files are invisible to git status, so the doubling is easy to miss.

I mention it here only because #1449 just dropped the clean:generated requirement on the reasoning that stale outputs can't leak into package jobs from developer machines. That reasoning is correct for package jobs — this is a different consequence of the same stale outputs, in the local test loop. The sharp edge is a stale generated proof from an earlier revision continuing to run against current sources. Given AGENTS.md already devotes a section to fake-pass hazards from wrong invocation, this seems worth recording as its own upstream todo.

@sergey-shandar
sergey-shandar added this pull request to the merge queue Aug 7, 2026
Merged via the queue into main with commit 7631050 Aug 7, 2026
19 checks passed
@sergey-shandar
sergey-shandar deleted the first branch August 7, 2026 21:08
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