feat(commonjs): add debug mode

packages/commonjs/README.md

@@ -115,15 +115,21 @@ Instructs the plugin whether to enable mixed module transformations. This is use
 
 ### `strictRequireSemantic`
 
-Type: `boolean | string | string[]`<br>
-Default: `false`
+Type: `"auto" | boolean | "debug" | string[]`<br>
+Default: `"auto"`
+
+By default, this plugin will try to hoist `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 as the order of side effects like log statements may change. But it is especially problematic when there are circular `require` calls between CommonJS modules as those often rely on the lazy execution of nested `require` calls.
 
-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 an impact on the size and performance of the generated code.
 
-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.
+The default value of `"auto"` will only wrap CommonJS files when they are part of a CommonJS dependency cycle, e.g. an index file that is required by many of its dependencies. All other CommonJS files are hoisted. This is the recommended setting for most code bases.
+
+`false` will entirely prevent wrapping and hoist all files. This may still work depending on the nature of cyclic dependencies but will often cause problems.
 
 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.
 
+`"debug"` works like `"auto"` but after bundling, it will display a warning containing a list of ids that have been wrapped which can be used as minimatch pattern for fine-tuning.
+
 ### `ignore`
 
 Type: `string[] | ((id: string) => boolean)`<br>

packages/commonjs/src/generate-imports.js

@@ -112,7 +112,7 @@ export function getRequireHandlers() {
     id,
     exportMode,
     resolveRequireSourcesAndGetMeta,
-    usesRequireWrapper,
+    needsRequireWrapper,
     isEsModule,
     usesRequire
   ) {
@@ -135,13 +135,16 @@ export function getRequireHandlers() {
       );
     }
     const requiresBySource = collectSources(requireExpressions);
-    const requireTargets = await resolveRequireSourcesAndGetMeta(
+    const { requireTargets, usesRequireWrapper } = await resolveRequireSourcesAndGetMeta(
       id,
-      usesRequireWrapper ? IS_WRAPPED_COMMONJS : !isEsModule,
+      needsRequireWrapper ? IS_WRAPPED_COMMONJS : !isEsModule,
       Object.keys(requiresBySource)
     );
     processRequireExpressions(imports, requireTargets, requiresBySource, magicString);
-    return imports.length ? `${imports.join('\n')}\n\n` : '';
+    return {
+      importBlock: imports.length ? `${imports.join('\n')}\n\n` : '',
+      usesRequireWrapper
+    };
   }
 
   return {

packages/commonjs/src/index.js

@@ -1,4 +1,4 @@
-import { extname } from 'path';
+import { extname, relative } from 'path';
 
 import { createFilter } from '@rollup/pluginutils';
 import getCommonDir from 'commondir';
@@ -27,7 +27,7 @@ import getResolveId from './resolve-id';
 import { getResolveRequireSourcesAndGetMeta } from './resolve-require-sources';
 import validateRollupVersion from './rollup-version';
 import transformCommonjs from './transform-commonjs';
-import { getName, normalizePathSlashes } from './utils';
+import { getName, getStrictRequireSemanticFilter, normalizePathSlashes } from './utils';
 
 export default function commonjs(options = {}) {
   const {
@@ -38,12 +38,7 @@ export default function commonjs(options = {}) {
   } = 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 { strictRequireSemanticFilter, detectCycles } = getStrictRequireSemanticFilter(options);
 
   const getRequireReturnsDefault =
     typeof requireReturnsDefaultOption === 'function'
@@ -61,7 +56,10 @@ export default function commonjs(options = {}) {
   const defaultIsModuleExports =
     typeof options.defaultIsModuleExports === 'boolean' ? options.defaultIsModuleExports : 'auto';
 
-  const resolveRequireSourcesAndGetMeta = getResolveRequireSourcesAndGetMeta(extensions);
+  const { resolveRequireSourcesAndGetMeta, getWrappedIds } = getResolveRequireSourcesAndGetMeta(
+    extensions,
+    detectCycles
+  );
   const dynamicRequireModuleSet = getDynamicRequireModuleSet(options.dynamicRequireTargets);
   const isDynamicRequireModulesEnabled = dynamicRequireModuleSet.size > 0;
   const commonDir = isDynamicRequireModulesEnabled
@@ -118,7 +116,7 @@ export default function commonjs(options = {}) {
       return { meta: { commonjs: { isCommonJS: false } } };
     }
 
-    const usesRequireWrapper =
+    const needsRequireWrapper =
       !isEsModule &&
       (dynamicRequireModuleSet.has(normalizePathSlashes(id)) || strictRequireSemanticFilter(id));
 
@@ -137,7 +135,7 @@ export default function commonjs(options = {}) {
       commonDir,
       ast,
       defaultIsModuleExports,
-      usesRequireWrapper,
+      needsRequireWrapper,
       resolveRequireSourcesAndGetMeta(this)
     );
   }
@@ -154,6 +152,28 @@ export default function commonjs(options = {}) {
       }
     },
 
+    buildEnd() {
+      if (options.strictRequireSemantic === 'debug') {
+        const wrappedIds = getWrappedIds();
+        if (wrappedIds.length) {
+          this.warn({
+            code: 'WRAPPED_IDS',
+            ids: wrappedIds,
+            message: `The commonjs plugin automatically wrapped the following files:\n[\n${wrappedIds
+              .map((id) => `\t${JSON.stringify(relative(process.cwd(), id))}`)
+              .join(',\n')}\n]`
+          });
+        } else {
+          // TODO Lukas test
+          this.warn({
+            code: 'WRAPPED_IDS',
+            ids: wrappedIds,
+            message: 'The commonjs plugin did not wrap any files.'
+          });
+        }
+      }
+    },
+
     resolveId,
 
     load(id) {

packages/commonjs/src/resolve-require-sources.js

@@ -1,54 +1,80 @@
 import { EXTERNAL_SUFFIX, IS_WRAPPED_COMMONJS, PROXY_SUFFIX, wrapId } from './helpers';
 import { resolveExtensions } from './resolve-id';
 
-// TODO Lukas auto-detect circular dependencies
-// * only return once all dependencies have been analyzed
-// * wait for this.load dependencies unless they already have an entry in knownCjsModuleTypes to avoid deadlocks
-//   as those have already started being processed
-// * only analyze cycles if we do not have an explicit config
-export function getResolveRequireSourcesAndGetMeta(extensions) {
+export function getResolveRequireSourcesAndGetMeta(extensions, detectCycles) {
   const knownCjsModuleTypes = Object.create(null);
-  return (rollupContext) => (id, isParentCommonJS, sources) => {
-    knownCjsModuleTypes[id] = isParentCommonJS;
-    return Promise.all(
-      sources.map(async (source) => {
-        // Never analyze or proxy internal modules
-        if (source.startsWith('\0')) {
-          return { source, id: source, isCommonJS: false };
-        }
-        const resolved =
-          (await rollupContext.resolve(source, id, {
-            skipSelf: true,
-            custom: {
-              'node-resolve': { isRequire: true }
+  const dependentModules = Object.create(null);
+  const getDependentModules = (id) =>
+    dependentModules[id] || (dependentModules[id] = Object.create(null));
+
+  return {
+    getWrappedIds: () =>
+      Object.keys(knownCjsModuleTypes).filter(
+        (id) => knownCjsModuleTypes[id] === IS_WRAPPED_COMMONJS
+      ),
+    resolveRequireSourcesAndGetMeta: (rollupContext) => async (id, isParentCommonJS, sources) => {
+      knownCjsModuleTypes[id] = isParentCommonJS;
+      const requireTargets = await Promise.all(
+        sources.map(async (source) => {
+          // Never analyze or proxy internal modules
+          if (source.startsWith('\0')) {
+            return { id: source, allowProxy: false };
+          }
+          const resolved =
+            (await rollupContext.resolve(source, id, {
+              skipSelf: true,
+              custom: {
+                'node-resolve': { isRequire: true }
+              }
+            })) || resolveExtensions(source, id, extensions);
+          if (!resolved) {
+            return { id: wrapId(source, EXTERNAL_SUFFIX), allowProxy: false };
+          }
+          const childId = resolved.id;
+          if (resolved.external) {
+            return { id: wrapId(childId, EXTERNAL_SUFFIX), allowProxy: false };
+          }
+          const parentDependentModules = getDependentModules(id);
+          const childDependentModules = getDependentModules(childId);
+          childDependentModules[id] = true;
+          for (const dependentId of Object.keys(parentDependentModules)) {
+            childDependentModules[dependentId] = true;
+          }
+          if (parentDependentModules[childId]) {
+            // If we depend on one of our dependencies, we have a cycle. Then all modules that
+            // we depend on that also depend on the same module are part of a cycle as well.
+            if (detectCycles && isParentCommonJS) {
+              knownCjsModuleTypes[id] = IS_WRAPPED_COMMONJS;
+              knownCjsModuleTypes[childId] = IS_WRAPPED_COMMONJS;
+              for (const dependentId of Object.keys(parentDependentModules)) {
+                if (getDependentModules(dependentId)[childId]) {
+                  knownCjsModuleTypes[dependentId] = IS_WRAPPED_COMMONJS;
+                }
+              }
             }
-          })) || resolveExtensions(source, id, extensions);
-        if (!resolved) {
-          return { source, id: wrapId(source, EXTERNAL_SUFFIX), isCommonJS: false };
-        }
-        if (resolved.external) {
-          return { source, id: wrapId(resolved.id, EXTERNAL_SUFFIX), isCommonJS: false };
-        }
-        if (resolved.id in knownCjsModuleTypes) {
+          } else {
+            // This makes sure the current transform handler waits for all direct dependencies to be
+            // loaded and transformed and therefore for all transitive CommonJS dependencies to be
+            // loaded as well so that all cycles have been found and knownCjsModuleTypes is reliable.
+            await rollupContext.load(resolved);
+          }
+          return { id: childId, allowProxy: true };
+        })
+      );
+      return {
+        requireTargets: requireTargets.map(({ id: dependencyId, allowProxy }, index) => {
+          const isCommonJS = knownCjsModuleTypes[dependencyId];
           return {
-            source,
+            source: sources[index],
             id:
-              knownCjsModuleTypes[resolved.id] === IS_WRAPPED_COMMONJS
-                ? resolved.id
-                : wrapId(resolved.id, PROXY_SUFFIX),
-            isCommonJS: knownCjsModuleTypes[resolved.id]
+              allowProxy && isCommonJS !== IS_WRAPPED_COMMONJS
+                ? wrapId(dependencyId, PROXY_SUFFIX)
+                : dependencyId,
+            isCommonJS
           };
-        }
-        const {
-          meta: { commonjs: commonjsMeta }
-        } = await rollupContext.load(resolved);
-        const isCommonJS = commonjsMeta && commonjsMeta.isCommonJS;
-        return {
-          source,
-          id: isCommonJS === IS_WRAPPED_COMMONJS ? resolved.id : wrapId(resolved.id, PROXY_SUFFIX),
-          isCommonJS
-        };
-      })
-    );
+        }),
+        usesRequireWrapper: knownCjsModuleTypes[id] === IS_WRAPPED_COMMONJS
+      };
+    }
   };
 }

packages/commonjs/src/transform-commonjs.js

@@ -50,7 +50,7 @@ export default async function transformCommonjs(
   commonDir,
   astCache,
   defaultIsModuleExports,
-  usesRequireWrapper,
+  needsRequireWrapper,
   resolveRequireSourcesAndGetMeta
 ) {
   const ast = astCache || tryParse(parse, code, id);
@@ -447,7 +447,7 @@ export default async function transformCommonjs(
     ? 'exports'
     : 'module';
 
-  const importBlock = await rewriteRequireExpressionsAndGetImportBlock(
+  const { importBlock, usesRequireWrapper } = await rewriteRequireExpressionsAndGetImportBlock(
     magicString,
     topLevelDeclarations,
     topLevelRequireDeclarators,
@@ -459,7 +459,7 @@ export default async function transformCommonjs(
     id,
     exportMode,
     resolveRequireSourcesAndGetMeta,
-    usesRequireWrapper,
+    needsRequireWrapper,
     isEsModule,
     uses.require
   );

packages/commonjs/src/utils.js

@@ -2,7 +2,7 @@
 
 import { basename, dirname, extname } from 'path';
 
-import { makeLegalIdentifier } from '@rollup/pluginutils';
+import { createFilter, makeLegalIdentifier } from '@rollup/pluginutils';
 
 export function deconflict(scopes, globals, identifier) {
   let i = 1;
@@ -45,3 +45,26 @@ export const getVirtualPathForDynamicRequirePath = (path, commonDir) => {
 export function capitalize(name) {
   return name[0].toUpperCase() + name.slice(1);
 }
+
+export function getStrictRequireSemanticFilter({ strictRequireSemantic }) {
+  switch (strictRequireSemantic) {
+    case true:
+      return { strictRequireSemanticFilter: () => true, detectCycles: false };
+    // eslint-disable-next-line no-undefined
+    case undefined:
+    case 'auto':
+    case 'debug':
+    case null:
+      return { strictRequireSemanticFilter: () => false, detectCycles: true };
+    case false:
+      return { strictRequireSemanticFilter: () => false, detectCycles: false };
+    default:
+      if (typeof strictRequireSemantic === 'string' || Array.isArray(strictRequireSemantic)) {
+        return {
+          strictRequireSemanticFilter: createFilter(strictRequireSemantic),
+          detectCycles: false
+        };
+      }
+      throw new Error('Unexpected value for "strictRequireSemantic" option.');
+  }
+}

packages/commonjs/test/fixtures/function/circular-dependencies-wrapped/_config.js

@@ -9,5 +9,8 @@ module.exports = {
     output: {
       exports: 'named'
     }
+  },
+  pluginOptions: {
+    strictRequireSemantic: false
   }
 };

packages/commonjs/test/fixtures/function/circular-dependencies/_config.js

@@ -9,5 +9,8 @@ module.exports = {
     output: {
       exports: 'named'
     }
+  },
+  pluginOptions: {
+    strictRequireSemantic: false
   }
 };

packages/commonjs/test/fixtures/function/strict-require-semantic-auto/_config.js

@@ -0,0 +1,7 @@
+module.exports = {
+  description:
+    'automatically detects cycles and switches those modules to strict semantics for "auto"',
+  pluginOptions: {
+    strictRequireSemantic: 'auto'
+  }
+};

packages/commonjs/test/fixtures/function/strict-require-semantic-auto/a-imports-b.js

@@ -0,0 +1,2 @@
+exports.a = 'a';
+t.is(require('./b-imports-c').a, 'a');

packages/commonjs/test/fixtures/function/strict-require-semantic-auto/b-imports-c.js

@@ -0,0 +1 @@
+exports.a = require('./c-imports-a.js').a;

packages/commonjs/test/fixtures/function/strict-require-semantic-auto/c-imports-a.js

@@ -0,0 +1 @@
+exports.a = require('./a-imports-b').a;

packages/commonjs/test/fixtures/function/strict-require-semantic-auto/main.js

@@ -0,0 +1 @@
+require('./a-imports-b');

packages/commonjs/test/fixtures/function/strict-require-semantic-cycle-detection/_config.js

@@ -0,0 +1,3 @@
+module.exports = {
+  description: 'automatically detects cycles and switches those modules to strict semantics'
+};

packages/commonjs/test/fixtures/function/strict-require-semantic-cycle-detection/a-imports-b.js

@@ -0,0 +1,2 @@
+exports.a = 'a';
+t.is(require('./b-imports-c').a, 'a');

packages/commonjs/test/fixtures/function/strict-require-semantic-cycle-detection/b-imports-c.js

@@ -0,0 +1 @@
+exports.a = require('./c-imports-a.js').a;

packages/commonjs/test/fixtures/function/strict-require-semantic-cycle-detection/c-imports-a.js

@@ -0,0 +1 @@
+exports.a = require('./a-imports-b').a;

packages/commonjs/test/fixtures/function/strict-require-semantic-cycle-detection/main.js

@@ -0,0 +1 @@
+require('./a-imports-b');