2.55.0 regression - Unable to bundle semver@7

[nicolo-ribaudo]

Rollup Version

2.55.0

Operating System (or Browser)

Ubuntu 21.04, but it probably doesn't matter

Node Version (if applicable)

16.3.0

Link To Reproduction

https://github.com/nicolo-ribaudo/rollup-2.55-bug-semver-7

Expected Behaviour

Rollup 2.55 should produce the same valid code as produced by Rollup 2.54

Actual Behaviour

Due to a cyclic dependency, it emits some code that uses a variable before that it's initialized.

---

⚠️ When updating to @rollup/plugin-commonjs@19, this bug disappears. However, a minor version of Rollup shouldn't force me to update a major version of a plugin (also, @rollup/plugin-commonjs@19 isn't working for me: rollup/plugins#962).

[kzc]

If treeshake is disabled then the latest version of rollup does work with @rollup/plugin-commonjs@18:

$ rollup -v
rollup v2.55.1

$ grep '"version":' node_modules/\@rollup/plugin-commonjs/package.json 
  "version": "18.1.0"

$ rollup index.mjs --no-treeshake -p node-resolve -p ./node_modules/\@rollup/plugin-commonjs --silent | node
true

$ rollup index.mjs --treeshake -p node-resolve -p ./node_modules/\@rollup/plugin-commonjs --silent | node
[stdin]:2315
const {ANY: ANY$1} = require$$26;
            ^
TypeError: Cannot destructure property 'ANY' of 'require$$26' as it is undefined.

Curiously if you take the good run above and pipe its no-treeshake rollup output through rollup again to treeshake it, it still will work:

$ rollup index.mjs --no-treeshake -p node-resolve -p ./node_modules/\@rollup/plugin-commonjs --silent | rollup --silent | node
true

So it's only a tree shaking problem if done within rollup on the initial run.

The rollup regression was introduced in 9a23190.

If this patch is applied:

--- a/src/ast/variables/ExportDefaultVariable.ts
+++ b/src/ast/variables/ExportDefaultVariable.ts
@@ -53,9 +53,8 @@ export default class ExportDefaultVariable extends LocalVariable {
        getDirectOriginalVariable(): Variable | null {
                return this.originalId &&
                        (this.hasId ||
                                !(
-                                       this.originalId.isPossibleTDZ() ||
                                        this.originalId.variable.isReassigned ||
                                        this.originalId.variable instanceof UndefinedVariable ||
                                        // this avoids a circular dependency
                                        'syntheticNamespace' in this.originalId.variable

then #4195 will work once again with treeshake enabled:

$ rollup index.mjs --treeshake -p node-resolve -p ./node_modules/\@rollup/plugin-commonjs --silent | node
true

However, test/function/samples/default-export-before-declaration will now fail.

[kzc]

When updating to @rollup/plugin-commonjs@19, this bug disappears.

Unpatched rollup v2.55.1 with @rollup/plugin-commonjs@19 fails on your test - whether treeshake is disabled or not - confirming that rollup/plugins#962 is a separate matter.

$ rollup -v
rollup v2.55.1

$ grep '"version":' node_modules/\@rollup/plugin-commonjs/package.json 
  "version": "19.0.2"

$ node index.mjs 
true

$ rollup index.mjs --no-treeshake -p node-resolve,./node_modules/\@rollup/plugin-commonjs --silent | node
false

$ rollup index.mjs --treeshake -p node-resolve,./node_modules/\@rollup/plugin-commonjs --silent | node
false

[lukastaegert]

Which again leads me to think we should add a flag to disable the TDZ detection logic, or at least the part that deals with TDZ access errors, because without it, Rollup accidentally paints over issues in the commonjs plugin. And to avoid "regressions", it should probably be disabled until we either fixed the commonjs plugin sufficiently or reach the next major version.

[veaba]

I thought it was semver problem.

:)

resolved: @rollup/plugin-commonjs@15 upgrade to @rollup/plugin-commonjs@19

[kzc]

TDZ and its undefined var before decl equivalent is an ES correctness issue. Disabling TDZ would only paper over the regression for the older @rollup/plugin-commonjs@18 with treeshake enabled since the introduction of #4182. It doesn't help when treeshake is disabled for #4195 with @rollup/plugin-commonjs@19, #4193 and #4187 for commonjs generated require cycles. This was a longstanding issue that just wasn't noticed before and was brought to light by the introduction of the TDZ detection logic. I appreciate that overhauling commonjs to deal with cycles is not a trivial piece of work, but I don't see another good solution.

[lukastaegert]

I understand your point but people are currently raising issues as regressions because we removed the papering. Most people have tree-shaking enabled. There is virtually zero benefit to nearly all users to have proper TDZ errors. Sure, it is not correct, but most people prefer working but slightly incorrect code over non-working code.
I do not see this as a long-term solution, but as a solution to avoid further Rollup 2 regression tickets.

[kzc]

Just want to make sure we're on the same page. The term "TDZ" is normally associated with ES6 style let and const variables only. The commonjs problems typically involve var variables used before declaration. The detection mechanism within rollup is the same, only the effect is different.

If we applied the following patch to latest master to simulate the disabling of TDZ by rollup by default:

--- a/src/ast/nodes/Identifier.ts
+++ b/src/ast/nodes/Identifier.ts
@@ -178,2 +178,4 @@ export default class Identifier extends NodeBase implements PatternNode {
        isPossibleTDZ(): boolean {
+               return false;
+
                // return cached value to avoid issues with the next tree-shaking pass

then the following ES5 code would fail:

$ echo 'console.log(x ? "FAIL" : "PASS"); var x = 1;' | node
PASS

$ echo 'console.log(x ? "FAIL" : "PASS"); var x = 1;' | rollup --silent | node
FAIL

Is this the default Rollup 2 behavior being proposed as a stop-gap fix?

[kzc]

fwiw, the version of rollup before TDZ logic was introduced does not work with the latest version of @rollup/plugin-commonjs and semver@7:

$ cat index.mjs 
import semver from "semver";
console.log(semver.satisfies("7.14.8", "^7.0.0"));

$ grep '"version":' node_modules/semver/package.json node_modules/\@rollup/plugin-commonjs/package.json 
node_modules/semver/package.json:  "version": "7.3.5",
node_modules/@rollup/plugin-commonjs/package.json:  "version": "20.0.0",

expected:

$ node index.mjs 
true

actual:

$ node_modules/.bin/rollup -v
rollup v2.52.7

$ node_modules/.bin/rollup index.mjs --treeshake -p node-resolve,./node_modules/\@rollup/plugin-commonjs --silent | node
false

$ node_modules/.bin/rollup index.mjs --no-treeshake -p node-resolve,./node_modules/\@rollup/plugin-commonjs --silent | node
false

$ echo 'console.log(x ? "FAIL" : "PASS"); var x = 1;' | node_modules/.bin/rollup --silent | node
FAIL

[lukastaegert]

I was very much under the impression that it is the const/let behaviour that is causing issues, and only that I want to put behind a toggle. The var behaviour is crucial in preventing bugs and should of course stay.

[kzc]

As far as I know, the few recent commonjs bug reports all involve var and disabling TDZ for let and const wouldn't help.

[kzc]

@rollup/plugin-commonjs should be able to handle require cycles with its dynamicRequireTargets option according to its documentation:

Some modules contain dynamic require calls, or require modules that contain circular dependencies, which are not handled well by static imports. Including those modules as dynamicRequireTargets will simulate a CommonJS (NodeJS-like) environment for them with support for dynamic and circular dependencies.

There are many test examples of its use, but I couldn't get it to work with non-trivial npm packages with circular require dependencies like semver and glob:

$ cat index.mjs 
import semver from "semver";
console.log(semver.satisfies("7.14.8", "^7.0.0"));

$ node index.mjs
true

$ rollup index.mjs -p node-resolve -p commonjs='{"dynamicRequireTargets":["node_modules/semver/**/*.js"],"transformMixedEsModules":true}' --silent | node
[stdin]:157
	throw new Error('Could not dynamically require "' + path + '". Please configure the dynamicRequireTargets or/and ignoreDynamicRequires option of @rollup/plugin-commonjs appropriately for this require call to work.');
	^

Error: Could not dynamically require "/$$rollup_base$$/node_modules/semver/index.js". Please configure the dynamicRequireTargets or/and ignoreDynamicRequires option of @rollup/plugin-commonjs appropriately for this require call to work.

If anyone is able to configure @rollup/plugin-commonjs correctly to use either of these packages from an ES entry point, please post the solution.

[Idered]

You can import one by one as workaround:

import maxSatisfying from "semver/ranges/max-satisfying";
import minSatisfying from "semver/ranges/min-satisfying";
import coerce from "semver/functions/coerce";

[kzc]

I've created a new plugin that can be used instead of @rollup/plugin-commonjs to correctly handle require cycles such as the ones reported in readable-stream #1507, #4231, semver #4195, glob #4187, #3805, #3816, protobufjs #4216, and winston rollup/rollup-plugin-commonjs#365.

It can be downloaded from here: rollup-plugin-strict-cjs-0.1.0.tar.gz and installed with:

npm i rollup-plugin-strict-cjs-0.1.0.tar.gz

The single source file for the plugin is included in the tarball package and could be used in user projects directly without the need for a package.

This plugin strives to be compatible with NodeJS cjs behavior. When mixed esm/cjs modules are used it tries to mimic esbuild's behavior. The generated bundled cjs code does not rely on treeshaking and works equally well with treeshaking disabled. The plugin options are similar to @rollup/plugin-commonjs, although not all options are supported and some defaults are different. In general the default options work fine for most projects.

Here's a simple example of a commonjs circular dependency, also known as a require cycle:

$ cat bird.js 
console.log("enter bird.js");
exports.prop = "BIRD";
var mod = require("./dog.js");
console.log(mod.prop);
console.log("leave bird.js");

$ cat dog.js 
console.log("enter dog.js");
exports.prop = "DOG";
var mod = require("./bird.js");
console.log(mod.prop);
console.log("leave dog.js");
0 && require("./cat.js");

$ cat cat.js
console.log("CAT");

Expected result:

$ node bird.js 
enter bird.js
enter dog.js
BIRD
leave dog.js
DOG
leave bird.js

@rollup/plugin-commonjs@20.0.0 result:

$ rollup bird.js -p node-resolve -p commonjs --silent | node --input-type=module
CAT
enter dog.js
undefined
leave dog.js
enter bird.js
DOG
leave bird.js

rollup-plugin-strict-cjs result:

$ rollup bird.js -p node-resolve -p strict-cjs --silent | node --input-type=module
enter bird.js
enter dog.js
BIRD
leave dog.js
DOG
leave bird.js

which matches the expected NodeJS output.

For the real world projects I've tried, rollup-plugin-strict-cjs produces bundles of comparable size to @rollup/plugin-commonjs@20.0.0 when minified and gzipped.

The plugin was tested on rollup itself by successfully building and running npm test with this patch applied:

diff --git a/build-plugins/conditional-fsevents-import.ts b/build-plugins/conditional-fsevents-import.ts
index 5378269..a98feff 100644
--- a/build-plugins/conditional-fsevents-import.ts
+++ b/build-plugins/conditional-fsevents-import.ts
@@ -17 +17 @@ export default function conditionalFsEventsImport(): Plugin {
-                       if (id.endsWith('fsevents-handler.js')) {
+                       if (id.endsWith('fsevents-handler.js?cjs')) {
diff --git a/rollup.config.ts b/rollup.config.ts
index 0f4061d..ab7914d 100644
--- a/rollup.config.ts
+++ b/rollup.config.ts
@@ -4 +4 @@ import alias from '@rollup/plugin-alias';
-import commonjs from '@rollup/plugin-commonjs';
+import commonjs from 'rollup-plugin-strict-cjs';

An implementation detail bleeds through in the rollup build case, namely the ?cjs suffix for the internal intermediate cjs files, but that's not typical in most projects using cjs modules. None of the recently proposed rollup plugin API extensions were used, although I understand the motivation for those changes given the workarounds this plugin had to perform. Feel free to take ideas from this plugin to improve @rollup/plugin-commonjs, or just use this plugin as is. I don't plan to publish or support it, although it's fairly well documented and can be easily changed.

[shellscape]

@guybedford this might be worth a look

[lemoustachiste]

@kzc thanks for diving into it and proposing a fix.

I looked at what you are saying in rollup/plugins#988, I find that I have a similar error but with another rollup plugin:

[!] (plugin strict-cjs) TypeError: Could not load node-resolve:empty.js?cjs (imported by node_modules/rdf-canonize/lib/index.js?cjs): The argument 'path' must be a string or Uint8Array without null bytes. Received '\x00node-resolve:empty.js'

This is I believe coming from this line: https://github.com/rollup/rollup-plugin-node-resolve/blob/master/src/index.js#L11. I am not sure I understand what's happening all in all so if you have any idea I am happy to hear them.

I think there is a reason for me to use that old plugin rather than the newer @rollup/plugin-node-resolve, I don't remember exactly but it had to do with builtins injections and breakage of my build, so I can't really move away from the legacy one.

[kzc]

[!] (plugin strict-cjs) TypeError: Could not load node-resolve:empty.js?cjs (imported by node_modules/rdf-canonize/lib/index.js?cjs): The argument 'path' must be a string or Uint8Array without null bytes. Received '\x00node-resolve:empty.js'

Cannot reproduce with latest versions of all packages:

$ rollup node_modules/rdf-canonize/lib/index.js -p node-resolve -p strict-cjs='{ignore:["rdf-canonize-native"]}' -f cjs --exports auto -o out.js && node out.js && echo ok

node_modules/rdf-canonize/lib/index.js → out.js...
created out.js in 477ms
ok

[lukastaegert]

@nicolo-ribaudo We are finalizing a new version of the commonjs plugin in rollup/plugins#1038. It is pre-published as @rollup/plugin-commonjs@beta. Please give it a spin to see if it resolves the issue, ideally without additional config needed.

[phil294]

@lukastaegert @rollup/plugin-commonjs@beta works great in my case. I am running a fork of Vetur, CoffeeSense, and even though I am not really sure what's going on, plugin-commonjs v18 until v21 inclusive breaks the build process, but 22 (beta) works fine again :-) Thanks for your work!