feat(commonjs): support strict require semantics for circular imports

[lukastaegert]

Rollup Plugin Name: commonjs

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

Are tests included?

• yes (bugfixes and features will not be merged without tests)
• no

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no

If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.

This includes the following breaking changes:

• minimum Rollup version raised to 2.60.0 to support this.load for plugins
• minimum Node version raised to 12. This allows us to use "async await" (though 10 would have been enough for that...)
• if you use @rollup/plugin-node-resolve, it needs to be at least v13.0.6
• Even with dynamicRequireTargets, packages that manipulate the commonjs cache like request-promise-native are no longer supported

List any relevant issue numbers:
resolves #985
resolves #988
resolves #1038
resolves #1044
resolves #1014
resolves #1082
resolves rollup/rollup#1507
resolves rollup/rollup#2747
resolves rollup/rollup#3805
resolves rollup/rollup#3816
resolves rollup/rollup#4187
resolves rollup/rollup#4195
resolves rollup/rollup#4216
resolves rollup/rollup#4231
resolves rollup/rollup#4273

Description

This introduces a new option strictRequires: "auto" | boolean | "debug" | string[] with a default of "auto". This will allow for some/all files to be wrapped in function closures so that require calls are retained as actual function calls and the first require of a module will run the module while subsequent calls will just return its current value of module.exports. true will activate it for all CommonJS files (at a small size/performance penalty), false will use the old hoisting logic for all files and you can also provide a list of mini match patterns to filter which files use the new mode.

Here is an example what this might look like for strictRequires: ['foo.js']:

