fix(commonjs): attach correct plugin meta-data to commonjs modules (#1038)

lukastaegert · lukastaegert · commit a6f807703668 · 2022-04-24T07:58:20.000+02:00
diff --git a/packages/commonjs/README.md b/packages/commonjs/README.md
@@ -113,6 +113,17 @@ Default: `false`
 
 Instructs the plugin whether to enable mixed module transformations. This is useful in scenarios with modules that contain a mix of ES `import` statements and CommonJS `require` expressions. Set to `true` if `require` calls should be transformed to imports in mixed modules, or `false` if the `require` expressions should survive the transformation. The latter can be important if the code contains environment detection, or you are coding for an environment with special treatment for `require` calls such as [ElectronJS](https://www.electronjs.org/). See also the "ignore" option.
 
+### `strictRequireSemantic`
+
+Type: `boolean | string | string[]`<br>
+Default: `false`
+
+By default, this plugin will try to hoist all `require` statements as imports to the top of each file. While this works well for many code bases and allows for very efficient ESM output, it does not perfectly capture CommonJS semantics. This is especially problematic when there are circular `require` calls between CommonJS modules as those often rely on the lazy execution of nested `require` calls.
+
+Setting this option to `true` will wrap all CommonJS files in functions which are executed when they are required for the first time, preserving NodeJS semantics. Note that this can have a small impact on the size and performance of the generated code.
+
+You can also provide a [minimatch pattern](https://github.com/isaacs/minimatch), or array of patterns, to only specify a subset of files which should be wrapped in functions for proper `require` semantics.
+
 ### `ignore`
 
 Type: `string[] | ((id: string) => boolean)`<br>
diff --git a/packages/commonjs/package.json b/packages/commonjs/package.json
@@ -47,7 +47,7 @@
     "require"
   ],
   "peerDependencies": {
-    "rollup": "^2.38.3"
+    "rollup": "^2.61.1"
   },
   "dependencies": {
     "@rollup/pluginutils": "^3.1.0",
diff --git a/packages/commonjs/src/generate-imports.js b/packages/commonjs/src/generate-imports.js
@@ -2,14 +2,7 @@ import { dirname, resolve } from 'path';
 
 import { sync as nodeResolveSync } from 'resolve';
 
-import {
-  EXPORTS_SUFFIX,
-  HELPERS_ID,
-  MODULE_SUFFIX,
-  PROXY_SUFFIX,
-  REQUIRE_SUFFIX,
-  wrapId
-} from './helpers';
+import { EXPORTS_SUFFIX, HELPERS_ID, MODULE_SUFFIX, PROXY_SUFFIX, wrapId } from './helpers';
 import { normalizePathSlashes } from './utils';
 
 export function isRequireStatement(node, scope) {
@@ -153,12 +146,9 @@ export function getRequireHandlers() {
       );
     }
     for (const source of dynamicRegisterSources) {
-      imports.push(`import ${JSON.stringify(wrapId(source, REQUIRE_SUFFIX))};`);
+      imports.push(`import ${JSON.stringify(source)};`);
     }
     for (const source of requiredSources) {
-      if (!source.startsWith('\0')) {
-        imports.push(`import ${JSON.stringify(wrapId(source, REQUIRE_SUFFIX))};`);
-      }
       const { name, nodesUsingRequired } = requiredBySource[source];
       imports.push(
         `import ${nodesUsingRequired.length ? `${name} from ` : ''}${JSON.stringify(
diff --git a/packages/commonjs/src/helpers.js b/packages/commonjs/src/helpers.js
@@ -3,7 +3,6 @@ export const wrapId = (id, suffix) => `\0${id}${suffix}`;
 export const unwrapId = (wrappedId, suffix) => wrappedId.slice(1, -suffix.length);
 
 export const PROXY_SUFFIX = '?commonjs-proxy';
-export const REQUIRE_SUFFIX = '?commonjs-require';
 export const EXTERNAL_SUFFIX = '?commonjs-external';
 export const EXPORTS_SUFFIX = '?commonjs-exports';
 export const MODULE_SUFFIX = '?commonjs-module';
diff --git a/packages/commonjs/src/index.js b/packages/commonjs/src/index.js
@@ -26,7 +26,6 @@ import {
   PROXY_SUFFIX,
   unwrapId
 } from './helpers';
-import { setCommonJSMetaPromise } from './is-cjs';
 import { hasCjsKeywords } from './parse';
 import {
   getDynamicJsonProxy,
@@ -43,6 +42,12 @@ import { getName, getVirtualPathForDynamicRequirePath, normalizePathSlashes } fr
 export default function commonjs(options = {}) {
   const extensions = options.extensions || ['.js'];
   const filter = createFilter(options.include, options.exclude);
+  const strictRequireSemanticFilter =
+    options.strictRequireSemantic === true
+      ? () => true
+      : !options.strictRequireSemantic
+      ? () => false
+      : createFilter(options.strictRequireSemantic);
   const {
     ignoreGlobal,
     ignoreDynamicRequires,
@@ -77,7 +82,6 @@ export default function commonjs(options = {}) {
 
   const esModulesWithDefaultExport = new Set();
   const esModulesWithNamedExports = new Set();
-  const commonJsMetaPromises = new Map();
 
   const ignoreRequire =
     typeof options.ignore === 'function'
@@ -248,7 +252,7 @@ export default function commonjs(options = {}) {
           getRequireReturnsDefault(actualId),
           esModulesWithDefaultExport,
           esModulesWithNamedExports,
-          commonJsMetaPromises
+          this.load
         );
       }
 
@@ -277,14 +281,6 @@ export default function commonjs(options = {}) {
       } catch (err) {
         return this.error(err, err.loc);
       }
-    },
-
-    moduleParsed({ id, meta: { commonjs: commonjsMeta } }) {
-      if (commonjsMeta && commonjsMeta.isCommonJS != null) {
-        setCommonJSMetaPromise(commonJsMetaPromises, id, commonjsMeta);
-        return;
-      }
-      setCommonJSMetaPromise(commonJsMetaPromises, id, null);
     }
   };
 }
diff --git a/packages/commonjs/src/is-cjs.js b/packages/commonjs/src/is-cjs.js
diff --git a/packages/commonjs/src/proxies.js b/packages/commonjs/src/proxies.js
@@ -1,7 +1,6 @@
 import { readFileSync } from 'fs';
 
 import { DYNAMIC_JSON_PREFIX, HELPERS_ID } from './helpers';
-import { getCommonJSMetaPromise } from './is-cjs';
 import { getName, getVirtualPathForDynamicRequirePath, normalizePathSlashes } from './utils';
 
 // e.g. id === "commonjsHelpers?commonjsRegister"
@@ -16,11 +15,11 @@ export function getUnknownRequireProxy(id, requireReturnsDefault) {
   const name = getName(id);
   const exported =
     requireReturnsDefault === 'auto'
-      ? `import {getDefaultExportFromNamespaceIfNotNamed} from "${HELPERS_ID}"; export default /*@__PURE__*/getDefaultExportFromNamespaceIfNotNamed(${name});`
+      ? `import { getDefaultExportFromNamespaceIfNotNamed } from "${HELPERS_ID}"; export default /*@__PURE__*/getDefaultExportFromNamespaceIfNotNamed(${name});`
       : requireReturnsDefault === 'preferred'
-      ? `import {getDefaultExportFromNamespaceIfPresent} from "${HELPERS_ID}"; export default /*@__PURE__*/getDefaultExportFromNamespaceIfPresent(${name});`
+      ? `import { getDefaultExportFromNamespaceIfPresent } from "${HELPERS_ID}"; export default /*@__PURE__*/getDefaultExportFromNamespaceIfPresent(${name});`
       : !requireReturnsDefault
-      ? `import {getAugmentedNamespace} from "${HELPERS_ID}"; export default /*@__PURE__*/getAugmentedNamespace(${name});`
+      ? `import { getAugmentedNamespace } from "${HELPERS_ID}"; export default /*@__PURE__*/getAugmentedNamespace(${name});`
       : `export default ${name};`;
   return `import * as ${name} from ${JSON.stringify(id)}; ${exported}`;
 }
@@ -47,13 +46,15 @@ export async function getStaticRequireProxy(
   requireReturnsDefault,
   esModulesWithDefaultExport,
   esModulesWithNamedExports,
-  commonJsMetaPromises
+  loadModule
 ) {
   const name = getName(id);
-  const commonjsMeta = await getCommonJSMetaPromise(commonJsMetaPromises, id);
+  const {
+    meta: { commonjs: commonjsMeta }
+  } = await loadModule({ id });
   if (commonjsMeta && commonjsMeta.isCommonJS) {
     return `export { __moduleExports as default } from ${JSON.stringify(id)};`;
-  } else if (commonjsMeta === null) {
+  } else if (!commonjsMeta) {
     return getUnknownRequireProxy(id, requireReturnsDefault);
   } else if (!requireReturnsDefault) {
     return `import { getAugmentedNamespace } from "${HELPERS_ID}"; import * as ${name} from ${JSON.stringify(
diff --git a/packages/commonjs/src/resolve-id.js b/packages/commonjs/src/resolve-id.js
@@ -13,7 +13,6 @@ import {
   isWrappedId,
   MODULE_SUFFIX,
   PROXY_SUFFIX,
-  REQUIRE_SUFFIX,
   unwrapId,
   wrapId
 } from './helpers';
@@ -66,14 +65,11 @@ export default function getResolveId(extensions) {
     }
 
     const isProxyModule = isWrappedId(importee, PROXY_SUFFIX);
-    const isRequiredModule = isWrappedId(importee, REQUIRE_SUFFIX);
     let isModuleRegistration = false;
 
     if (isProxyModule) {
       importee = unwrapId(importee, PROXY_SUFFIX);
-    } else if (isRequiredModule) {
-      importee = unwrapId(importee, REQUIRE_SUFFIX);
-
+    } else {
       isModuleRegistration = isWrappedId(importee, DYNAMIC_REGISTER_SUFFIX);
       if (isModuleRegistration) {
         importee = unwrapId(importee, DYNAMIC_REGISTER_SUFFIX);
@@ -98,20 +94,29 @@ export default function getResolveId(extensions) {
       Object.assign({}, resolveOptions, {
         skipSelf: true,
         custom: Object.assign({}, resolveOptions.custom, {
-          'node-resolve': { isRequire: isProxyModule || isRequiredModule }
+          'node-resolve': { isRequire: isProxyModule || isModuleRegistration }
         })
       })
     ).then((resolved) => {
       if (!resolved) {
         resolved = resolveExtensions(importee, importer);
       }
-      if (resolved && isProxyModule) {
-        resolved.id = wrapId(resolved.id, resolved.external ? EXTERNAL_SUFFIX : PROXY_SUFFIX);
-        resolved.external = false;
-      } else if (resolved && isModuleRegistration) {
-        resolved.id = wrapId(resolved.id, DYNAMIC_REGISTER_SUFFIX);
-      } else if (!resolved && (isProxyModule || isRequiredModule)) {
-        return { id: wrapId(importee, EXTERNAL_SUFFIX), external: false };
+      if (isProxyModule) {
+        if (!resolved || resolved.external) {
+          return {
+            id: wrapId(resolved ? resolved.id : importee, EXTERNAL_SUFFIX),
+            external: false
+          };
+        }
+        // This will make sure meta properties in "resolved" are correctly attached to the module
+        this.load(resolved);
+        return {
+          id: wrapId(resolved.id, PROXY_SUFFIX),
+          external: false
+        };
+      }
+      if (resolved && isModuleRegistration) {
+        return { id: wrapId(resolved.id, DYNAMIC_REGISTER_SUFFIX), external: false };
       }
       return resolved;
     });
diff --git a/packages/commonjs/src/transform-commonjs.js b/packages/commonjs/src/transform-commonjs.js
@@ -82,11 +82,8 @@ export default function transformCommonjs(
   const dynamicRegisterSources = new Set();
   let hasRemovedRequire = false;
 
-  const {
-    addRequireStatement,
-    requiredSources,
-    rewriteRequireExpressionsAndGetImportBlock
-  } = getRequireHandlers();
+  const { addRequireStatement, requiredSources, rewriteRequireExpressionsAndGetImportBlock } =
+    getRequireHandlers();
 
   // See which names are assigned to. This is necessary to prevent
   // illegally replacing `var foo = require('foo')` with `import foo from 'foo'`,
@@ -235,10 +232,8 @@ export default function transformCommonjs(
             let shouldRemoveRequireStatement = false;
 
             if (currentTryBlockEnd !== null) {
-              ({
-                canConvertRequire,
-                shouldRemoveRequireStatement
-              } = getIgnoreTryCatchRequireStatementMode(node.arguments[0].value));
+              ({ canConvertRequire, shouldRemoveRequireStatement } =
+                getIgnoreTryCatchRequireStatementMode(node.arguments[0].value));
 
               if (shouldRemoveRequireStatement) {
                 hasRemovedRequire = true;
diff --git a/packages/commonjs/test/fixtures/form/constant-template-literal/output.js b/packages/commonjs/test/fixtures/form/constant-template-literal/output.js
@@ -1,6 +1,5 @@
 import * as commonjsHelpers from "_commonjsHelpers.js";
 import { __exports as input } from "\u0000fixtures/form/constant-template-literal/input.js?commonjs-exports"
-import "\u0000tape?commonjs-require";
 import require$$0 from "\u0000tape?commonjs-proxy";
 
 var foo = require$$0;
diff --git a/packages/commonjs/test/fixtures/form/defaultIsModuleExports-auto-reassign-exports-no-__esModule/output.js b/packages/commonjs/test/fixtures/form/defaultIsModuleExports-auto-reassign-exports-no-__esModule/output.js
@@ -3,4 +3,4 @@ import * as commonjsHelpers from "_commonjsHelpers.js";
 var input = { default: { foo: 'bar' }}
 
 export default input;
-export { input as __moduleExports };
+export { input as __moduleExports };
diff --git a/packages/commonjs/test/fixtures/form/ignore-ids-function/output.js b/packages/commonjs/test/fixtures/form/ignore-ids-function/output.js
@@ -1,6 +1,5 @@
 import * as commonjsHelpers from "_commonjsHelpers.js";
 import { __exports as input } from "\u0000fixtures/form/ignore-ids-function/input.js?commonjs-exports"
-import "\u0000bar?commonjs-require";
 import require$$0 from "\u0000bar?commonjs-proxy";
 
 var foo = require( 'foo' );
diff --git a/packages/commonjs/test/fixtures/form/ignore-ids/output.js b/packages/commonjs/test/fixtures/form/ignore-ids/output.js
@@ -1,6 +1,5 @@
 import * as commonjsHelpers from "_commonjsHelpers.js";
 import { __exports as input } from "\u0000fixtures/form/ignore-ids/input.js?commonjs-exports"
-import "\u0000bar?commonjs-require";
 import require$$0 from "\u0000bar?commonjs-proxy";
 
 var foo = require( 'foo' );
diff --git a/packages/commonjs/test/fixtures/form/multi-entry-module-exports/output1.js b/packages/commonjs/test/fixtures/form/multi-entry-module-exports/output1.js
@@ -1,5 +1,4 @@
 import * as commonjsHelpers from "_commonjsHelpers.js";
-import "\u0000./input2.js?commonjs-require";
 import require$$0 from "\u0000./input2.js?commonjs-proxy";
 
 const t2 = require$$0;
diff --git a/packages/commonjs/test/fixtures/form/multiple-var-declarations-b/output.js b/packages/commonjs/test/fixtures/form/multiple-var-declarations-b/output.js
@@ -1,6 +1,5 @@
 import * as commonjsHelpers from "_commonjsHelpers.js";
 import { __exports as input } from "\u0000fixtures/form/multiple-var-declarations-b/input.js?commonjs-exports"
-import "\u0000./a?commonjs-require";
 import require$$0 from "\u0000./a?commonjs-proxy";
 
 var a = require$$0
diff --git a/packages/commonjs/test/fixtures/form/multiple-var-declarations-c/output.js b/packages/commonjs/test/fixtures/form/multiple-var-declarations-c/output.js
@@ -1,6 +1,5 @@
 import * as commonjsHelpers from "_commonjsHelpers.js";
 import { __exports as input } from "\u0000fixtures/form/multiple-var-declarations-c/input.js?commonjs-exports"
-import "\u0000./b?commonjs-require";
 import require$$0 from "\u0000./b?commonjs-proxy";
 
 var a = 'a'
diff --git a/packages/commonjs/test/fixtures/form/multiple-var-declarations/output.js b/packages/commonjs/test/fixtures/form/multiple-var-declarations/output.js
@@ -1,8 +1,6 @@
 import * as commonjsHelpers from "_commonjsHelpers.js";
 import { __exports as input } from "\u0000fixtures/form/multiple-var-declarations/input.js?commonjs-exports"
-import "\u0000./a?commonjs-require";
 import require$$0 from "\u0000./a?commonjs-proxy";
-import "\u0000./b?commonjs-require";
 import require$$1 from "\u0000./b?commonjs-proxy";
 
 var a = require$$0()
diff --git a/packages/commonjs/test/fixtures/form/no-exports-entry/output.js b/packages/commonjs/test/fixtures/form/no-exports-entry/output.js
@@ -1,6 +1,5 @@
 import * as commonjsHelpers from "_commonjsHelpers.js";
 import { __exports as input_1 } from "\u0000fixtures/form/no-exports-entry/input.js?commonjs-exports"
-import "\u0000./dummy?commonjs-require";
 import require$$0 from "\u0000./dummy?commonjs-proxy";
 
 var dummy = require$$0;
diff --git a/packages/commonjs/test/fixtures/form/require-collision/output.js b/packages/commonjs/test/fixtures/form/require-collision/output.js
@@ -1,6 +1,5 @@
 import * as commonjsHelpers from "_commonjsHelpers.js";
 import { __exports as input } from "\u0000fixtures/form/require-collision/input.js?commonjs-exports"
-import "\u0000foo?commonjs-require";
 import require$$1 from "\u0000foo?commonjs-proxy";
 
 (function() {
diff --git a/packages/commonjs/test/fixtures/form/unambiguous-with-default-export/output.js b/packages/commonjs/test/fixtures/form/unambiguous-with-default-export/output.js
@@ -1,6 +1,5 @@
 import * as commonjsHelpers from "_commonjsHelpers.js";
 import { __exports as input } from "\u0000fixtures/form/unambiguous-with-default-export/input.js?commonjs-exports"
-import "\u0000./foo.js?commonjs-require";
 import "\u0000./foo.js?commonjs-proxy";
 
 export default {};
diff --git a/packages/commonjs/test/fixtures/form/unambiguous-with-import/output.js b/packages/commonjs/test/fixtures/form/unambiguous-with-import/output.js
@@ -1,6 +1,5 @@
 import * as commonjsHelpers from "_commonjsHelpers.js";
 import { __exports as input } from "\u0000fixtures/form/unambiguous-with-import/input.js?commonjs-exports"
-import "\u0000./foo.js?commonjs-require";
 import "\u0000./foo.js?commonjs-proxy";
 
 import './bar.js';
diff --git a/packages/commonjs/test/fixtures/form/unambiguous-with-named-export/output.js b/packages/commonjs/test/fixtures/form/unambiguous-with-named-export/output.js
@@ -1,6 +1,5 @@
 import * as commonjsHelpers from "_commonjsHelpers.js";
 import { __exports as input } from "\u0000fixtures/form/unambiguous-with-named-export/input.js?commonjs-exports"
-import "\u0000./foo.js?commonjs-require";
 import "\u0000./foo.js?commonjs-proxy";
 
 export {};
diff --git a/packages/commonjs/test/fixtures/function/module-meta-properties/_config.js b/packages/commonjs/test/fixtures/function/module-meta-properties/_config.js
@@ -0,0 +1,27 @@
+module.exports = {
+  description: 'preserves meta properties attached to modules by resolvers',
+  options: {
+    plugins: [
+      {
+        async resolveId(source, importer, options) {
+          if (source.endsWith('dep.js')) {
+            return {
+              ...(await this.resolve(source, importer, { skipSelf: true, ...options })),
+              meta: { test: 'provided' }
+            };
+          }
+          return null;
+        },
+        moduleParsed({ id, meta: { test } }) {
+          if (id.endsWith('dep.js')) {
+            if (test !== 'provided') {
+              throw new Error(`Meta property missing for ${id}.`);
+            }
+          } else if (test === 'provided') {
+            throw new Error(`Meta property was unexpectedly added to ${id}.`);
+          }
+        }
+      }
+    ]
+  }
+};
diff --git a/packages/commonjs/test/fixtures/function/module-meta-properties/dep.js b/packages/commonjs/test/fixtures/function/module-meta-properties/dep.js
@@ -0,0 +1 @@
+exports.foo = 'foo';
diff --git a/packages/commonjs/test/fixtures/function/module-meta-properties/main.js b/packages/commonjs/test/fixtures/function/module-meta-properties/main.js
diff --git a/packages/commonjs/test/snapshots/function.js.md b/packages/commonjs/test/snapshots/function.js.md
diff --git a/packages/commonjs/test/snapshots/function.js.snap b/packages/commonjs/test/snapshots/function.js.snap
diff --git a/packages/commonjs/types/index.d.ts b/packages/commonjs/types/index.d.ts
