Import issue with circular re-exports

[DavidZbarsky-at]

Hi, I found another behavior change when upgrading esbuild from 0.13.5 to latest. To repro:

• clone https://github.com/DavidZbarsky-at/esbuild-imports-repro-2
• run npm install && ./test.sh. Should see "done!" printed
• Upgrade esbuild to latest and repeat
• Should see "TypeError: Cannot read property 'f' of undefined"

This is the smallest reduction I could get, everything else I tried removing doesn't trigger the change anymore. The code is a bit convoluted so perhaps it's tripping up the analyzer?

[AlCalzone]

I recently upgraded from 0.14.1 to 0.14.9 and observed the same. In particular, these imports:
https://github.com/zwave-js/node-zwave-js/blob/6ab67d68fa8dd6eb6b2a9be9a21b32aac15c4632/packages/zwave-js/src/lib/commandclass/API.ts#L19
https://github.com/zwave-js/node-zwave-js/blob/6ab67d68fa8dd6eb6b2a9be9a21b32aac15c4632/packages/zwave-js/src/lib/commandclass/CommandClass.ts#L35

cause CCAPI to be undefined in this line:
https://github.com/zwave-js/node-zwave-js/blob/6ab67d68fa8dd6eb6b2a9be9a21b32aac15c4632/packages/zwave-js/src/lib/commandclass/CommandClass.ts#L1519

[mlrawlings]

We're seeing circular export issues in our project after upgrading as well. I did a test of the intermediate versions and the last working version is 0.14.3.

[DavidZbarsky-at]

@evanw in case you had a chance to take a look, did the repro steps work for you?

[indutny-signal]

I second this, this is our repro: https://github.com/indutny-signal/esbuild-bug-repro

Running node main.js would print "hello world", while running node build/main.js would error.

[indutny-signal]

As @mlrawlings said the issue was introduced between 0.14.3 and 0.14.4 (even though it wasn't published), and it comes down to this generated code:

var a_exports = {};
__export(a_exports, {
  a1: () => a1,
  a2: () => a2
});
...
module.exports = __toCommonJS(b_exports);

vs previously generated

__export(exports, {
  a1: () => a1,
  a2: () => a2
});

I'll see if I can come up with a fix for this.

---

FWIW, the main difference between the top and bottom snippets is that exports is not updated in the top case and so circular requires see empty exports object, and the culprit is obviously #1849

[indutny-signal]

Now that I look more into it, it is reproducible with just two files:

import b from './b.js';

b.method();

and b.js

export default {
  method() {
    console.log('hello world');
  }
}

If compiled with --format=cjs and --platform=node - a.js won't run.

[hyrious]

FYI, Node.js has documented circular require() calls:
https://nodejs.org/api/modules.html#modules_cycles

In short, when circular require happens, any conflicting modules' incomplete exports will be returned. That's exactly what mentioned above because 0.14.5 moved module.exports = to the end of the file, which happens after circular require() taking place, results in the importer receiving {}.

The ideal solution for keeping esm circular import behavior correct in cjs context, should be always exporting names at the beginning of the file (like what previously did). It does not mean you won't get bugs any more, you can still reproduce the behavior like node.js docs said. In other words, there's no problem if all your input code is in esm.

Maybe related: #1737.

[indutny-signal]

@hyrious exactly, but also as mentioned here default exports are currently completely broken when running under node.

[indutny-signal]

In all honesty, I think it might be worth it to revert #1849 for the time being until better solution appears. What do you think @evanw ?

[hyrious]

In all honesty, I think it might be worth it to revert 1849 for the time being until better solution appears. What do you think evanw ?

I'm afraid not. That PR makes esbuild to align with other front-end bundlers' behaviors, which is adopted for a long time in the ecosystem. In fact, the node behavior is the new one.

[indutny-signal]

I'm afraid not. That PR makes esbuild to align with other front-end bundlers' behaviors, which is adopted for a long time in the ecosystem. In fact, the node behavior is the new one.

I'll check how babel/swc does it today, but I'm pretty sure that when output format is set to commonjs the expectation is that default export works 😁 Circular dependencies or not, you cannot currently import a default when compiling with esbuild without bundling.

[indutny-signal]

Here's the output from swc when compiling to commonJS:

b.js

Object.defineProperty(exports, "__esModule", {
    value: true
});
exports.default = void 0;
var _default = {
    method: function method() {
        console.log('hello world');
    }
};
exports.default = _default;

a.js

var _bJs = _interopRequireDefault(require("./b.js"));
function _interopRequireDefault(obj) {
    return obj && obj.__esModule ? obj : {
        default: obj
    };
}
_bJs.default.method();

[indutny-signal]

Babel does it similarly.

[indutny]

I take my comments back about default exports. I see how esbuild behaves now and more importantly why it is done this way.

The circular imports on other hand still needs to be fixed. I think the fix could be as easy as dropping:

var exports = {};

and just using exports provided by node when --platform=node is used.

[indutny]

Opened a PR to fix this #2059

[indutny-signal]

Just a quick heads up. Y'all should try updating to 0.14.27 which was just released!