evanw / esbuild

An extremely fast bundler for the web
https://esbuild.github.io/
MIT License
37.98k stars 1.14k forks source link

circular dependency not being detected/reported #3808

Closed viktomas closed 3 months ago

viktomas commented 3 months ago

I'm not sure if this issue can be fixed by esbuild. I want to raise it for awareness, if not anything else. Also, a huge thank you, Evan, for building and maintaining this project 🙇

A plain node with ES modules seems to be able to detect this issue at module loading time whilst esbuild produces a bundle.

The issue

The issue is, that circular dependency is not detected at bundling time. This leads to a bug in the bundled code that's hard to understand.

// a.js
import { b } from './b';
export const a = 'aloha';
console.log('from a', b);
// b.js
import { c } from './c';
export const b = 'bye';
console.log('from b', c);
// c.js
import { a } from './a';
export const c = 'ciao';
console.log('from c', a);
// index.js
import { a } from './a';
console.log(a);

compiling this code with

esbuild index.ts --sourcemap --platform=node --target=node21 --bundle --outfile=out.js

produces this code

"use strict";

// c.ts
var c = "ciao";
console.log("from c", a);

// b.ts
var b = "bye";
console.log("from b", c);

// a.ts
var a = "aloha";
console.log("from a", b);

// index.ts
console.log(a);
//# sourceMappingURL=out.js.map

which when run produces this output

from c undefined
from b ciao
from a bye
aloha

Again, I'm not sure that this can be solved by esbuild without implementing some more complex static analysis like what eslint-plugin-import/no-cycle rule does.

Comparison with plain node

I tried this same scenario in node 21.7. I tried both CommojJS and ES modules, and both produced useful errors or warnings.

I read this response https://github.com/evanw/esbuild/issues/1582#issuecomment-915401785 on an existing closed issue that was making distinction between runtime and "compile-time" error (as in module loading time). I think that at least for ES modules, this is a compile time error.

ES modules

//a.js
import { b } from "./b.js";
export const a = "aloha";
console.log("from a", b);
//b.js
import { c } from "./c.js";
export const b = "bye";
console.log("from b", c);
//c.js
import { a } from './a.js';
export const c = 'ciao';
console.log('from c', a);
//index.js
import { a } from "./a.js";
console.log(a);

running node index.js produces what is IMO a compile-time error (sidenote: you have to specify type: module in package.json)

node index.js
file:///Users/tomas/workspace/tmp/test-modules-esm/c.js:5
console.log('from c', a);
                      ^

ReferenceError: Cannot access 'a' before initialization
    at file:///Users/tomas/workspace/tmp/test-modules-esm/c.js:5:23
    at ModuleJob.run (node:internal/modules/esm/module_job:222:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:323:24)
    at async loadESM (node:internal/process/esm_loader:28:7)
    at async handleMainPromise (node:internal/modules/run_main:120:12)

Node.js v21.7.3

CommonJS

// a.js
const b = require("./b.js");
module.exports = "aloha";
console.log("from a", b);
// b.js
const c = require("./c.js");
module.exports = "bye";
console.log("from b", c);
// c.js
const a = require("./a.js");
module.exports = "ciao";
console.log("from c", a);
// index.js
const a = require("./a.js");
console.log(a);

running node index.js runs the code but produces a warning

node index.js
from c {}
from b ciao
from a bye
aloha
(node:68187) Warning: Accessing non-existent property 'Symbol(nodejs.util.inspect.custom)' of module exports inside circular dependency
(Use `node --trace-warnings ...` to show where the warning was created)
(node:68187) Warning: Accessing non-existent property 'constructor' of module exports inside circular dependency
(node:68187) Warning: Accessing non-existent property 'Symbol(Symbol.toStringTag)' of module exports inside circular dependency
hyrious commented 3 months ago

Similar issue raised before: https://github.com/evanw/esbuild/issues/2056

  1. esbuild transforms all top-level const to var to avoid a TDZ performance issue. This is a trade-off to have better performance rather than keeping native behavior.

  2. Rollup does report circular dependency warnings, however that does not always mean a runtime error. If you write your code in a way that all side-effects (mainly initialization statements) are happen in a constructor / init function and make sure they are called after all dependencies being loaded, this issue disappear even if there is circular dependencies.

evanw commented 3 months ago

I'm already aware of esbuild's behavior regarding this case. There is a FAQ entry on why esbuild does things this way: https://esbuild.github.io/faq/#top-level-var

viktomas commented 3 months ago

Ok, got you, thanks @hyrious and @evanw for your response 🙏 It makes sense; I'll close this issue.

To anyone who's reading this thread. I strongly recommend using the eslint-plugin-import/docs/rules/no-cycle eslint rule as a prevention of some of the issues that can be caused by the top-level-var design choice 👍