feat(commonjs)!: default strictRequires to true (#1639)

* feat(commonjs)!: default strictRequires to true

* test: update fixtures

* test: update different values

* test: update some tests to use auto

* test: adapt assertion

While the error message has a different variable name, the point of
the test is that we get an error.

* test: correctly pass options in test

Unfortunately, I did not find a way to detect this case from the plugin
and show a warning.

* fix(commonjs): respect defaultIsModuleExports:false for ES imports from wrapped CommonJS

* test: adapt assertions for wrapped CommonJS

* docs: improve readme to explain about CommonJS entry points

* test: allow tests to specify an entry point for running the code

* test: refine late-entry tests

* fix(commonjs): treat moduleSideEffects as __PURE__ comments for wrapped modules

* fix(commonjs): treat moduleSideEffects as __PURE__ comments for proxied wrapped modules

---------

Co-authored-by: Lukas Taegert-Atkinson <[email protected]>

packages/commonjs/README.md

@@ -49,13 +49,13 @@ When used together with the node-resolve plugin
 ### `strictRequires`
 
 Type: `"auto" | boolean | "debug" | string[]`<br>
-Default: `"auto"`
+Default: `true`
 
-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 initialisation order of required modules will be different. The resultant side effects can include log statements being emitted in a different order, and some code that is dependent on the initialisation order of polyfills in require statements may not work. 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.
+Historically, this plugin tried 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 initialisation order of required modules will be different. The resultant side effects can include log statements being emitted in a different order, and some code that is dependent on the initialisation order of polyfills in require statements may not work. 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.
 
-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. This is the safest setting and should be used if the generated code does not work correctly with `"auto"`. Note that `strictRequires: true` can have a small impact on the size and performance of generated code, but less so if the code is minified.
+The default value of `true` will wrap all CommonJS files in functions which are executed when they are required for the first time, preserving NodeJS semantics. This is the safest setting and should be used if the generated code does not work correctly with `"auto"`. Note that `strictRequires: true` can have a small impact on the size and performance of generated code, but less so if the code is minified.
 
-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 some of its dependencies, or if they are only required in a potentially "conditional" way like from within an if-statement or a function. All other CommonJS files are hoisted. This is the recommended setting for most code bases. Note that the detection of conditional requires can be subject to race conditions if there are both conditional and unconditional requires of the same file, which in edge cases may result in inconsistencies between builds. If you think this is a problem for you, you can avoid this by using any value other than `"auto"` or `"debug"`.
+Setting this option to `"auto"` will only wrap CommonJS files when they are part of a CommonJS dependency cycle, e.g. an index file that is required by some of its dependencies, or if they are only required in a potentially "conditional" way like from within an if-statement or a function. All other CommonJS files are hoisted. This is the recommended setting for most code bases. Note that the detection of conditional requires can be subject to race conditions if there are both conditional and unconditional requires of the same file, which in edge cases may result in inconsistencies between builds. If you think this is a problem for you, you can avoid this by using any value other than `"auto"` or `"debug"`.
 
 `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.
 
@@ -386,6 +386,30 @@ For these situations, you can change Rollup's behaviour either globally or per m
 
 To change this for individual modules, you can supply a function for `requireReturnsDefault` instead. This function will then be called once for each required ES module or external dependency with the corresponding id and allows you to return different values for different modules.
 
+## Using CommonJS files as entry points
+
+With this plugin, you can also use CommonJS files as entry points. This means, however, that when you are bundling to an ES module, your bundle will only have a default export. If you want named exports instead, you should use an ES module entry point instead that reexports from your CommonJS entry point, e.g.
+
+```js
+// main.cjs, the CommonJS entry
+exports.foo = 'foo';
+exports.bar = 'bar';
+
+// main.mjs, the ES module entry
+export { foo, bar } from './main.cjs';
+
+// rollup.config.mjs
+export default {
+  input: 'main.mjs',
+  output: {
+    format: 'es',
+    file: 'bundle.mjs'
+  }
+};
+```
+
+When bundling to CommonJS, i.e `output.format === 'cjs'`, make sure that you do not set `output.exports` to `'named'`. The default value of `'auto'` will usually work, but you can also set it explicitly to `'default'`. That makes sure that Rollup assigns the default export that was generated for your CommonJS entry point to `module.exports`, and semantics do not change.
+
 ## Using with @rollup/plugin-node-resolve
 
 Since most CommonJS packages you are importing are probably dependencies in `node_modules`, you may need to use [@rollup/plugin-node-resolve](https://github.com/rollup/plugins/tree/master/packages/node-resolve):

packages/commonjs/src/generate-imports.js

@@ -165,7 +165,7 @@ function processRequireExpressions(
   magicString
 ) {
   const generateRequireName = getGenerateRequireName();
-  for (const { source, id: resolvedId, isCommonJS } of requireTargets) {
+  for (const { source, id: resolvedId, isCommonJS, wrappedModuleSideEffects } of requireTargets) {
     const requires = requiresBySource[source];
     const name = generateRequireName(requires);
     let usesRequired = false;
@@ -184,7 +184,11 @@ function processRequireExpressions(
       } else if (canConvertRequire) {
         needsImport = true;
         if (isCommonJS === IS_WRAPPED_COMMONJS) {
-          magicString.overwrite(node.start, node.end, `${name}()`);
+          magicString.overwrite(
+            node.start,
+            node.end,
+            `${wrappedModuleSideEffects ? '' : '/*@__PURE__*/ '}${name}()`
+          );
         } else if (usesReturnValue) {
           usesRequired = true;
           magicString.overwrite(node.start, node.end, name);

packages/commonjs/src/helpers.js

@@ -2,12 +2,19 @@ export const isWrappedId = (id, suffix) => id.endsWith(suffix);
 export const wrapId = (id, suffix) => `\0${id}${suffix}`;
 export const unwrapId = (wrappedId, suffix) => wrappedId.slice(1, -suffix.length);
 
+// A proxy module when a module is required from non-wrapped CommonJS. Is different for ESM and CommonJS requires.
 export const PROXY_SUFFIX = '?commonjs-proxy';
+// Indicates that a required module is wrapped commonjs and needs special handling.
 export const WRAPPED_SUFFIX = '?commonjs-wrapped';
+// Indicates that a required module is external
 export const EXTERNAL_SUFFIX = '?commonjs-external';
+// A helper module that contains the exports object of a module
 export const EXPORTS_SUFFIX = '?commonjs-exports';
+// A helper module that contains the module object of a module, e.g. when module.exports is reassigned
 export const MODULE_SUFFIX = '?commonjs-module';
+// A special proxy for CommonJS entry points
 export const ENTRY_SUFFIX = '?commonjs-entry';
+// A proxy when wrapped ESM is required from CommonJS
 export const ES_IMPORT_SUFFIX = '?commonjs-es-import';
 
 export const DYNAMIC_MODULES_ID = '\0commonjs-dynamic-modules';

packages/commonjs/src/index.js

@@ -1,4 +1,4 @@
-import { extname, relative, resolve, dirname } from 'path';
+import { dirname, extname, relative, resolve } from 'path';
 
 import { createFilter } from '@rollup/pluginutils';
 
@@ -239,7 +239,7 @@ export default function commonjs(options = {}) {
       }
     },
 
-    load(id) {
+    async load(id) {
       if (id === HELPERS_ID) {
         return getHelpersModule();
       }
@@ -285,7 +285,11 @@ export default function commonjs(options = {}) {
 
       if (isWrappedId(id, ES_IMPORT_SUFFIX)) {
         const actualId = unwrapId(id, ES_IMPORT_SUFFIX);
-        return getEsImportProxy(actualId, getDefaultIsModuleExports(actualId));
+        return getEsImportProxy(
+          actualId,
+          getDefaultIsModuleExports(actualId),
+          (await this.load({ id: actualId })).moduleSideEffects
+        );
       }
 
       if (id === DYNAMIC_MODULES_ID) {

packages/commonjs/src/proxies.js

@@ -57,26 +57,28 @@ export function getEntryProxy(id, defaultIsModuleExports, getModuleInfo, shebang
     }
     return shebang + code;
   }
-  const result = getEsImportProxy(id, defaultIsModuleExports);
+  const result = getEsImportProxy(id, defaultIsModuleExports, true);
   return {
     ...result,
     code: shebang + result.code
   };
 }
 
-export function getEsImportProxy(id, defaultIsModuleExports) {
+export function getEsImportProxy(id, defaultIsModuleExports, moduleSideEffects) {
   const name = getName(id);
   const exportsName = `${name}Exports`;
   const requireModule = `require${capitalize(name)}`;
   let code =
     `import { getDefaultExportFromCjs } from "${HELPERS_ID}";\n` +
     `import { __require as ${requireModule} } from ${JSON.stringify(id)};\n` +
-    `var ${exportsName} = ${requireModule}();\n` +
+    `var ${exportsName} = ${moduleSideEffects ? '' : '/*@__PURE__*/ '}${requireModule}();\n` +
     `export { ${exportsName} as __moduleExports };`;
   if (defaultIsModuleExports === true) {
     code += `\nexport { ${exportsName} as default };`;
+  } else if (defaultIsModuleExports === false) {
+    code += `\nexport default ${exportsName}.default;`;
   } else {
-    code += `export default /*@__PURE__*/getDefaultExportFromCjs(${exportsName});`;
+    code += `\nexport default /*@__PURE__*/getDefaultExportFromCjs(${exportsName});`;
   }
   return {
     code,

packages/commonjs/src/resolve-id.js

@@ -63,11 +63,7 @@ export default function getResolveId(extensions, isPossibleCjsId) {
       // All logic below is specific to ES imports.
       // Also, if we do not skip this logic for requires that are resolved while
       // transforming a commonjs file, it can easily lead to deadlocks.
-      if (
-        customOptions &&
-        customOptions['node-resolve'] &&
-        customOptions['node-resolve'].isRequire
-      ) {
+      if (customOptions?.['node-resolve']?.isRequire) {
         return null;
       }
       const currentlyResolvingForParent = currentlyResolving.get(importer);

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

@@ -192,13 +192,14 @@ export function getRequireResolver(extensions, detectCyclesAndConditional, curre
           // eslint-disable-next-line no-multi-assign
           const isCommonJS = (parentMeta.isRequiredCommonJS[dependencyId] =
             getTypeForFullyAnalyzedModule(dependencyId));
+          const isWrappedCommonJS = isCommonJS === IS_WRAPPED_COMMONJS;
           fullyAnalyzedModules[dependencyId] = true;
           return {
+            wrappedModuleSideEffects:
+              isWrappedCommonJS && rollupContext.getModuleInfo(dependencyId).moduleSideEffects,
             source: sources[index].source,
             id: allowProxy
-              ? isCommonJS === IS_WRAPPED_COMMONJS
-                ? wrapId(dependencyId, WRAPPED_SUFFIX)
-                : wrapId(dependencyId, PROXY_SUFFIX)
+              ? wrapId(dependencyId, isWrappedCommonJS ? WRAPPED_SUFFIX : PROXY_SUFFIX)
               : dependencyId,
             isCommonJS
           };

packages/commonjs/src/utils.js

@@ -43,10 +43,10 @@ export function capitalize(name) {
 
 export function getStrictRequiresFilter({ strictRequires }) {
   switch (strictRequires) {
-    case true:
-      return { strictRequiresFilter: () => true, detectCyclesAndConditional: false };
     // eslint-disable-next-line no-undefined
     case undefined:
+    case true:
+      return { strictRequiresFilter: () => true, detectCyclesAndConditional: false };
     case 'auto':
     case 'debug':
     case null:

packages/commonjs/test/fixtures/form/async-function/output.js

@@ -1,8 +1,15 @@
 import * as commonjsHelpers from "_commonjsHelpers.js";
 
-var input = async function () {
-	// TODO
-};
+var input;
+var hasRequiredInput;
 
-export default /*@__PURE__*/commonjsHelpers.getDefaultExportFromCjs(input);
-export { input as __moduleExports };
+function requireInput () {
+	if (hasRequiredInput) return input;
+	hasRequiredInput = 1;
+	input = async function () {
+		// TODO
+	};
+	return input;
+}
+
+export { requireInput as __require };

packages/commonjs/test/fixtures/form/compiled-esm-assign-exports/output.js

@@ -1,8 +1,15 @@
 import * as commonjsHelpers from "_commonjsHelpers.js";
 import { __exports as input } from "\u0000fixtures/form/compiled-esm-assign-exports/input.js?commonjs-exports";
 
-input.__esModule = true;
-var _default = input.default = 'x';
-var foo = input.foo = 'foo';
+var hasRequiredInput;
 
-export { input as __moduleExports, foo, _default as default };
+function requireInput () {
+	if (hasRequiredInput) return input;
+	hasRequiredInput = 1;
+	input.__esModule = true;
+	input.default = 'x';
+	input.foo = 'foo';
+	return input;
+}
+
+export { requireInput as __require };

packages/commonjs/test/fixtures/form/compiled-esm-assign-module/output.js

@@ -1,8 +1,15 @@
 import * as commonjsHelpers from "_commonjsHelpers.js";
 import { __exports as input } from "\u0000fixtures/form/compiled-esm-assign-module/input.js?commonjs-exports";
 
-input.__esModule = true;
-var _default = input.default = 'x';
-var foo = input.foo = 'foo';
+var hasRequiredInput;
 
-export { input as __moduleExports, foo, _default as default };
+function requireInput () {
+	if (hasRequiredInput) return input;
+	hasRequiredInput = 1;
+	input.__esModule = true;
+	input.default = 'x';
+	input.foo = 'foo';
+	return input;
+}
+
+export { requireInput as __require };

packages/commonjs/test/fixtures/form/compiled-esm-deconflict/output.js

@@ -1,9 +1,16 @@
 import * as commonjsHelpers from "_commonjsHelpers.js";
 import { __exports as input } from "\u0000fixtures/form/compiled-esm-deconflict/input.js?commonjs-exports";
 
-Object.defineProperty(input, '__esModule', { value: true });
-var foo_1 = input.foo = 'bar';
+var hasRequiredInput;
 
-const foo = 'also bar';
+function requireInput () {
+	if (hasRequiredInput) return input;
+	hasRequiredInput = 1;
+	Object.defineProperty(input, '__esModule', { value: true });
+	input.foo = 'bar';
 
-export { input as __moduleExports, foo_1 as foo, input as default };
+	const foo = 'also bar';
+	return input;
+}
+
+export { requireInput as __require };

packages/commonjs/test/fixtures/form/compiled-esm-define-exports-empty/output.js

@@ -1,6 +1,13 @@
 import * as commonjsHelpers from "_commonjsHelpers.js";
 import { __exports as input } from "\u0000fixtures/form/compiled-esm-define-exports-empty/input.js?commonjs-exports";
 
-Object.defineProperty(input, "__esModule", { value: true });
+var hasRequiredInput;
 
-export { input as __moduleExports, input as default };
+function requireInput () {
+	if (hasRequiredInput) return input;
+	hasRequiredInput = 1;
+	Object.defineProperty(input, "__esModule", { value: true });
+	return input;
+}
+
+export { requireInput as __require };

packages/commonjs/test/fixtures/form/compiled-esm-define-exports/output.js

@@ -1,8 +1,15 @@
 import * as commonjsHelpers from "_commonjsHelpers.js";
 import { __exports as input } from "\u0000fixtures/form/compiled-esm-define-exports/input.js?commonjs-exports";
 
-Object.defineProperty(input, '__esModule', { value: true });
-var _default = input.default = 'x';
-var foo = input.foo = 'foo';
+var hasRequiredInput;
 
-export { input as __moduleExports, foo, _default as default };
+function requireInput () {
+	if (hasRequiredInput) return input;
+	hasRequiredInput = 1;
+	Object.defineProperty(input, '__esModule', { value: true });
+	input.default = 'x';
+	input.foo = 'foo';
+	return input;
+}
+
+export { requireInput as __require };

packages/commonjs/test/fixtures/form/compiled-esm-define-module/output.js

@@ -1,8 +1,15 @@
 import * as commonjsHelpers from "_commonjsHelpers.js";
 import { __exports as input } from "\u0000fixtures/form/compiled-esm-define-module/input.js?commonjs-exports";
 
-Object.defineProperty(input, '__esModule', { value: true });
-var _default = input.default = 'x';
-var foo = input.foo = 'foo';
+var hasRequiredInput;
 
-export { input as __moduleExports, foo, _default as default };
+function requireInput () {
+	if (hasRequiredInput) return input;
+	hasRequiredInput = 1;
+	Object.defineProperty(input, '__esModule', { value: true });
+	input.default = 'x';
+	input.foo = 'foo';
+	return input;
+}
+
+export { requireInput as __require };

packages/commonjs/test/fixtures/form/compiled-esm-minified/output.js

@@ -1,7 +1,14 @@
 import * as commonjsHelpers from "_commonjsHelpers.js";
 import { __exports as input } from "\u0000fixtures/form/compiled-esm-minified/input.js?commonjs-exports";
 
-Object.defineProperty(input, '__esModule', { value: !0 });
-var foo = input.foo = 'foo';
+var hasRequiredInput;
 
-export { input as __moduleExports, foo, input as default };
+function requireInput () {
+	if (hasRequiredInput) return input;
+	hasRequiredInput = 1;
+	Object.defineProperty(input, '__esModule', { value: !0 });
+	input.foo = 'foo';
+	return input;
+}
+
+export { requireInput as __require };

packages/commonjs/test/fixtures/form/compiled-esm-only-named/output.js

@@ -1,8 +1,15 @@
 import * as commonjsHelpers from "_commonjsHelpers.js";
 import { __exports as input } from "\u0000fixtures/form/compiled-esm-only-named/input.js?commonjs-exports";
 
-Object.defineProperty(input, '__esModule', { value: true });
-var foo = input.foo = 'bar';
-var bar = input.bar = 'foo';
+var hasRequiredInput;
 
-export { input as __moduleExports, foo, bar, input as default };
+function requireInput () {
+	if (hasRequiredInput) return input;
+	hasRequiredInput = 1;
+	Object.defineProperty(input, '__esModule', { value: true });
+	input.foo = 'bar';
+	input.bar = 'foo';
+	return input;
+}
+
+export { requireInput as __require };

packages/commonjs/test/fixtures/form/compiled-esm-reassign-exports/output.js

@@ -2,9 +2,14 @@ import * as commonjsHelpers from "_commonjsHelpers.js";
 import { __module as inputModule } from "\u0000fixtures/form/compiled-esm-reassign-exports/input.js?commonjs-module";
 var input = inputModule.exports;
 
-Object.defineProperty(input, '__esModule', { value: true });
-inputModule.exports = { foo: 'bar' };
+var hasRequiredInput;
 
-var inputExports = inputModule.exports;
-export default /*@__PURE__*/commonjsHelpers.getDefaultExportFromCjs(inputExports);
-export { inputExports as __moduleExports };
+function requireInput () {
+	if (hasRequiredInput) return inputModule.exports;
+	hasRequiredInput = 1;
+	Object.defineProperty(input, '__esModule', { value: true });
+	inputModule.exports = { foo: 'bar' };
+	return inputModule.exports;
+}
+
+export { requireInput as __require };