-
-
Notifications
You must be signed in to change notification settings - Fork 61
fix: include and transform .mjs files from module-sync exports #256
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -936,6 +936,97 @@ class Walker { | |
| store: STORE_BLOB, | ||
| reason: record.file, | ||
| }); | ||
|
|
||
| // Also include files from other export conditions (e.g., module-sync, import) | ||
| // that Node.js may resolve to at runtime instead of the default/require entry. | ||
| // Without this, .mjs files referenced by module-sync would be missing from the snapshot. | ||
| const effectiveMarker = newPackageForNewRecords | ||
| ? newPackageForNewRecords.marker | ||
| : marker; | ||
| if (effectiveMarker?.configPath) { | ||
| await this.includeAlternateExportEntries( | ||
| effectiveMarker, | ||
| newFile, | ||
| record.file, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Include alternate export entry points (module-sync, import) from a package's | ||
| * exports field. These files may be loaded by Node.js at runtime instead of the | ||
| * default/require entry, so they must be in the snapshot. | ||
| */ | ||
| private async includeAlternateExportEntries( | ||
| marker: Marker | undefined, | ||
| resolvedFile: string, | ||
| reason: string, | ||
| ) { | ||
| if (!marker?.configPath || !marker.config) return; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Nit] · Readability Redundant with the caller's Also: naming. |
||
|
|
||
| const pkgExports = (marker.config as Record<string, unknown>).exports; | ||
| if (!pkgExports) return; | ||
|
|
||
| const pkgDir = path.dirname(marker.configPath); | ||
| const alternateFiles = this.collectAlternateExportFiles(pkgExports); | ||
|
|
||
| for (const relFile of alternateFiles) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Minor] · Performance
Fix: |
||
| const absFile = normalizePath(path.resolve(pkgDir, relFile)); | ||
| // Skip the file we already resolved | ||
| if (absFile === resolvedFile) continue; | ||
|
|
||
| try { | ||
| const stat = await fs.stat(absFile); | ||
| if (stat.isFile()) { | ||
| await this.appendBlobOrContent({ | ||
| file: absFile, | ||
| marker, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Major] · Design This is the load-bearing architectural choice. Fix: Use |
||
| store: STORE_CONTENT, | ||
| reason, | ||
| }); | ||
| } | ||
| } catch { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Minor] · Operability / Correctness Bare Fix: Narrow to |
||
| // File doesn't exist, skip | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Collect file paths from export conditions that Node.js may use at runtime. | ||
| * Specifically targets module-sync and import conditions. | ||
| */ | ||
| private collectAlternateExportFiles( | ||
| exports: unknown, | ||
| files: Set<string> = new Set(), | ||
| ): Set<string> { | ||
| if (typeof exports === 'string') { | ||
| if (exports.endsWith('.mjs')) files.add(exports); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Blocker] · Correctness / Tests Asymmetric filter: this top-level string branch filters to Why: A Fix: Drive |
||
| return files; | ||
| } | ||
|
|
||
| if (Array.isArray(exports)) { | ||
| for (const item of exports) { | ||
| this.collectAlternateExportFiles(item, files); | ||
| } | ||
| return files; | ||
| } | ||
|
|
||
| if (exports && typeof exports === 'object') { | ||
| for (const [key, value] of Object.entries(exports)) { | ||
| // Include files from conditions that Node.js may use at runtime | ||
| if (key === 'module-sync' || key === 'import') { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Major] · Design / DRY Hardcoding
Fix: Collect every terminal string-valued target in |
||
| if (typeof value === 'string') { | ||
| files.add(value); | ||
| } | ||
| } | ||
| // Recurse into nested conditions and subpath patterns | ||
| if (typeof value === 'object' || typeof value === 'string') { | ||
| this.collectAlternateExportFiles(value, files); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return files; | ||
| } | ||
|
|
||
| async stepDerivatives( | ||
|
|
@@ -1005,10 +1096,18 @@ class Walker { | |
|
|
||
| const needsSeaRead = this.needsSeaRead(record); | ||
|
|
||
| // Also read .mjs STORE_CONTENT files so they can be transformed to CJS | ||
| const needsMjsTransform = | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Blocker] · Correctness / Design STORE_CONTENT Why: Any Fix: Route alternate entries through the same STORE_BLOB + |
||
| store === STORE_CONTENT && | ||
| !this.params.seaMode && | ||
| record.file.endsWith('.mjs') && | ||
| isESMFile(record.file); | ||
|
|
||
| if ( | ||
| store === STORE_BLOB || | ||
| needsSeaRead || | ||
| (store === STORE_CONTENT && isPackageJson(record.file)) || | ||
| needsMjsTransform || | ||
| this.hasPatch(record) | ||
| ) { | ||
| if (!record.body) { | ||
|
|
@@ -1101,8 +1200,10 @@ class Walker { | |
|
|
||
| // Transform ESM to CJS before bytecode compilation | ||
| // Check all JS-like files (.js, .mjs, .cjs) but only transform ESM ones | ||
| // Also transform .mjs files stored as STORE_CONTENT (e.g., from dependencies) | ||
| // to prevent Node.js from loading them as ESM at runtime | ||
| if ( | ||
| store === STORE_BLOB && | ||
| (store === STORE_BLOB || needsMjsTransform) && | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Major] · Design / DRY Two parallel ESM→CJS pipelines now exist: the STORE_BLOB branch (body read → transform → rewrite paths inside Why: Future changes to the transform (sourcemaps, new file types, bug fixes) will be applied to one branch and forgotten on the other. Fix: Unify on a single predicate |
||
| !this.params.seaMode && | ||
| record.body && | ||
| (isDotJS(record.file) || record.file.endsWith('.mjs')) | ||
|
|
@@ -1145,6 +1246,14 @@ class Walker { | |
| ); | ||
| } | ||
| } | ||
|
|
||
| // Also rewrite .mjs require paths for STORE_CONTENT files that were transformed | ||
| if (needsMjsTransform && record.wasTransformed && record.body) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Minor] · DRY / Operability This Fix (combined): Drop this block, widen the outer gate to |
||
| record.body = Buffer.from( | ||
| rewriteMjsRequirePaths(record.body.toString('utf8')), | ||
| 'utf8', | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| record[store] = true; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| #!/usr/bin/env node | ||
|
|
||
| 'use strict'; | ||
|
|
||
| const path = require('path'); | ||
| const assert = require('assert'); | ||
| const utils = require('../utils.js'); | ||
|
|
||
| assert(!module.parent); | ||
| assert(__dirname === process.cwd()); | ||
|
|
||
| const target = process.argv[2] || 'host'; | ||
| const input = './test-x-index.js'; | ||
| const output = './run-time/test-output'; | ||
|
|
||
| console.log('Testing .mjs importing .js (module-sync pattern)...'); | ||
|
|
||
| let left, right; | ||
| utils.mkdirp.sync(path.dirname(output)); | ||
|
|
||
| // Run with node first to get expected output | ||
| left = utils.spawn.sync('node', [input]); | ||
| console.log('Node output:', left.trim()); | ||
|
|
||
| // Package with pkg | ||
| utils.pkg.sync(['--target', target, '--output', output, input], { | ||
| stdio: 'inherit', | ||
| }); | ||
|
|
||
| // Run packaged version | ||
| right = utils.spawn.sync('./' + path.basename(output), [], { | ||
| cwd: path.dirname(output), | ||
| }); | ||
| console.log('Packaged output:', right.trim()); | ||
|
|
||
| assert.strictEqual(left.trim(), right.trim(), 'Outputs should match'); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Major] · Tests
Fix: Assert |
||
|
|
||
| console.log('Test passed: .mjs importing .js works correctly'); | ||
|
|
||
| utils.vacuum.sync(path.dirname(output)); | ||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| 'use strict'; | ||
|
|
||
| const getAsyncFunction = require('esm-module'); | ||
| const AsyncFunction = getAsyncFunction(); | ||
| console.log(typeof AsyncFunction === 'function' ? 'ok' : 'fail'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Major] · Correctness
effectiveMarker = newPackageForNewRecords ? newPackageForNewRecords.marker : marker. InstepDerivatives_ALIAS_AS_RESOLVABLE,newPackageForNewRecordsis often undefined (only set when double-resolution flips the resolved file). The fallback then resolves./require.mjsagainst 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 ofnewFile), not fromnewPackageForNewRecords.