fix: include and transform .mjs files from module-sync exports

[iankhou]

Two related fixes for ERR_MODULE_NOT_FOUND when packaging projects that depend on packages using the module-sync export condition (e.g., async-function used by get-intrinsic v1.3.1):

1. Discover alternate export entry points: When resolving a package, also include .mjs files referenced by module-sync and import export conditions. These files may be loaded by Node.js at runtime instead of the default/require entry.

2. Transform .mjs STORE_CONTENT files to CJS: Extend the ESM-to-CJS transformation to also apply to .mjs files stored as STORE_CONTENT (from dependencies), not just STORE_BLOB. This ensures they get transformed and renamed to .js, preventing Node from loading them as ESM in the snapshot.

Add test-54-esm-mjs-imports-js to verify the fix.

Fixes #189, #195

[robertsLando]

Summary

Thanks for digging into this — the diagnosis (Node.js module-sync exports, nodejs/node#54648) is spot on, and reusing transformESMtoCJS / rewriteMjsRequirePaths / wasTransformed is the right plumbing. However, after a deep review I think the fix as written only works for the async-function reproduction and will silently fail on adjacent cases the PR claims to cover. Flagging before merge.

Verdict: Needs work

Top 3 risks

1. Alternate-entry .mjs files skip stepDetect / stepDerivatives → transitive imports of the module-sync target are not walked. The test passes only because require.mjs happens to import ./index.js which is also reachable via the default condition. Any real module-sync target with its own graph will silently drop dependencies.
2. Leaf filter + needsMjsTransform gate are asymmetric. collectAlternateExportFiles filters top-level strings to .endsWith('.mjs') but the module-sync/import condition-match branch adds values unconditionally. Combined with needsMjsTransform being extension-gated on .mjs, a module-sync: "./entry.js" under a "type":"module" parent is included but never transformed → Node loads raw ESM → the very ERR_MODULE_NOT_FOUND this PR is fixing.
3. Test assertion is tautological. assert.strictEqual(left.trim(), right.trim()) passes when both sides fail identically. The fixture also references ./index.mjs that doesn't exist — silently swallowed by the bare catch {}.

Themes

• Scope of the harvest is too narrow. Hardcoding module-sync | import + .mjs-only is a band-aid for this specific reproduction. Node's resolver consumes an arbitrary condition stack (require, node, default, --conditions, community keys like workerd/bun), and packages ship ESM as .js too. The next bug report reopens this hole under a different condition key. The correct design collects every terminal file target in exports and feeds each through the normal STORE_BLOB + stepDetect pipeline.
• Two parallel transform pipelines must stay in sync. needsMjsTransform forks the existing STORE_BLOB transform path. Future edits to ESM→CJS transformation will almost certainly be applied to one branch and forgotten on the other. A single predicate (!seaMode && record.body && isESMFile(record.file)) would unify both.
• Test doesn't actually verify the fix. See the tautology + unreferenced index.mjs above. Replacing the hand-built fixture with a real failing package (e.g., async-function@1.x or a trimmed clone) would pin down the regression.

Strengths

• Correctly diagnoses the root cause (module-sync introduced by nodejs/node#54648).
• Reuses existing transform/rewrite/wasTransformed machinery instead of reinventing it.
• SEA-mode skip on the transform is consistent with the surrounding policy; discovery still running under SEA is the correct asymmetry.

Findings outside line-anchorable hunks (FYI)

• Per-configPath caching. includeAlternateExportEntries runs once per resolving call-site, not once per package. 50 files require('foo') → 50 re-walks of foo's exports + 50 serial fs.stat bursts. A Set<string> keyed on marker.configPath on the Walker would collapse this to one.
• Divergence with lib/resolver.ts. resolver.ts (resolveWithExports) encodes its own condition set and has no awareness of module-sync. A package using module-sync is collected here but won't be resolved by resolver.ts. Worth sharing a single RUNTIME_CONDITIONS constant and updating resolver.ts fallback for consistency.
• Method placement. collectAlternateExportFiles / includeAlternateExportEntries are pure functions of an exports map — they'd sit better in follow.ts or a new lib/exports.ts than accreting on Walker.
• No opt-out. No CLI flag or pkg.options knob to disable this for projects that regress. Low risk today, worth a follow-up.

Specialists run: Correctness, Performance, DRY, Design/API, Tests, Operability, Readability.

[robertsLando]

[Blocker] · Correctness / Design

STORE_CONTENT .mjs files reaching this needsMjsTransform path never have their import/require statements walked — stepDetect / stepDerivatives are gated on store === STORE_BLOB || needsSeaRead at the block below, and needsMjsTransform is not included there.

Why: Any import inside an alternate-entry .mjs that isn't also reachable via another condition will silently drop out of the snapshot → ERR_MODULE_NOT_FOUND at runtime. The current test passes only because require.mjs imports ./index.js, which is also in default.

Fix: Route alternate entries through the same STORE_BLOB + stepDetect pipeline used for primary resolved files, or at minimum extend the gate to include needsMjsTransform so derivatives are walked.

[robertsLando]

[Blocker] · Correctness / Tests

Asymmetric filter: this top-level string branch filters to .endsWith('.mjs'), but the module-sync/import condition-match branch below (line 1017) adds values unconditionally. Meanwhile needsMjsTransform (line 1100) is .mjs-extension-gated.

Why: A module-sync: "./entry.js" under a "type":"module" parent is collected as STORE_CONTENT but never transformed → Node loads raw ESM inside the snapshot → the very ERR_MODULE_NOT_FOUND this PR aims to fix.

Fix: Drive needsMjsTransform off isESMFile(record.file) (which already handles .js + "type":"module"), not the .mjs extension — and make the leaf filter consistent (or drop it entirely and collect every terminal string under exports).

[robertsLando]

[Major] · Correctness

effectiveMarker = newPackageForNewRecords ? newPackageForNewRecords.marker : marker. In stepDerivatives_ALIAS_AS_RESOLVABLE, newPackageForNewRecords is often undefined (only set when double-resolution flips the resolved file). The fallback then resolves ./require.mjs against the caller's package root, not the dep's.

Why: Wrong absolute paths either get silently ENOENT-swallowed by the bare catch (line 988), or — worse — pick up a same-named file from the caller's tree.

Fix: Derive the package from newPackages (prefer the one whose dir is an ancestor of newFile), not from newPackageForNewRecords.

[robertsLando]

[Major] · Design / DRY

Hardcoding 'module-sync' | 'import' is the wrong shape. Node's resolver walks an arbitrary condition stack: require, node, node-addons, default, --conditions, community keys like workerd/deno/bun. The symptom today is module-sync; tomorrow it will be another key and the same class of bug reopens.

lib/resolver.ts:33 (resolveWithExports) already encodes a different condition set — two sources of truth.

Fix: Collect every terminal string-valued target in exports (filtered to paths under pkgDir), regardless of condition key. Share a single RUNTIME_CONDITIONS constant with resolver.ts, update resolver.ts fallback to match.

[robertsLando]

[Major] · Design

This is the load-bearing architectural choice. STORE_CONTENT means raw bytes + no derivative walking. It pairs with the Blocker at line 1100 — the async-function fixture only works because require.mjs re-imports a file already captured via default. A real module-sync target with its own transitive graph will silently drop files and fail at runtime.

Fix: Use STORE_BLOB for alternate entries so stepDetect / stepDerivatives recurse. Drops the needsMjsTransform fork naturally.

[robertsLando]

[Major] · Tests

"import": "./index.mjs" references a file that does not exist in the fixture. Silently swallowed by the bare catch {} in includeAlternateExportEntries (lib/walker.ts:988). The import condition is not actually exercised by the test.

Fix: Either ship index.mjs in the fixture and add a second assertion that confirms it's included, or remove the import reference to match what's being tested.

[robertsLando]

[Minor] · Operability / Correctness

Bare catch {} swallows every fs.stat error (EPERM, EIO, ENOTDIR, ELOOP, …) as if the file simply did not exist. A user with a broken symlink or wrong permissions on an alternate export gets a silent skip at build time and an opaque ERR_MODULE_NOT_FOUND inside the snapshot at runtime — hiding the other Tests finding too.

Fix: Narrow to ENOENT only — if ((err as NodeJS.ErrnoException).code !== 'ENOENT') throw err; — matching the pattern at step_STORE_STAT (lib/walker.ts:1200-1203).

[robertsLando]

[Minor] · DRY / Operability

This rewriteMjsRequirePaths call duplicates the one in the STORE_BLOB tail at line 1244. It also runs outside any try/catch — if it throws (malformed buffer, regex engine), the build crashes with a bare stack trace and no file name.

Fix (combined): Drop this block, widen the outer gate to store === STORE_BLOB || needsSeaRead || needsMjsTransform so the existing rewrite at line 1244 covers both paths, and wrap it with the same descriptive Failed to rewrite .mjs require paths for file "${record.file}": ${message} pattern used around transformESMtoCJS.

[robertsLando]

[Minor] · Performance

for..of with await fs.stat is serial. A package with ~10 subpath exports × 2 alternate conditions = 20 serial RTTs per invocation. Combined with the per-call-site redundancy (no cache keyed on marker.configPath), this scales poorly on monorepos.

Fix: Promise.all(...) the stat-and-append — the Set already deduplicates — and add a per-configPath visited guard on the Walker instance so the scan runs once per package, not once per call-site.

[robertsLando]

[Nit] · Readability

Redundant with the caller's if (effectiveMarker?.configPath) check at line 946. The marker.config half is the only meaningful addition — and if config can actually be undefined here, the caller should check it too.

Also: naming. effectiveMarker reads as "the effective one" without saying effective vs what — resolvedMarker or targetMarker communicates the intent (marker of the newly-resolved dependency) better.