variable is used before declaration

[szdailei]

Rollup Version

2.52.8

Operating System (or Browser)

Linux

Node Version (if applicable)

v14.16.0

Link To Reproduction

https://github.com/szdailei/www/tree/main/packages/api-server

Expected Behaviour

Rollup 2.52.7 bundle glob 7.1.6 sync.js.
Bundled code is:

var sync = globSync$1;
globSync$1.GlobSync = GlobSync$1;

var fs$5 = fs__default;
var rp$1 = fs_realpath;
var minimatch$1 = minimatch_1;
var path$5 = path__default;
var assert$2 = require$$6;
var isAbsolute$1 = pathIsAbsolute.exports;
var common$1 = common$2;
common$1.alphasort;
common$1.alphasorti;

Actual Behaviour

Rollup 2.52.8 bundle glob 7.1.6 sync.js. glob_1 is used before declaration.
Bundled code is:

var sync = globSync$1;
globSync$1.GlobSync = GlobSync$1;

var fs$5 = fs__default;
var rp$1 = fs_realpath;
var minimatch$1 = minimatch_1;
glob_1.Glob;
var path$5 = path__default;
var assert$2 = require$$6;
var isAbsolute$1 = pathIsAbsolute.exports;
var common$1 = common$2;
common$1.alphasort;
common$1.alphasorti;

[nborko]

I've got the same issue when bundling glob, on Windows 10 19043.1110, Node v14.17.3.

Bundling fails on rollup 2.52.8 through 2.53.3, 2.52.7 works.

[kzc]

This issue is likely related to Rollup now respecting ECMAScript rules governing Temporal Dead Zone (TDZ) for const/let variables.

Before rollup would incorrectly process code like the following and mask user created and library errors:

$ cat tdz-example.js 
console.log(C && "WRONG BEHAVIOR"); const C = true;

expected ECMAScript runtime behavior:

$ cat tdz-example.js | node
[stdin]:1
console.log(C && "WRONG BEHAVIOR"); const C = true;
            ^
ReferenceError: Cannot access 'C' before initialization

rollup v2.52.7 incorrect output:

$ rollup tdz-example.js --silent
console.log("WRONG BEHAVIOR");

$ rollup tdz-example.js --silent | node
WRONG BEHAVIOR

rollup v2.53.3 correct output:

$ rollup tdz-example.js --silent
console.log(C && "WRONG BEHAVIOR"); const C = true;

$ rollup tdz-example.js --silent | node
[stdin]:1
console.log(C && "WRONG BEHAVIOR"); const C = true;
            ^
ReferenceError: Cannot access 'C' before initialization

[kzc]

A simple way to determine whether your code or some imported library has a TDZ error is to run the old (or new) version of rollup with --no-treeshake and then test the bundle as you normally would.

Edit:

NOTE: terser minification should also be disabled to test rollup input files for TDZ errors, as it does not reliably retain TDZ errors (as of terser 5.7.0).

[szdailei]

@kzc You are right. there is a TDZ error.
When I run rollup 2.52.7 with --no-treeshake, I got same error with rollup 2.52.8

[nborko]

In this case, globSync is a function, and aren't functions hoisted in ECMAScript?

[lukastaegert]

I wonder if it is an artifact of the commonjs conversion and the issue is not present in the original sources. Maybe we need a flag to disable TDZ detection after all until we can fix the CommonJS plugin, which is definitely harder.

[lukastaegert]

In this case, globSync is a function, and aren't functions hoisted in ECMAScript?

The function may be hoisted, but the property access in

var Glob = require('./glob.js').Glob

is not hoisted. The commonjs plugin replaces the require call with a variable that is undefined when used due to the imperfectly resolved circular dependency. It probably works in the original sources because here, the require calls are basically run like function calls. Until now, Rollup does not support circular CommonJS dependencies properly, unfortunately.

[kzc]

Maybe we need a flag to disable TDZ detection after all until we can fix the CommonJS plugin, which is definitely harder.

I don't think adding a flag to disable TDZ detection to have non-standard ES behavior could help considering that the problem also occurs when tree shaking is disabled altogether:

When I run rollup 2.52.7 with --no-treeshake, I got same error with rollup 2.52.8

But I'm still missing what's the actual problem. If we can create the smallest possible program demonstrating the issue we can compare it to an unbundled set of source files running directly in NodeJS and see whether it works there.

[lukastaegert]

The problem is very likely that the commonjs plugin does not wrap commonjs modules into function calls, maintaining a registry to resolve require calls. Instead, it tries to rewrite them to ESM directly. This works surprisingly well except when there are circular require calls in the code. In that case, the "correct" logic would be to

• Execute part of the current file, putting properties on exports until the require call
• Then execute the required file, resolving imports of the initial file with the current state of its exports object

It is the "half-finished exports object" that is usually missing and breaking things.

[kzc]

@szdailei Just out of curiosity could you please try running both rollup 2.52.7 and rollup 2.52.8 with --treeshake --no-treeshake.unknownGlobalSideEffects --no-treeshake.propertyReadSideEffects? This would eliminate glob_1.Glob; from the Actual Behavior output seen in the top post. This might be enough to paper over the real underlying issue (whatever that may be) and allow the code to run.