// main.js
console.log(require('./foo.js));

// foo.js
exports.foo = 'bar';

// output
var foo = {};
var hasRequiredFoo; // not specifying a value is shorter than "false" for minification purposes.

function requireFoo () {
  if (hasRequiredFoo) return foo; // if this is not the first time we are importing, return exports
  hasRequiredFoo = 1; // track that we have run the module. "1" is shorter than "true" for minification purposes.
  foo.foo = 'bar';
  return foo;
}

console.log(requireFoo());

As you may already see, it already recognizes that we did not reassign module.exports and can therefore just track exports, which is called foo here re-using the module name. The variables hasRequiredFoo and requireFoo will also re-use the module name to make the code more readable. If we reassign module.exports, the code will adapt accordingly:

// main.js
console.log(require('./foo.js));

// foo.js
module.exports = 'replaced'

// output
var foo;
var hasRequiredFoo;

function requireFoo () {
  if (hasRequiredFoo) return foo;
  hasRequiredFoo = 1;
  foo = 'replaced';
  return foo;
}

console.log(requireFoo());

and if more advanced things happen, it will change further (this is mirroring the existing logic we have for hoisted modules):

// main.js
console.log(require('./foo.js));

// foo.js
if (Math.random() < 0.5) {
  module.exports = 'replaced';
} else {
  exports.foo = 'bar';
}

// output
var foo = { exports: {} };
var hasRequiredFoo;

function requireFoo () {
  if (hasRequiredFoo) return foo.exports;
  hasRequiredFoo = 1;
  if (Math.random() < 0.5) {
    foo.exports = 'replaced';
  } else {
   foo.exports.foo = 'bar';
  }
  return foo.exports;
}

console.log(requireFoo());

Why not use a shared helper to track module execution?

The implemented approach may produce slightly (but only slightly) larger results than using a shared require function. But it has some advantages:

• requiring is only a single function call and thus rather fast
• it can be easily statically analyzed. In the future, Rollup could possibly decide to simplify and inline/remove the wrappers again if it makes sense.
• it works really well with code-splitting

What about "auto" and "debug" mode?

Here is the real magic: Having a configuration option is ok, but you do not want to learn how to use it correctly. As it turns out, nearly 100% of all issues we have result from cyclic dependencies. And with the new this.load helper, we can now figure out if a module is part of a cycle before we finished transforming the module. Thus by default, all modules that are part of a cycle will be wrapped magically and cyclic dependencies should now finally "just work" out of the box!
Note that using any value other than "auto" or "debug" for strictRequires will disable cycle detection. "debug" on the other hand works like "auto" but in the end emits a warning telling you exactly which modules were wrapped automatically so that you could copy and paste this array for manual tweaking.

What about dynamicRequireTargets?

I completely re-implemented this feature so that it is now only about dynamic requires. This should also fix some bugs related to code splitting. The new logic will no longer "register" modules, but there is a central registry for dynamic modules that just contains an object with the require functions of all dynamic modules. Modules designated as dynamic require targets are automatically wrapped in functions. The registry itself is only accessed via function helpers, and as a function is always hoisted, there should not be any issues even with complex cyclic dependencies.

Note however that the current implementation does not fully support everything dynamicRequireTargets supported before. Mainly support for require.cache is missing as this feature is no longer easily added to the logic.

Further changes

• Use async-await instead of Promises in the plugin code
• Meta-data from plugins returned in resolveId hooks was not attached to CommonJS modules. This is fixed now.
• Infer the module type by usage for modules that have neither ESM features nor CommonJS features. I.e. if such a module is required, we assume it is CommonJS, otherwise we assume it is an ES module. This is important to decide if we should wrap if if the strictRequires option is used.
• Use the options hook to inject a separate "resolver" plugin as first plugin. This is necessary as the commonjs plugin needs to be aware whenever an entry point is resolved to be able to inject an entry point ESM wrapper if strictRequires is used. Otherwise, "catch-all" resolvers like node-resolve will prevent the commonjs plugin from getting to resolve those. This is an interesting pattern for plugins that need to proxy some modules but usually just call this.resolve in their resolveId hook anyway to defer to other plugins.
• When requiring an ES module with a default export that is a function, make the returned namespace callable itself to solve some interop issues.
• Fix an issue where external files that were resolved to different ids by plugins were no longer treated as external. Resolves external files are being bundled for commonjs #1044 and external files are being bundled for commonjs rollup#4273
• Limit the ignoreTryCatch option to external imports
• Automatically wrap modules in dependency cycles, as well as modules that are required conditionally
• add dynamicRequireRoot option to control which paths are stored in the bundle when using dynamic requires
• throw if dynamic requires are encountered outside the dynamicRequireRoot option
• make sure injected helpers never conflict with used global variables
• expose plugin version on plugin object
• do not transform "typeof module/module.exports/exports" for mixed modules
• when using dynamicRequireTargets, ALL usages of require that are not part of a call expression are replaced with a function createCommonjsRequire('/path/to/module') so that it is possible to pass them around will still enabling them to be used. This extends to module.require as well and also includes support for require.resolve.

[lukastaegert]

It seems like prettier is flagging my build, which is formatted using only ESLint. Should I use prettier now instead of ESLint as they seem to be at odds here?

[shellscape]

pnpm lint should take care of it

[lukastaegert]

Unfortunately not. It finds quite a few other issues in other packages but not the ones in my package. Reason seems to be that pnpm lint is running ESLint last, while CI appears to be running pnpm prettier:check, which is not equivalent.

[shellscape]

I'm not sure how you are running things on your end, but we don't usually see an issue with this setup.

[lukastaegert]

I will just try to rewrite the code snippet in question so that ESLint and prettier do not disagree. Funnily, it seems to be that the prettier/prettier rule of ESLint disagrees with prettier...

[kzc]

The PR looks pretty solid.

Here's a couple of failures I encountered with this PR that worked with other bundlers and the alternative commonjs plugin from rollup/rollup#4195 (comment):

$ cat unreached.js 
0 && require("./error.js") || console.log("PASS1");

$ cat error.js 
throw new Error("FAIL");

$ node unreached.js 
PASS1

$ esbuild unreached.js --bundle | node
PASS1
 
$ node_modules/rollup/dist/bin/rollup unreached.js -p ../commonjs/dist/index.js -p node-resolve -f cjs --exports auto --silent | node
[stdin]:5
throw new Error("FAIL");
^
Error: FAIL

$ cat ms.js
const MagicString = require("magic-string"); // [email protected]
var m = new MagicString("0123456789");
console.log(m.prependRight(0, "W").prependLeft(3, "AB").appendRight(9, "XY").remove(6,8).toString());
var bundle = new MagicString.Bundle();
bundle.addSource({ filename: "foo.txt", content: m });
var map = bundle.generateMap({ file: "bundle.txt", includeContent: true, hires: true });
console.log(JSON.stringify(map));

$ node ms.js 
W012AB3458XY9
{"version":3,"file":"bundle.txt","sources":["foo.txt"],"sourcesContent":["0123456789"],"names":[],"mappings":"CAAA,CAAC,CAAC,GAAC,CAAC,CAAC,CAAG,GAAC"}

$ esbuild ms.js --bundle | node
W012AB3458XY9
{"version":3,"file":"bundle.txt","sources":["foo.txt"],"sourcesContent":["0123456789"],"names":[],"mappings":"CAAA,CAAC,CAAC,GAAC,CAAC,CAAC,CAAG,GAAC"}

$ node_modules/rollup/dist/bin/rollup ms.js -p ../commonjs/dist/index.js -p node-resolve -f cjs --exports auto --silent | node
[stdin]:1389
var m = new MagicString("0123456789");
        ^
TypeError: MagicString is not a constructor

There's also a discrepancy when strictRequireSemantic: true is used with transformMixedEsModules: true:

$ cat mixed.js 
var {hi} = require("./greetings");
hi();
import "./mod";

$ cat greetings.js 
!function(ex) {
    ex.hi = () => console.log("Hello");
}(exports);

$ cat mod.js 
console.log(require("./echo.js"));

$ cat echo.js 
module.exports = 1003;
console.log("Echo");

$ esbuild mixed.js --bundle --format=esm | node --input-type=module
Echo
1003
Hello

$ node_modules/rollup/dist/bin/rollup mixed.js -p ../commonjs/dist/index.js='{transformMixedEsModules:true}' -p node-resolve -f es --silent | node --input-type=module
Echo
1003
Hello

$ node_modules/rollup/dist/bin/rollup mixed.js -p ../commonjs/dist/index.js='{strictRequireSemantic:true,transformMixedEsModules:true}' -p node-resolve -f es --silent | node --input-type=module
Hello

An unimportant note that won't affect anyone else... As I don't use pnpm I had to apply this patch to correctly build this commonjs plugin PR with npm@6 from sources. Without this patch npm was installing [email protected] without the yet to be merged load function and was failing as result:

--- a/packages/commonjs/package.json
+++ b/packages/commonjs/package.json
@@ -66 +66 @@
-    "rollup": "^2.59.0-0",
+    "rollup": "2.59.0-0",

Edit: the mixed test was simplified.

[kzc]

Should the strictRequireSemantic option be renamed strictRequireSemantics since it's generally used in the plural form? Or perhaps the more succinct strictRequires or simply strict?

[lukastaegert]

Thanks for having a look!

Should the strictRequireSemantic option be renamed strictRequireSemantics since it's generally used in the plural form? Or perhaps the more succinct strictRequires or simply strict?

strict is probably too easily confused with other things that could be "strict" 😉

strictRequires sounds good to me as it still seems to carry the point and avoids a word that maybe not everyone understands.

The first issue you mention looks to me like it could only be solved from the plugin if we turn the feature on for all modules. I would be hesitant to do that as there is quite some impact from doing that with regard to bundle size and performance and I would regard this very much as an edge case. As long as it works correctly with strictRequires I would be happy as the idea should be that strictRequires should give you a guarantee that stuff works.

I will need to look into the other issues, that does look buggy to me.

[kzc]

The first issue you mention looks to me like it could only be solved from the plugin if we turn the feature on for all modules. I would be hesitant to do that as there is quite some impact from doing that with regard to bundle size and performance and I would regard this very much as an edge case

It's not uncommon to have a conditional require in debug code that is optimized away in production. Build time conditional polyfills are another example.

In any case, I am unable to get that first example working even with the strict require option.

$ node_modules/rollup/dist/bin/rollup unreached.js -p ../commonjs/dist/index.js='{strictRequireSemantic:true}' -p node-resolve -f cjs --exports auto --silent | node
[stdin]:5
throw new Error("FAIL");
^
Error: FAIL

[lukastaegert]

Unfortunately, it is not possible to detect conditional requires reliably even with this.load as the first entry point may use a top-level require while the second entry may use a conditional one. In such a situation, the resulting output could vary depending on which resolves faster, something we want to avoid.

I will also look why the first example does not work with strict semantics.

[kzc]

The commonjs circular test case in rollup/rollup#4195 (comment) also fails with this commonjs PR for the same reason that unreached.js fails above.

If any commonjs global variables (module, exports) are introduced into error.js above and cat.js then the commonjs plugin in this PR will work correctly if strict require semantics are used:

$ cat unreached2.js 
0 && require("./error2.js") || console.log("PASS2");

$ cat error2.js 
console.log("FAIL2");
exports.BLAH = 0;     // <-----------

$ node unreached2.js 
PASS2

$ node_modules/rollup/dist/bin/rollup unreached2.js -p ../commonjs='{strictRequireSemantic:true}' -p node-resolve -f esm --silent | node --input-type=module
PASS2

However, the default commonjs plugin behavior will still continue to fail:

$ node_modules/rollup/dist/bin/rollup unreached2.js -p ../commonjs -p node-resolve -f esm --silent | node --input-type=module
FAIL2
PASS2

So the obvious plugin fix would be to treat any required source file that lacks CJS or ESM keywords as if it were a CJS module for its side effects. This pattern is common for polyfills.

[lukastaegert]

So the obvious plugin fix would be to treat any required source file that lacks CJS or ESM keywords as if it were a CJS module for its side effects. This pattern is common for polyfills

Here the same logic applies as before: What if the file is also imported elsewhere? But as this will be a breaking release anyway, we can experiment here. So if the first "import" of a file that cannot be identified is a require, I will see if we can identify it as CommonJS.

In the end, we should eventually teach the node-resolve plugin to identify file types based on extension or package.json, at least in the node_modules folder. Then we no longer need to rely on auto-detection for those files.

[lukastaegert]

I renamed the option to strictRequires and added a logic that will make non-identifiable modules "commonjs" if their first import is a require.

[lukastaegert]

The "magic-string" issue is not a regression but "just" an interop issue. The reason is that due to the presence of the module property in magic-string's package.json, Rollup imports the ESM version of magic-string and returns its namespace, which hast the MagicString property as its default constructor. There are several ways how to solve this:

• fix magic-string by providing an exports field in its package.json file that distinguishes between import and require (preferred solution)
• instantiate new MagicString.default() instead of new MagicString() in the test code, accepting that you are dealing with an ESM namespace
• set mainFields: ['main'] in the node-resolve plugin (thus ignoring the module property)

I can only assume esbuild works because it ignores the module property.

However it appears there might be another issue when using CommonJS with strict require semantics as entry points, looking into it.

[lukastaegert]

So there is an interaction with the node-resolve plugin that I did not foresee. Basically with these changes, the commonjs plugin always needs to be placed before the node-resolve plugin, otherwise CommonJS entries will not work with strictRequires. I wonder if we should just let the plugin re-order plugins in the options hook if necessary. Otherwise this will cause a lot of grief.

[kzc]

I can only assume esbuild works because it ignores the module property.

If memory serves, esbuild loads the CJS version of the module if it is required after applying its NodeJS module resolution rules against magic-string/package.json, just as NodeJS does:

$ node ms.js
*** magic-string.cjs.js
W012AB3458XY9
{"version":3,"file":"bundle.txt","sources":["foo.txt"],"sourcesContent":["0123456789"],"names":[],"mappings":"CAAA,CAAC,CAAC,GAAC,CAAC,CAAC,CAAG,GAAC"}

$ esbuild ms.js --bundle --format=esm | node --input-type=module
*** magic-string.cjs.js
W012AB3458XY9
{"version":3,"file":"bundle.txt","sources":["foo.txt"],"sourcesContent":["0123456789"],"names":[],"mappings":"CAAA,CAAC,CAAC,GAAC,CAAC,CAAC,CAAG,GAAC"}

The magic string bundles have been modified to print which version is being used.

There are several ways how to solve this:

There's another way to have a commonjs require deal with an ES module with named exports and a default export. Please see my alternative plugin implementation in rollup/rollup#4195 (comment) which also works with [email protected] whether the CJS or ESM version is requested via node-resolve's mainFields:

$ rollup ms.js -p node-resolve -p strict-cjs --silent | node --input-type=module
*** magic-string.es.js
W012AB3458XY9
{"version":3,"file":"bundle.txt","sources":["foo.txt"],"sourcesContent":["0123456789"],"names":[],"mappings":"CAAA,CAAC,CAAC,GAAC,CAAC,CAAC,CAAG,GAAC"}

$ rollup ms.js -p node-resolve='{"mainFields":["main"]}' -p strict-cjs --silent | node --input-type=module
*** magic-string.cjs.js
W012AB3458XY9
{"version":3,"file":"bundle.txt","sources":["foo.txt"],"sourcesContent":["0123456789"],"names":[],"mappings":"CAAA,CAAC,CAAC,GAAC,CAAC,CAAC,CAAG,GAAC"}

[lukastaegert]

I added a change to the plugin so that it automatically places the node-resolve plugin after commonjs if present and not yet placed correctly

[kzc]

It'd be ideal if the test case ms.js just worked out of the box using [email protected] without application-specific bundling options just as esbuild, webpack and rollup-plugin-strict-cjs handle it. Is there a way for @rollup/plugin-commonjs to also accomplish this with default options?

[kzc]

Hmm, I'm a bit baffled by this... I am unable to get the commonjs plugin in this PR to work on sources with require cycles when used with the recent version of @rollup/plugin-node-resolve specified in the latest rollup tree:

https://github.com/rollup/rollup/blob/8d98341bf746d4baa57bbd730b1fa6449555cfca/package.json#L65

No output and no errors are produced for me when using the latest version of node-resolve for applications with require cycles.

This PR's rollup-plugins/plugins/packages/commonjs/package.json is specifying a rather old version of node-resolve:

plugins/packages/commonjs/package.json

Line 63 in efd0273

"@rollup/plugin-node-resolve": "^8.4.0",

Applications with require cycles do work with this PR's commonjs plugin when using this older version of node-resolve, but not the most recent version. I'm using rollup v2.60.0 for testing. Is there anything obvious that I'm doing wrong?

[kzc]

An example of the node-resolve issue above using the commonjs plugin from efd0273:

$ cat one.js
console.log(module.exports = 1, require("./two"));

$ cat two.js
console.log(module.exports = 2, require("./one"));

$ node one.js
2 1
1 2

$ npm i @rollup/plugin-node-resolve@8 >/dev/null 2>&1 && npm ls | egrep 'rollup@|plugin-node-resolve' && node_modules/.bin/rollup one.js -p node-resolve -p ../commonjs --silent | node --input-type=module
├─┬ @rollup/[email protected]
├─┬ [email protected]
2 1
1 2

$ npm i @rollup/plugin-node-resolve@11 >/dev/null 2>&1 && npm ls | egrep 'rollup@|plugin-node-resolve' && node_modules/.bin/rollup one.js -p node-resolve -p ../commonjs --silent | node --input-type=module
├─┬ @rollup/[email protected]
├─┬ [email protected]
2 1
1 2

$ npm i @rollup/plugin-node-resolve@13 >/dev/null 2>&1 && npm ls | egrep 'rollup@|plugin-node-resolve' && node_modules/.bin/rollup one.js -p node-resolve -p ../commonjs --silent | node --input-type=module
├─┬ @rollup/[email protected]
├─┬ [email protected]
$      # no error, no output from rollup

[lukastaegert]

Unfortunately I will need to look into this on Monday. The problem was that node-resolve was preventing the plugin from proxying entry points.

[kzc]

The following patch adapted from rollup-plugin-strict-cjs gives @rollup/plugin-commonjs the ability to run the unmodified test ms.js correctly with the rollup defaulted ES module form of [email protected] without the need for custom plugin options:

--- a/packages/commonjs/src/helpers.js
+++ b/packages/commonjs/src/helpers.js
@@ -35,5 +35,9 @@ export function getDefaultExportFromNamespaceIfNotNamed (n) {
 export function getAugmentedNamespace(n) {
        if (n.__esModule) return n;
-       var a = Object.defineProperty({}, '__esModule', {value: true});
+       if (typeof n == "object" && typeof n.default == "function") {
+               var o = function() { return n.default.apply(this, arguments); };
+               o.prototype = n.default.prototype;
+       } else o = {__proto__: null};
+       var a = Object.defineProperty(o, '__esModule', {value: true});
        Object.keys(n).forEach(function (k) {
                var d = Object.getOwnPropertyDescriptor(n, k);

$ node_modules/rollup/dist/bin/rollup ms.js -p ../commonjs -p node-resolve -f es --silent | node --input-type=module
W012AB3458XY9
{"version":3,"file":"bundle.txt","sources":["foo.txt"],"sourcesContent":["0123456789"],"names":[],"mappings":"CAAA,CAAC,CAAC,GAAC,CAAC,CAAC,CAAG,GAAC"}

The CJS main form of [email protected] also continues to work:

$ node_modules/rollup/dist/bin/rollup ms.js -p ../commonjs -p node-resolve='{mainFields:["main"]}' -f es --silent | node --input-type=module
W012AB3458XY9
{"version":3,"file":"bundle.txt","sources":["foo.txt"],"sourcesContent":["0123456789"],"names":[],"mappings":"CAAA,CAAC,CAAC,GAAC,CAAC,CAAC,CAAG,GAAC"}

[bhovhannes]

@lukastaegert sorry for tagging you directly. I have an issue in upgrading from "@rollup/plugin-commonjs"@21.1.0 to "@rollup/plugin-commonjs"@latest. I tracked problem down to version "@rollup/plugin-commonjs"@22.0.0 and it looks like all changes in this version are in this PR. I hope you'll be able to provide valuable insights only by reading my question, that's why I tagged you.

I have a case when n which arrives to getAugmentedNamespace function already contains __esModule property.
As the if (n.__esModule) return n; line was removed by this PR, the code now throws at the Object.defineProperty(a, k, d.get ? d : { line because it tries to define __esModule property again.

Unfortunately my codebase is complex and I didn't managed to create a reproducible repo for opening an issue. Looking into exports of n it looks similar to react-dom (we use v17.0.2), but that's all I was able to find. Can you please tell me in which circumstances getAugmentedNamespace helper is added to the bundle? That may help with creating a repo to reproduce and better understanding of root cause.

How do you feel about adding back the if (n.__esModule) return n; line or modifying Object.keys(n).forEach(function (k) { line to be Object.keys(n).filter(k => k !== '__esModule').forEach(function (k) { ?

Thanks!

[lukastaegert]

It is actually described in the readme https://github.com/rollup/plugins/blob/34f4c4f294b1c024552fc9674954a2d6b978e360/packages/commonjs/README.md#requirereturnsdefault in the requireReturnsDefault section, though the helper code there is not the most current one. It is only used when requireReturnsDefault is set to false or not set. It is used when importing an ES module, or when importing a module where the type cannot be determined.
But yes, that is an oversight. I guess the motivation was that __esModule is a valid ES export that would however then return a namespace object without a prototype while require usually returns an object that has the Object.prototype.
A solution could be to make __esModule configurable. One would need to look into old test cases to see if adding the __esModule short cut back breaks anything important.

[bhovhannes]

Thank you very much! That was super helpful.
Adding requireReturnsDefault: 'namespace' seems to be solving issue in my case. react-dom is external in my case, so I guess this is actually what we want.
I'll try to add back a __esModule shortcut and see if PR will be green.

[bhovhannes]

@lukastaegert unfortunately adding requireReturnsDefault option fixes the code for react-dom but resulted in a bad code generated to bundle some other NPM package. After a number of tries I think adding check back will be a much better fix.
I opened a MR - #1379. Only snapshot tests failed, and I reviewed them one by one and the difference was only in the line which I added back to helper. Which makes me think I didn't break something.
I'll be grateful for your review. Thanks!