diff --git a/README.md b/README.md index 9b6b11f..359a0af 100644 --- a/README.md +++ b/README.md @@ -133,6 +133,7 @@ Behavior notes (defaults in parentheses) - `requireSource` (`builtin`): whether `require` comes from Node or `createRequire`. - `cjsDefault` (`auto`): bundler-style default interop vs direct `module.exports`. - `out`/`inPlace`: write the transformed code to a file; otherwise the function returns the transformed string only. +- CommonJS → ESM lowering will throw on `with` statements and unshadowed `eval` calls to avoid unsound rewrites. See [docs/esm-to-cjs.md](docs/esm-to-cjs.md) for deeper notes on live bindings, interop helpers, top-level await behavior, and `import.meta.main` handling. For CommonJS to ESM lowering details, read [docs/cjs-to-esm.md](docs/cjs-to-esm.md). @@ -141,3 +142,4 @@ See [docs/esm-to-cjs.md](docs/esm-to-cjs.md) for deeper notes on live bindings, - Remove `@knighted/specifier` and avoid double parsing. - Emit source maps and clearer diagnostics for transform choices. - Broaden fixtures covering live-binding and top-level await edge cases across Node versions. +- Benchmark scope analysis choices: compare `periscopic`, `scope-analyzer`, and `eslint-scope` on fixtures and pick the final adapter. diff --git a/docs/cjs-to-esm.md b/docs/cjs-to-esm.md index 71270d3..5101d77 100644 --- a/docs/cjs-to-esm.md +++ b/docs/cjs-to-esm.md @@ -5,6 +5,7 @@ - Rewrites CommonJS modules to ESM when `target: 'module'` with `transformSyntax` enabled. - Assumes Node 22.21+ runtime with native ESM. - Skips lowering when `module` or `exports` are shadowed at module scope to avoid mis-compilation. +- Throws when encountering `with` statements or unshadowed `eval` to avoid unsound rewrites. - Deprecated CJS features (`require.extensions`, `module.parent`, legacy folder-as-module resolution) are left as-is. ## Imports diff --git a/docs/esm-to-cjs.md b/docs/esm-to-cjs.md index ba9241c..b22ccc2 100644 --- a/docs/esm-to-cjs.md +++ b/docs/esm-to-cjs.md @@ -3,7 +3,7 @@ ## Scope - Translates ESM syntax to CommonJS when `target: 'commonjs'` with `transformSyntax` enabled. -- Assumes Node 20.11+ runtime with `__filename`/`__dirname` shims supplied by the host. +- Assumes Node 22.21+ runtime with `__filename`/`__dirname` shims supplied by the host (matches package engines). - Keeps optional live-binding behavior via `liveBindings` (strict/loose/off). - Top-level await (TLA) handling is controlled by `topLevelAwait: 'error' | 'wrap' | 'preserve'` when lowering to CommonJS. - Optional import.meta.main gating via `importMetaMain: 'shim' | 'warn' | 'error'`. diff --git a/docs/hoisting-tdz.md b/docs/hoisting-tdz.md new file mode 100644 index 0000000..6dc6536 --- /dev/null +++ b/docs/hoisting-tdz.md @@ -0,0 +1,21 @@ +# Hoisting and TDZ in identifier tracking + +## Purpose + +This library tracks module-scope identifiers so CJS↔ESM lowering can avoid collisions and emit correct exports/imports. We model only hoisting behaviors that affect module-scope resolution and skip cases that either error at runtime or are handled by the module loader. + +## What we treat as hoisted + +- `function` declarations at module scope: reads before the declaration are counted. +- `var` declarations at module scope: reads before the declaration are counted (value is `undefined`). + +## What we do not hoist + +- `let` / `const` / `class`: reads in the temporal dead zone are ignored for hoist accounting (they are runtime errors). See fixtures/tests under `test/fixtures/identifiers/hoisting/tdz.js`. +- `import` bindings: import hoisting is handled by the module system; we exclude imports from module-scope hoist tracking. See `test/fixtures/identifiers/hoisting/importHoist.js`. +- Function declarations inside blocks (strict mode): they stay block-scoped and are not treated as module-scope hoists. See `test/fixtures/identifiers/hoisting/functionInBlock.js`. + +## Why this scope + +- Our goal is collision-free lowering, not simulating all JS runtime hoisting/TDZ behavior. +- Excluding TDZ and import hoists keeps the identifier table focused on cases that meaningfully affect CJS↔ESM transforms. diff --git a/oxlint.json b/oxlint.json index db79128..52ec73c 100644 --- a/oxlint.json +++ b/oxlint.json @@ -17,7 +17,10 @@ "no-unused-expressions": "off", "no-dupe-class-members": "off", "no-useless-rename": "off", - "unicorn/no-empty-file": "off" + "unicorn/no-empty-file": "off", + "no-with": "off", + "no-console": "off", + "no-eval": "off" } }, { diff --git a/package-lock.json b/package-lock.json index acb703e..098553e 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@knighted/module", - "version": "1.0.0-beta.4", + "version": "1.0.0-beta.5", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@knighted/module", - "version": "1.0.0-beta.4", + "version": "1.0.0-beta.5", "license": "MIT", "dependencies": { "@knighted/specifier": "^2.0.9", diff --git a/package.json b/package.json index d333d60..cad359b 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@knighted/module", - "version": "1.0.0-beta.4", + "version": "1.0.0-beta.5", "description": "Transforms differences between ES modules and CommonJS.", "type": "module", "main": "dist/module.js", diff --git a/src/format.ts b/src/format.ts index e866fce..65245ad 100644 --- a/src/format.ts +++ b/src/format.ts @@ -515,6 +515,20 @@ const format = async (src: string, ast: ParseResult, opts: FormatterOptions) => } } + if (shouldRaiseEsm && node.type === 'WithStatement') { + throw new Error('Cannot transform to ESM: with statements are not supported.') + } + + if ( + shouldRaiseEsm && + node.type === 'CallExpression' && + node.callee.type === 'Identifier' && + node.callee.name === 'eval' && + !shadowedBindings.has('eval') + ) { + throw new Error('Cannot transform to ESM: eval is not supported.') + } + if ( shouldRaiseEsm && node.type === 'CallExpression' && diff --git a/src/utils.ts b/src/utils.ts index 9c7b1ab..679daf1 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -10,13 +10,13 @@ import { scopeNodes } from './utils/scopeNodes.js' type UpdateSrcLang = Parameters[1] const getLangFromExt = (filename: string): UpdateSrcLang => { - const ext = extname(filename) + const ext = extname(filename).toLowerCase() - if (ext.endsWith('.js')) { + if (ext === '.js' || ext === '.mjs' || ext === '.cjs') { return 'js' } - if (ext.endsWith('.ts')) { + if (ext === '.ts' || ext === '.mts' || ext === '.cts') { return 'ts' } @@ -276,6 +276,14 @@ const collectModuleIdentifiers = async (ast: Node, hoisting: boolean = true) => const isVarDeclarationInGlobalScope = identifier.isVarDeclarationInGlobalScope(ancestors) + const parent = ancestors[ancestors.length - 2] + const grandParent = ancestors[ancestors.length - 3] + const hoistSafe = + parent.type === 'FunctionDeclaration' || + (parent.type === 'VariableDeclarator' && + grandParent?.type === 'VariableDeclaration' && + grandParent.kind === 'var') + if ( isModuleScope || isClassOrFuncDeclaration || @@ -284,7 +292,7 @@ const collectModuleIdentifiers = async (ast: Node, hoisting: boolean = true) => meta.declare.push(node) // Check for hoisted reads - if (hoisting && globalReads.has(name)) { + if (hoisting && hoistSafe && globalReads.has(name)) { const reads = globalReads.get(name) if (reads) { diff --git a/src/utils/identifiers.ts b/src/utils/identifiers.ts index 4b344dc..f4cbc93 100644 --- a/src/utils/identifiers.ts +++ b/src/utils/identifiers.ts @@ -155,6 +155,14 @@ const collectModuleIdentifiers = async (ast: Node, hoisting: boolean = true) => const isVarDeclarationInGlobalScope = identifier.isVarDeclarationInGlobalScope(ancestors) + const parent = ancestors[ancestors.length - 2] + const grandParent = ancestors[ancestors.length - 3] + const hoistSafe = + parent.type === 'FunctionDeclaration' || + (parent.type === 'VariableDeclarator' && + grandParent?.type === 'VariableDeclaration' && + grandParent.kind === 'var') + if ( isModuleScope || isClassOrFuncDeclaration || @@ -163,7 +171,7 @@ const collectModuleIdentifiers = async (ast: Node, hoisting: boolean = true) => meta.declare.push(node) // Check for hoisted reads - if (hoisting && globalReads.has(name)) { + if (hoisting && hoistSafe && globalReads.has(name)) { const reads = globalReads.get(name) if (reads) { diff --git a/src/utils/lang.ts b/src/utils/lang.ts index 577c59e..6803a48 100644 --- a/src/utils/lang.ts +++ b/src/utils/lang.ts @@ -4,13 +4,13 @@ import type { Specifier } from '@knighted/specifier' // Determine language from filename extension for specifier rewrite. type UpdateSrcLang = Parameters[1] const getLangFromExt = (filename: string): UpdateSrcLang => { - const ext = extname(filename) + const ext = extname(filename).toLowerCase() - if (ext.endsWith('.js')) { + if (ext === '.js' || ext === '.mjs' || ext === '.cjs') { return 'js' } - if (ext.endsWith('.ts')) { + if (ext === '.ts' || ext === '.mts' || ext === '.cts') { return 'ts' } diff --git a/test/fixtures/computedReexport.cjs b/test/fixtures/computedReexport.cjs new file mode 100644 index 0000000..f7ca439 --- /dev/null +++ b/test/fixtures/computedReexport.cjs @@ -0,0 +1,8 @@ +exports['foo-bar'] = 'fb' +exports['zap'] = 2 + +module.exports = { + bag: exports, + fooBar: exports['foo-bar'], + zap: exports['zap'], +} diff --git a/test/fixtures/identifiers/hoisting/functionInBlock.js b/test/fixtures/identifiers/hoisting/functionInBlock.js new file mode 100644 index 0000000..ba906ea --- /dev/null +++ b/test/fixtures/identifiers/hoisting/functionInBlock.js @@ -0,0 +1,11 @@ +// In strict mode, function declarations inside blocks are block-scoped and not hoisted +// to the outer scope. Ensure we don't count reads before the declaration as hoisted. + +{ + func() + function func() { + return 'block-func' + } +} + +export {} diff --git a/test/fixtures/identifiers/hoisting/importHoist.js b/test/fixtures/identifiers/hoisting/importHoist.js new file mode 100644 index 0000000..5087a4c --- /dev/null +++ b/test/fixtures/identifiers/hoisting/importHoist.js @@ -0,0 +1,9 @@ +// Imports are hoisted by the module system, but we do not treat them as part of +// the module-scope identifier hoisting for collision/reads accounting. + +import { x } from './importHoistDep.js' + +const a = x +const b = (() => x)() + +export { a, b } diff --git a/test/fixtures/identifiers/hoisting/importHoistDep.js b/test/fixtures/identifiers/hoisting/importHoistDep.js new file mode 100644 index 0000000..80bc525 --- /dev/null +++ b/test/fixtures/identifiers/hoisting/importHoistDep.js @@ -0,0 +1 @@ +export const x = 'import-hoist' diff --git a/test/fixtures/identifiers/hoisting/tdz.js b/test/fixtures/identifiers/hoisting/tdz.js new file mode 100644 index 0000000..f1b8bcb --- /dev/null +++ b/test/fixtures/identifiers/hoisting/tdz.js @@ -0,0 +1,46 @@ +// These TDZ reads would throw at runtime; we should not count them as safe hoists. +// Access before declaration (let/const/class) remains in the TDZ. + +// let +const a = (() => { + try { + return foo + } catch { + return 'tdz' + } +})() +let foo = 'foo' + +// const +const b = (() => { + try { + return bar + } catch { + return 'tdz' + } +})() +const bar = 'bar' + +// class +const c = (() => { + try { + return Baz + } catch { + return 'tdz' + } +})() +class Baz {} + +// nested block TDZ +{ + const d = (() => { + try { + return inner + } catch { + return 'tdz' + } + })() + const inner = 'inner' +} + +export { a, b, c } diff --git a/test/fixtures/liveBindingMutation.cjs b/test/fixtures/liveBindingMutation.cjs new file mode 100644 index 0000000..ffebf43 --- /dev/null +++ b/test/fixtures/liveBindingMutation.cjs @@ -0,0 +1,9 @@ +exports.counter = 0 + +function inc() { + exports.counter += 1 +} + +inc() + +exports.inc = inc diff --git a/test/fixtures/nestedShadowExports.cjs b/test/fixtures/nestedShadowExports.cjs new file mode 100644 index 0000000..317da42 --- /dev/null +++ b/test/fixtures/nestedShadowExports.cjs @@ -0,0 +1,8 @@ +exports.foo = 'outer' + +function run() { + const innerExports = { foo: 'inner' } + return innerExports.foo +} + +module.exports = { foo: exports.foo, run } diff --git a/test/fixtures/withEval.cjs b/test/fixtures/withEval.cjs new file mode 100644 index 0000000..a106bcd --- /dev/null +++ b/test/fixtures/withEval.cjs @@ -0,0 +1,7 @@ +const obj = { foo: 'bar' } + +with (obj) { + console.log(foo) +} + +eval('exports.evalled = true') diff --git a/test/module.ts b/test/module.ts index 508256c..2654b20 100644 --- a/test/module.ts +++ b/test/module.ts @@ -251,6 +251,15 @@ describe('@knighted/module', () => { assert.equal((mod as any).default.commonjs, true) }) + it('throws when encountering with/eval while lowering to esm', async () => { + const fixturePath = join(fixtures, 'withEval.cjs') + + await assert.rejects( + () => transform(fixturePath, { target: 'module' }), + /with statements are not supported|eval is not supported/i, + ) + }) + it('keeps nested requires via createRequire when lowering to esm', async t => { const fixturePath = join(fixtures, 'nestedRequire.cjs') const outFile = join(fixtures, 'nestedRequire.mjs') @@ -273,6 +282,26 @@ describe('@knighted/module', () => { assert.equal((mod as any).default.commonjs, true) }) + it('preserves outer exports when inner scopes shadow exports', async t => { + const fixturePath = join(fixtures, 'nestedShadowExports.cjs') + const outFile = join(fixtures, 'nestedShadowExports.mjs') + + t.after(() => { + rm(outFile, { force: true }) + }) + + const result = await transform(fixturePath, { target: 'module' }) + await writeFile(outFile, result) + + const { status } = spawnSync('node', [outFile], { stdio: 'inherit' }) + assert.equal(status, 0) + + const mod = await import(pathToFileURL(outFile).href) + assert.equal((mod as any).default.foo, 'outer') + assert.equal(typeof (mod as any).default.run, 'function') + assert.equal((mod as any).default.run(), 'inner') + }) + it('keeps block-scoped require via createRequire when lowering to esm', async t => { const fixturePath = join(fixtures, 'blockRequire.cjs') const outFile = join(fixtures, 'blockRequire.mjs') @@ -314,6 +343,46 @@ describe('@knighted/module', () => { assert.equal((mod as any).baz, 3) }) + it('preserves runtime mutations on the exports bag default', async t => { + const fixturePath = join(fixtures, 'liveBindingMutation.cjs') + const outFile = join(fixtures, 'liveBindingMutation.mjs') + + t.after(() => { + rm(outFile, { force: true }) + }) + + const result = await transform(fixturePath, { target: 'module' }) + await writeFile(outFile, result) + + const { status } = spawnSync('node', [outFile], { stdio: 'inherit' }) + assert.equal(status, 0) + + const mod = await import(pathToFileURL(outFile).href) + assert.equal((mod as any).counter, 1) + ;(mod as any).inc() + assert.equal(typeof (mod as any).inc, 'function') + }) + + it('exports computed property names when lowering to esm', async t => { + const fixturePath = join(fixtures, 'computedReexport.cjs') + const outFile = join(fixtures, 'computedReexport.mjs') + + t.after(() => { + rm(outFile, { force: true }) + }) + + const result = await transform(fixturePath, { target: 'module' }) + await writeFile(outFile, result) + + const { status } = spawnSync('node', [outFile], { stdio: 'inherit' }) + assert.equal(status, 0) + + const mod = await import(pathToFileURL(outFile).href) + assert.equal((mod as any).default.fooBar, 'fb') + assert.equal((mod as any).default.zap, 2) + assert.equal((mod as any).default.bag['foo-bar'], 'fb') + }) + it('rewrites require.main to import.meta.main', async t => { const fixturePath = join(fixtures, 'requireMain.cjs') const outFile = join(fixtures, 'requireMain.mjs') diff --git a/test/utils.ts b/test/utils.ts index afb369d..6cc7064 100644 --- a/test/utils.ts +++ b/test/utils.ts @@ -6,6 +6,7 @@ import { spawnSync } from 'node:child_process' import { parse } from '#parse' import { collectModuleIdentifiers } from '#utils/identifiers.js' +import { getLangFromExt } from '#utils/lang.js' // Use fixtures to more easily track character offsets and line numbers in test cases. const fixtures = resolve(import.meta.dirname, 'fixtures') @@ -138,4 +139,77 @@ describe('collectModuleIdentifiers', () => { assert.deepEqual(innerBlockVarReads, [427]) assert.deepEqual(catchVarReads, [536]) }) + + it('does not treat TDZ reads (let/const/class) as hoists', async () => { + const fixturePath = join(fixtures, 'identifiers', 'hoisting', 'tdz.js') + const code = await readFile(fixturePath) + const ast = parse('file.ts', code.toString()) + const idents = await collectModuleIdentifiers(ast.program, true) + const { status } = spawnSync('node', [fixturePath], { stdio: 'inherit' }) + + // Test for valid syntax + assert.equal(status, 0) + + // TDZ reads should not be recorded as safe hoists; they should be absent or zero reads. + assert.equal(idents.get('foo')?.read.length ?? 0, 0) + assert.equal(idents.get('bar')?.read.length ?? 0, 0) + assert.equal(idents.get('Baz')?.read.length ?? 0, 0) + assert.equal(idents.get('inner')?.read.length ?? 0, 0) + }) + + it('ignores import hoisting for identifier tracking', async () => { + const fixturePath = join(fixtures, 'identifiers', 'hoisting', 'importHoist.js') + const code = await readFile(fixturePath) + const ast = parse('file.ts', code.toString()) + const idents = await collectModuleIdentifiers(ast.program, true) + const { status } = spawnSync('node', [fixturePath], { stdio: 'inherit' }) + + // Test for valid syntax + assert.equal(status, 0) + + // Imported names should not be counted as module-scope hoists + assert.equal(idents.has('x'), false) + + // Local reads of imported value still exist via binding 'x' inside the module + // but they should not appear in module hoist tracking because x is not declared locally. + }) + + it('does not hoist function declarations inside blocks to module scope', async () => { + const fixturePath = join(fixtures, 'identifiers', 'hoisting', 'functionInBlock.js') + const code = await readFile(fixturePath) + const ast = parse('file.ts', code.toString()) + const idents = await collectModuleIdentifiers(ast.program, true) + const funcReads: number[] = [] + const { status } = spawnSync('node', [fixturePath], { stdio: 'inherit' }) + + // Test for valid syntax + assert.equal(status, 0) + + const funcMeta = idents.get('func') + assert.equal(funcMeta, undefined) + + // Ensure no reads got captured as module-scope hoists + assert.deepEqual(funcReads, []) + }) +}) + +describe('getLangFromExt', () => { + it('returns language for js-like extensions', () => { + assert.equal(getLangFromExt('file.js'), 'js') + assert.equal(getLangFromExt('file.mjs'), 'js') + }) + + it('returns language for ts-like extensions', () => { + assert.equal(getLangFromExt('file.ts'), 'ts') + assert.equal(getLangFromExt('file.d.ts'), 'ts') + }) + + it('returns language for tsx/jsx extensions', () => { + assert.equal(getLangFromExt('file.tsx'), 'tsx') + assert.equal(getLangFromExt('file.jsx'), 'jsx') + }) + + it('returns undefined for unknown extensions', () => { + assert.equal(getLangFromExt('file.txt'), undefined) + }) })