v0.14.27 breaks bundle with outdent module

[fisker]

Should be caused by #2059

This from outdent module will override the module.exports.

Reproduce repo https://github.com/fisker/esbuild-0-14-27-issue

[evanw]

This is a problem with that package, not with esbuild. You are not supposed to use both CommonJS module exports and ES module exports in the same file. When you reference module in an ES module, the module reference is treated as a global reference and may collide with other code. If you bundle with --log-level=debug you'll see esbuild calling out the problem:

● [DEBUG] The CommonJS "module" variable is treated as a global variable in an ECMAScript module and may not work as expected

    node_modules/outdent/lib-module/index.js:165:8:
      165 │         module.exports = defaultOutdent;
          ╵         ~~~~~~

  This file is considered to be an ECMAScript module because of the "export" keyword here:

    node_modules/outdent/lib-module/index.js:160:0:
      160 │ export { defaultOutdent as outdent };
          ╵ ~~~~~~

[fisker]

I agree that module should be treat as a global variable, so why don't we replace it with globalThis.module or just throw an error instead of this debug info?

[evanw]

Most warnings for files inside node_modules are debug messages instead of warnings. One reason is because the warning isn't immediately actionable, since the person seeing the warning has no way of fixing the problem (only the package author can fix the problem). Another reason is because every time I add a warning that triggers for files inside node_modules, someone ends up filing an issue telling me to remove it. So I have been making them debug messages instead of warnings.

[evanw]

I'm going to close this issue since I consider this case to basically be undefined behavior, and I think esbuild's output here is reasonable. The correct fix here is for the package author to not do this.

[fisker]

why don't we replace it with globalThis.module

What's your opinion on this?

[evanw]

I don't think that should be done because globalThis is a recent addition to JavaScript, and polyfills for globalThis that work in all JavaScript environments are surprisingly complicated. It seems like a lot of work and maintenance overhead to support something that no one should be doing anyway.

[fisker]

How about rename it to something like __globalModule__? So this can throw on runtime, or if someone really mean to use the global variable module, it can be fixed by add getter/setter to globalThis before load the bundle. Sorry for those silly suggestions, but I really hate it silently change the module exports.