[kzc]

Confirmed that the workaround suggested above works...

Given:

$ cat glob-repro.mjs
import glob from "glob";
glob("*/glob/*js",{},(e,f)=>console.log(f.join(" ")));

Expected:

$ node glob-repro.mjs
node_modules/glob/common.js node_modules/glob/glob.js node_modules/glob/sync.js

Reproduction of issue:

$ rollup -v
rollup v2.54.0

$ rollup glob-repro.mjs -p node-resolve,commonjs --silent | node --input-type module
[eval1]:1883
glob_1.Glob;
       ^
TypeError: Cannot read property 'Glob' of undefined

Workaround:

$ rollup glob-repro.mjs --no-treeshake.unknownGlobalSideEffects --no-treeshake.propertyReadSideEffects -p node-resolve,commonjs --silent | node --input-type module
node_modules/glob/common.js node_modules/glob/glob.js node_modules/glob/sync.js

The diff between the two rollup output bundles above:

--- failing.mjs
+++ working.mjs
@@ -76,4 +76,2 @@
 
-pathModule.normalize;
-
 // Regexp that finds the next partion of a (partial) path
@@ -1882,3 +1880,2 @@
 var minimatch$1 = minimatch_1;
-glob_1.Glob;
 var path$1 = require$$0;

Ironically, the problematic line and local variable Glob in the glob package in node_modules/glob/sync.js is not used:

var Glob = require('./glob.js').Glob

and probably can be safely removed from the upstream package.

[kzc]

Here's a minimal repro of a bug where the previous workaround does not work...

$ cat test.mjs 
import foo from "./foo.js";
foo();

$ cat foo.js 
var foo = () => console.log("FOO");
module.exports = foo;
foo.prop = "PROP";
var bar = require('./bar.js');
console.log(bar);

$ cat bar.js 
module.exports = "BAR";
var prop = require('./foo.js').prop;
console.log(prop);

Expected output with node v14 and v16:

$ node test.mjs 
PROP
BAR
FOO

Actual:

$ rollup -v
rollup v2.54.0

$ rollup test.mjs -p commonjs --silent
var bar$1 = "BAR";
var prop = foo_1.prop;
console.log(prop);

var foo = () => console.log("FOO");
var foo_1 = foo;
foo.prop = "PROP";
var bar = bar$1;
console.log(bar);

foo_1();

$ rollup test.mjs -p commonjs --silent | node
[stdin]:2
var prop = foo_1.prop;
                 ^
TypeError: Cannot read property 'prop' of undefined

$ rollup test.mjs -p commonjs --no-treeshake.unknownGlobalSideEffects --no-treeshake.propertyReadSideEffects --silent | node
[stdin]:2
var prop = foo_1.prop;
                 ^
TypeError: Cannot read property 'prop' of undefined

The old version of rollup cited above also doesn't work in this case:

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

$ node_modules/.bin/rollup test.mjs -p commonjs --silent | node
[stdin]:2
var prop = foo_1.prop;
                 ^
TypeError: Cannot read property 'prop' of undefined

fwiw, esbuild can handle require cycles:

$ esbuild --version
0.12.16

$ esbuild test.mjs --bundle --minify | node
PROP
BAR
FOO

[kzc]

the commonjs plugin does not wrap commonjs modules into function calls, maintaining a registry to resolve require calls. Instead, it tries to rewrite them to ESM directly. This works surprisingly well except when there are circular require calls in the code.

To your point - but it also occurs without require cycles:

$ cat start.mjs 
console.log("start.js begin");
import "./a.js";
console.log("start.js end");

$ cat a.js
console.log("a.js begin");
require("./b.js");
console.log("a.js end");

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

Expected:

$ node start.mjs 
a.js begin
b.js
a.js end
start.js begin
start.js end

Actual:

$ rollup start.mjs -p node-resolve,commonjs --silent | node
b.js
a.js begin
a.js end
start.js begin
start.js end

It's interesting that the rollup commonjs require ordering hasn't been noticed before. It must not be an issue with many projects.

[szdailei]

@kzc I have added --treeshake --no-treeshake.unknownGlobalSideEffects --no-treeshake.propertyReadSideEffects and upgrade to rollup 2.54.0, it works.

[kzc]

It's not a general solution for require cycles, but it does work for bundling glob.

--treeshake is implicit and not needed. I specified that only to differentiate it from an earlier comment.

It's possible that --no-treeshake.unknownGlobalSideEffects is not necessary since glob_1 was a var that was declared later in the bundle:

$ rollup -v
rollup v2.54.0

$ rollup glob-repro.mjs --no-treeshake.propertyReadSideEffects -p node-resolve,commonjs --silent | node --input-type module
node_modules/glob/common.js node_modules/glob/glob.js node_modules/glob/sync.js

[szdailei]

@kzc You are right again. Only --no-treeshake.propertyReadSideEffects is required. --treeshake and --no-treeshake.unknownGlobalSideEffects are not necessary.

[lukastaegert]

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.