From 983a5ebc33d411036ecb1f4276f1668b1874c22b Mon Sep 17 00:00:00 2001 From: Hiroki Osame Date: Sat, 8 Feb 2025 10:46:55 +0900 Subject: [PATCH] fix: createRequire leaking into CommonJS (#3) fixes #103 --- package.json | 7 +- pnpm-lock.yaml | 25 +++++++ src/utils/get-rollup-configs.ts | 13 ++-- src/utils/rollup-plugins/create-require.ts | 69 ------------------- .../esm-inject-create-require.ts | 60 ++++++++++++++++ tests/specs/builds/minification.ts | 7 +- tests/specs/builds/output-module.ts | 56 +++++++++++++-- tests/utils.ts | 2 +- 8 files changed, 153 insertions(+), 86 deletions(-) delete mode 100644 src/utils/rollup-plugins/create-require.ts create mode 100644 src/utils/rollup-plugins/esm-inject-create-require.ts diff --git a/package.json b/package.json index 58afd9e..5167367 100644 --- a/package.json +++ b/package.json @@ -23,7 +23,8 @@ "files": [ "dist" ], - "bin": "./dist/cli.js", + "type": "module", + "bin": "./dist/cli.mjs", "imports": { "typescript": "./src/local-typescript-loader.ts" }, @@ -57,13 +58,15 @@ "@rollup/pluginutils": "^5.1.4", "esbuild": "^0.24.2", "magic-string": "^0.30.17", - "rollup": "^4.29.1" + "rollup": "^4.29.1", + "rollup-pluginutils": "^2.8.2" }, "devDependencies": { "@types/node": "^22.10.2", "@types/react": "^18.3.5", "clean-pkg-json": "^1.2.0", "cleye": "^1.3.2", + "estree-walker": "^3.0.3", "execa": "9.3.0", "fs-fixture": "^2.6.0", "get-node": "^15.0.1", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 3677ab2..c883d88 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -41,6 +41,9 @@ importers: rollup: specifier: ^4.29.1 version: 4.29.1 + rollup-pluginutils: + specifier: ^2.8.2 + version: 2.8.2 devDependencies: '@types/node': specifier: ^22.10.2 @@ -54,6 +57,9 @@ importers: cleye: specifier: ^1.3.2 version: 1.3.2 + estree-walker: + specifier: ^3.0.3 + version: 3.0.3 execa: specifier: 9.3.0 version: 9.3.0 @@ -1434,9 +1440,15 @@ packages: resolution: {integrity: sha512-MMdARuVEQziNTeJD8DgMqmhwR11BRQ/cBP+pLtYdSTnf3MIO8fFeiINEbX36ZdNlfU/7A9f3gUw49B3oQsvwBA==} engines: {node: '>=4.0'} + estree-walker@0.6.1: + resolution: {integrity: sha512-SqmZANLWS0mnatqbSfRP5g8OXZC12Fgg1IwNtLsyHDzJizORW4khDfjPqJZsemPWBB2uqykUah5YpQ6epsqC/w==} + estree-walker@2.0.2: resolution: {integrity: sha512-Rfkk/Mp/DL7JVje3u18FxFujQlTNR2q6QfMSMB7AvCBx91NGj/ba3kCfza0f6dVDbw7YlRf/nDrn7pQrCCyQ/w==} + estree-walker@3.0.3: + resolution: {integrity: sha512-7RUKfXgSMMkzt6ZuXmqapOurLGPPfgj6l9uRZ7lRGolvk0y2yocc35LdcxKC5PQZdn2DMqioAQ2NoWcrTKmm6g==} + esutils@2.0.3: resolution: {integrity: sha512-kVscqXk4OCp68SZ0dkgEKVi6/8ij300KBWTJq32P/dYeWTSwK41WyTxalN1eRmA5Z9UU/LX9D7FWSmV9SAYx6g==} engines: {node: '>=0.10.0'} @@ -2482,6 +2494,9 @@ packages: rollup: ^3.29.4 || ^4 typescript: ^4.5 || ^5.0 + rollup-pluginutils@2.8.2: + resolution: {integrity: sha512-EEp9NhnUkwY8aif6bxgovPHMoMoNr2FulJziTndpt5H9RdwC47GSGuII9XxpSdzVGM0GWrNPHV6ie1LTNJPaLQ==} + rollup@4.29.1: resolution: {integrity: sha512-RaJ45M/kmJUzSWDs1Nnd5DdV4eerC98idtUOVr6FfKcgxqvjwHmxc5upLF9qZU9EpsVzzhleFahrT3shLuJzIw==} engines: {node: '>=18.0.0', npm: '>=8.0.0'} @@ -4353,8 +4368,14 @@ snapshots: estraverse@5.3.0: {} + estree-walker@0.6.1: {} + estree-walker@2.0.2: {} + estree-walker@3.0.3: + dependencies: + '@types/estree': 1.0.6 + esutils@2.0.3: {} execa@9.3.0: @@ -5523,6 +5544,10 @@ snapshots: optionalDependencies: '@babel/code-frame': 7.24.2 + rollup-pluginutils@2.8.2: + dependencies: + estree-walker: 0.6.1 + rollup@4.29.1: dependencies: '@types/estree': 1.0.6 diff --git a/src/utils/get-rollup-configs.ts b/src/utils/get-rollup-configs.ts index 2ccd037..1ab2282 100644 --- a/src/utils/get-rollup-configs.ts +++ b/src/utils/get-rollup-configs.ts @@ -11,13 +11,13 @@ import dynamicImportVars from '@rollup/plugin-dynamic-import-vars'; import type { PackageJson } from 'type-fest'; import type { TsConfigResult } from 'get-tsconfig'; import type { ExportEntry, AliasMap } from '../types.js'; -import { isFormatEsm, createRequire } from './rollup-plugins/create-require.js'; import { esbuildTransform, esbuildMinify } from './rollup-plugins/esbuild.js'; import { externalizeNodeBuiltins } from './rollup-plugins/externalize-node-builtins.js'; import { patchBinary } from './rollup-plugins/patch-binary.js'; import { resolveTypescriptMjsCts } from './rollup-plugins/resolve-typescript-mjs-cjs.js'; import { resolveTsconfigPaths } from './rollup-plugins/resolve-tsconfig-paths.js'; import { stripHashbang } from './rollup-plugins/strip-hashbang.js'; +import { esmInjectCreateRequire } from './rollup-plugins/esm-inject-create-require.js'; import { getExternalDependencies } from './parse-package-json/get-external-dependencies.js'; type Options = { @@ -134,10 +134,13 @@ const getConfig = { : [] ), stripHashbang(), - commonjs(), json(), esbuildTransform(esbuildConfig), - createRequire(), + commonjs({ + ignoreDynamicRequires: true, + extensions: ['.js', '.ts', '.jsx', '.tsx'], + transformMixedEsModules: true, + }), dynamicImportVars({ warnOnError: true, }), @@ -147,6 +150,7 @@ const getConfig = { : [] ), patchBinary(executablePaths), + esmInjectCreateRequire(), ], output: [] as unknown as Output, external: [] as (string | RegExp)[], @@ -253,9 +257,6 @@ export const getRollupConfigs = async ( format: exportEntry.type, chunkFileNames: `[name]-[hash]${extension}`, sourcemap: flags.sourcemap, - plugins: [ - isFormatEsm(exportEntry.type === 'module'), - ], /** * Preserve source path in dist path diff --git a/src/utils/rollup-plugins/create-require.ts b/src/utils/rollup-plugins/create-require.ts deleted file mode 100644 index bfb45a6..0000000 --- a/src/utils/rollup-plugins/create-require.ts +++ /dev/null @@ -1,69 +0,0 @@ -import type { Plugin } from 'rollup'; -import replace from '@rollup/plugin-replace'; -import inject from '@rollup/plugin-inject'; - -const virtualModuleName = 'pkgroll:create-require'; - -/** - * Since rollup is bundled by rollup, it needs to add a run-time - * suffix so that this doesn't get replaced. - */ -const isEsmVariableName = `IS_ESM${Math.random().toString(36).slice(2)}`; - -/** - * Plugin to seamlessly allow usage of `require` - * across CJS and ESM modules. - * - * This is usually nor a problem for CJS outputs, - * but for ESM outputs, it must be used via - * createRequire. - * - * This plugin automatically injects it for ESM. - */ -export const createRequire = (): Plugin => ({ - ...inject({ - require: virtualModuleName, - }), - - name: 'create-require', - - resolveId: source => ( - (source === virtualModuleName) - ? source - : null - ), - - load: (id) => { - if (id !== virtualModuleName) { - return null; - } - - return ` - import { createRequire } from 'module'; - - export default ( - ${isEsmVariableName} - ? /* @__PURE__ */ createRequire(import.meta.url) - : require - ); - `; - }, -}); - -export const isFormatEsm = ( - isEsm: boolean, -): Plugin => { - const handler = replace({ - [isEsmVariableName]: isEsm, - }).renderChunk!; - - return ({ - name: 'create-require-insert-format', - - // Pick out renderChunk because it's used as an output plugin - renderChunk: { - order: 'pre', - handler: typeof handler === 'function' ? handler : handler.handler, - }, - }); -}; diff --git a/src/utils/rollup-plugins/esm-inject-create-require.ts b/src/utils/rollup-plugins/esm-inject-create-require.ts new file mode 100644 index 0000000..66981e6 --- /dev/null +++ b/src/utils/rollup-plugins/esm-inject-create-require.ts @@ -0,0 +1,60 @@ +import MagicString from 'magic-string'; +import { attachScopes, type AttachedScope } from 'rollup-pluginutils'; +import { walk } from 'estree-walker'; +import type { Plugin } from 'rollup'; + +export const esmInjectCreateRequire = (): Plugin => { + const createRequire = 'import{createRequire as _pkgrollCR}from"node:module";const require=_pkgrollCR(import.meta.url);'; + + return { + name: 'esmInjectCreateRequire', + renderChunk(code, _chunk, options) { + if ( + options.format !== 'es' + || !/\brequire\b/.test(code) + ) { + return null; + } + + const ast = this.parse(code); + let currentScope = attachScopes(ast, 'scope'); + let injectionNeeded = false; + + walk(ast, { + enter(node) { + // Not all nodes have scopes + if (node.scope) { + currentScope = node.scope as AttachedScope; + } + if ( + node.type === 'Identifier' + && node.name === 'require' + // If the current scope (or its parents) does not contain 'require' + && !currentScope.contains('require') + ) { + injectionNeeded = true; + + // No need to continue if one instance is found + this.skip(); + } + }, + leave: (node) => { + if (node.scope) { + currentScope = currentScope.parent!; + } + }, + }); + + if (!injectionNeeded) { + return null; + } + + const magicString = new MagicString(code); + magicString.prepend(createRequire); + return { + code: magicString.toString(), + map: magicString.generateMap({ hires: true }), + }; + }, + }; +}; diff --git a/tests/specs/builds/minification.ts b/tests/specs/builds/minification.ts index d645605..6aa1cf2 100644 --- a/tests/specs/builds/minification.ts +++ b/tests/specs/builds/minification.ts @@ -1,3 +1,4 @@ +import { pathToFileURL } from 'node:url'; import { testSuite, expect } from 'manten'; import { createFixture } from 'fs-fixture'; import { pkgroll } from '../../utils.js'; @@ -30,10 +31,8 @@ export default testSuite(({ describe }, nodePath: string) => { expect(content).not.toMatch('exports.foo=foo'); // Minification should preserve name - expect( - // eslint-disable-next-line @typescript-eslint/no-require-imports - require(fixture.getPath('dist/target.js')).functionName, - ).toBe('preservesName'); + const { functionName } = await import(pathToFileURL(fixture.getPath('dist/target.js')).toString()); + expect(functionName).toBe('preservesName'); }); }); }); diff --git a/tests/specs/builds/output-module.ts b/tests/specs/builds/output-module.ts index 2a45356..348d708 100644 --- a/tests/specs/builds/output-module.ts +++ b/tests/specs/builds/output-module.ts @@ -1,4 +1,5 @@ import fs from 'node:fs/promises'; +import { pathToFileURL } from 'node:url'; import { testSuite, expect } from 'manten'; import { createFixture } from 'fs-fixture'; import { pkgroll } from '../../utils.js'; @@ -181,7 +182,7 @@ export default testSuite(({ describe }, nodePath: string) => { expect(content).toMatch('export { sayHello }'); }); - test('require() works in esm', async () => { + test('require() gets converted to import in esm', async () => { await using fixture = await createFixture({ ...packageFixture(), 'package.json': createPackageJson({ @@ -202,7 +203,8 @@ export default testSuite(({ describe }, nodePath: string) => { expect(js).not.toMatch('createRequire'); const mjs = await fixture.readFile('dist/require.mjs', 'utf8'); - expect(mjs).toMatch('createRequire'); + expect(mjs).not.toMatch('require('); + expect(mjs).toMatch(/import . from"fs"/); }); test('conditional require() no side-effects', async () => { @@ -243,9 +245,55 @@ export default testSuite(({ describe }, nodePath: string) => { const content = await fixture.readFile('dist/conditional-require.mjs', 'utf8'); expect(content).not.toMatch('\tconsole.log(\'side effect\');'); + expect(content).not.toMatch('require('); + expect(content).toMatch('"development"'); + }); + + describe('injects createRequire', ({ test }) => { + test('dynamic require should get a createRequire', async () => { + await using fixture = await createFixture({ + 'src/dynamic-require.ts': 'require((() => \'fs\')());', + 'package.json': createPackageJson({ + main: './dist/dynamic-require.mjs', + }), + }); + + const pkgrollProcess = await pkgroll([], { + cwd: fixture.path, + nodePath, + }); + + expect(pkgrollProcess.exitCode).toBe(0); + expect(pkgrollProcess.stderr).toBe(''); + + const content = await fixture.readFile('dist/dynamic-require.mjs', 'utf8'); + expect(content).toMatch('createRequire'); + expect(content).toMatch('(import.meta.url)'); + + // Shouldn't throw + await import(pathToFileURL(fixture.getPath('dist/dynamic-require.mjs')).toString()); + }); - const [, createRequireMangledVariable] = content.toString().match(/createRequire as (\w+)/)!; - expect(content).not.toMatch(`${createRequireMangledVariable}(`); + test('defined require should not get a createRequire', async () => { + await using fixture = await createFixture({ + 'src/dynamic-require.ts': 'const require = ()=>{}; require((() => \'fs\')());', + 'package.json': createPackageJson({ + main: './dist/dynamic-require.mjs', + }), + }); + + const pkgrollProcess = await pkgroll([], { + cwd: fixture.path, + nodePath, + }); + + expect(pkgrollProcess.exitCode).toBe(0); + expect(pkgrollProcess.stderr).toBe(''); + + const content = await fixture.readFile('dist/dynamic-require.mjs', 'utf8'); + expect(content).not.toMatch('createRequire'); + expect(content).not.toMatch('(import.meta.url)'); + }); }); test('dynamic imports', async () => { diff --git a/tests/utils.ts b/tests/utils.ts index 8d9b7bf..ea20e0a 100644 --- a/tests/utils.ts +++ b/tests/utils.ts @@ -1,7 +1,7 @@ import path from 'node:path'; import { execa, type Options } from 'execa'; -const pkgrollBinPath = path.resolve('./dist/cli.js'); +const pkgrollBinPath = path.resolve('./dist/cli.mjs'); export const pkgroll = async ( cliArguments: string[],