Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The export name "__esModule" is reserved and cannot be used #1591

Closed
heypiotr opened this issue Sep 10, 2021 · 20 comments · Fixed by #1849
Closed

The export name "__esModule" is reserved and cannot be used #1591

heypiotr opened this issue Sep 10, 2021 · 20 comments · Fixed by #1849

Comments

@heypiotr
Copy link
Contributor

heypiotr commented Sep 10, 2021

Recently, I've been attempting to bundle modules from jspm.io (via a custom esbuild plugin which implements imports from URLs and import maps), and I've been running into this error a lot:

error: The export name "__esModule" is reserved and cannot be used (it's needed as an export marker when converting ES module syntax to CommonJS)

Here's an example module where I've run into this for the first time: https://jspm.dev/npm:react-calendly@2.2.1!cjs

It all boils down to something like this:

// MRE.js

const exports = {}
Object.defineProperty(exports, "__esModule", { value: true })
export default exports

const __esModule = exports.__esModule
export { __esModule }

It's that named __esModule export that upsets esbuild -- if I comment it out, it builds fine.

The way I understood it from reading esnext/es6-module-transpiler#85, the magic __esModule export was introduced so that when you transpile an ESM module to CJS, and do const foo = require("esm-foo-transpiled-to-cjs"), it'll know that you really mean const foo = require("esm-foo-transpiled-to-cjs").default. However, I belive JSPM's doing things the other way, i.e., it transpiles CJS modules to ESM. So I'm not sure if their usage of the __esModule export is correct.

We've had a brief conversation about this with @guybedford on the JSPM Discord, but we decided to move it here. I'm curious, what's the story behind this "__esModule is reserved" check/error in esbuild?

@guybedford
Copy link
Contributor

On the JSPM side, the issue here is that JSPM will convert CJS into ESM, and in doing so follow the Node.js semantics for what exports to expose.

Per the Node.js ESM CJS wrapper semantics, __esModule is a valid ESM export of a CJS module wrapper, and hence is reflected in the converted ESM of the CJS module, which then doesn't build with esbuild.

@heypiotr
Copy link
Contributor Author

heypiotr commented Sep 10, 2021

On the JSPM side, the issue here is that JSPM will convert CJS into ESM, and in doing so follow the Node.js semantics for what exports to expose.

Ah, I think I understand now. So e.g. react-calendly's dist/index.js does Object.defineProperty(exports, '__esModule', { value: true }). JSPM then iterates over all properties on exports, and converts them to ESM exports. So it sees __esModule, and treats it as any other export, hence generating the "offending" __esModule export. Which of course is not really "offending" by any standards.

But esbuild complains because if it now were to take that ESM with the__esModule export, and convert it to CJS, the export will clash with the __esModule magic marker which is added per esnext/es6-module-transpiler#85. 'Cause esbuild can't know that the __esModule export already is the magic marker from the original CJS module.

(It's like a game of telephone, CJS => ESM => CJS 😄)

But in my case, I'm esbuilding with format=esm, so it doesn't need to convert anything to CJS, so maybe it doesn't need to complain about the reserved export?

@hyrious
Copy link

hyrious commented Sep 11, 2021

It seems a bug in jspm.dev, it chooses the cjs version over the esm one, and wrongly exports __esModule.
You may instead use their generator service which seems choosing the right file:
https://ga.jspm.io/npm:react-calendly@2.2.1/dist/index.es.js

@evanw
Copy link
Owner

evanw commented Sep 13, 2021

On the JSPM side, the issue here is that JSPM will convert CJS into ESM, and in doing so follow the Node.js semantics for what exports to expose.

Ah, I think I understand now. So e.g. react-calendly's dist/index.js does Object.defineProperty(exports, '__esModule', { value: true }). JSPM then iterates over all properties on exports, and converts them to ESM exports. So it sees __esModule, and treats it as any other export, hence generating the "offending" __esModule export. Which of course is not really "offending" by any standards.

But esbuild complains because if it now were to take that ESM with the__esModule export, and convert it to CJS, the export will clash with the __esModule magic marker which is added per esnext/es6-module-transpiler#85. 'Cause esbuild can't know that the __esModule export already is the magic marker from the original CJS module.

(It's like a game of telephone, CJS => ESM => CJS 😄)

This all sounds correct to me.

But in my case, I'm esbuilding with format=esm, so it doesn't need to convert anything to CJS, so maybe it doesn't need to complain about the reserved export?

You can still get into a situation where this is needed because esbuild supports requiring ESM (which is not something that node supports). For example, consider your example file MRE.js:

// MRE.js

const exports = {}
Object.defineProperty(exports, "__esModule", { value:true })
export default exports

const __esModule = exports.__esModule
export { __esModule }

If I import that with require() and bundle that with a version of esbuild with this anti-__esModule check removed, this will happen:

$ echo 'console.log(require("./MRE.js"))' | esbuild --bundle | node
[stdin]:13
      __defProp(target, name, { get: all[name], enumerable: true });
      ^

TypeError: Cannot redefine property: __esModule
    at defineProperty (<anonymous>)
    at __export ([stdin]:13:7)
    at [stdin]:18:3
    at [stdin]:34:3
    at Script.runInThisContext (node:vm:129:12)
    at Object.runInThisContext (node:vm:305:38)
    at node:internal/process/execution:81:19
    at [stdin]-wrapper:6:22
    at evalScript (node:internal/process/execution:80:60)
    at node:internal/main/eval_stdin:29:5

This is why the __esModule check is there.

Trying to only do this check in the conditions when it actually matters is pretty complicated since it happens in a completely different part of the compile pipeline after linking and tree shaking, and by that point it may be pretty non-obvious why it happened so reporting a reasonable error that explains what happened and why isn't straightforward. And I don't want to have a very obscure error appear only sometimes seemingly at random with no explanation. I think if it's not possible to explain exactly why it happened, it should either always appear or never appear so it at least doesn't feel like a bug.

I suppose I could just remove the error and allow this code to be bundled but to potentially fail at run-time. But that doesn't feel great either since it still seems like "a very obscure error that appears only sometimes seemingly at random with no explanation."

See also: #1338

@guybedford
Copy link
Contributor

@evanw I was not aware ES build was adding __esModule to all ES modules internally. In the discussion in #1338 I thought it turned out that this was not the case but it seems my initial assumption in that PR was right.

If ES build is adding __esModule to all ESM modules internally, that's not really a spec-compatible mechanism.

Instead I would suggest using the spec marker - mod[Symbol.toStringTag] = 'Module' and using internal "real esm brand" checks (m && m[Symbol.toStringTag] === 'Module' which works in all modules environments) based on this instead, which will also properly capture real ES modules in ES module environments when wanting to check them.

More generally, this issue is an example of a valid ES module per the ES specification not bundling in esbuild. __esModule is a non-standard bundler convention and as much as we can this should not inhibit real JS semantics (which was the concern I raised in #1338).

@guybedford
Copy link
Contributor

An alternative can be to provide a different wrapper to require() than is provided to import() for the solution to the #1338 problem. I believe Webpack does something like this. //cc @sokra

@guybedford
Copy link
Contributor

This also seems to come down to a misunderstanding of what __esModule is for. __esModule is designed for CommonJS modules that want to be "seen" as ES modules. Real ES modules shouldn't need to pretend to be ES modules by also including __esModule - that is up to user code to ensure they are brand checked properly.

@sokra
Copy link

sokra commented Sep 13, 2021

Real ES modules shouldn't need to pretend to be ES modules by also including __esModule - that is up to user code to ensure they are brand checked properly.

It makes sense that a require(esm) returns an object with __esModule to be compatible with transpiled esm e.g. generated by babel.

@guybedford
Copy link
Contributor

@sokra thanks for sharing your perspective on this - can you clarify though for a normal ES module case like await import(esm) if you are adding __esModule to all ESM modules? Or is the ESM object provided to CommonJS require a different ESM object to the normal module namespace?

@sokra
Copy link

sokra commented Sep 13, 2021

webpack adds __esModule to all exposed esm namespace objects. I don't think it's needed for import(esm), but doesn't seem to hurt. In webpack it's a shared helper function which adds toStringTag and __esModule.
At least it's consistent. You also want to ensure await import(esm) === require(esm).

@guybedford
Copy link
Contributor

__esModule was never supposed to spill into real ESM bundling semantics. This is very unfortunate to hear on both accounts.

@guybedford
Copy link
Contributor

RollupJS as an ESM-first bundler will at least ensure you do not have to consider __esModule if not actually dealing with CommonJS, as there is no __esModule logic in core, it is only reflected in the CommonJS plugin.

The concern is that ESM-first code without any CJS is now getting interference from __esModule. Interop shouldn't be harming future semantics...

@guybedford
Copy link
Contributor

This really is a hill I'd die on if I had the power to in this situation - require(esm) should return real ESM, not an "amended" ESM.

require(esm) where the esm is external will otherwise simply not work, this pattern is highly restrictive.

There are three require cases - require(faux esm), require(esm) and require(cjs). require(esm) being anything other than what looks and acts like a real ES module namespace (even if synthetic), without alterations including __esModule is clearly the correct behaviour.

Anything else is harming the future ecosystem.

@sokra
Copy link

sokra commented Sep 14, 2021

require(esm) where the esm is external will otherwise simply not work, this pattern is highly restrictive.

That's illegal anyway. In Node.js you can't require(esm) and the browser doesn't know about require(). And it will only break if consumed by a transpiled ESM require(), that doesn't sound like a huge restriction. Technically we could place a faux ESM adapter in between.

require(esm) being anything other than what looks and acts like a real ES module namespace (even if synthetic), without alterations including __esModule is clearly the correct behaviour.

Anything else is harming the future ecosystem.

Technically there is no spec for CJS-ESM-interop, so we would have to define "correct" first. An additional __esModule property on the object might be unexpected, but it's improving compatibility. I don't think it's very harmful: It's not enumerable, so you only see it when explicitly accessed.

It increases the bundle size by Object.defineProperty(e,'__esModule',{value:true}) for the whole bundle, and only when ESM namespace objects in non-statically analyse-able way.

Considering the amount of babel-transpiled CommonJS in npm, I would say it might be more harmful to not provide a __esModule property.

Yes, it would probably better if __esModule would have never be used at all, but I don't know an alternative babel could have chosen at that time. I guess we have to live with it as long there is CommonJS-ESM-interop.

@guybedford
Copy link
Contributor

@evanw perhaps you could clarify with this error:

error: The export name "__esModule" is reserved and cannot be used (it's needed as an export marker when converting ES module syntax to CommonJS)

what exactly is the bug that is trying to be avoided here?

I think building ES modules that export __esModule that is true should be fine, it seems it is only arbitrary edge cases that unexpected things might happen.

For what it is worth, the CommonJS -> ESM converter that is effectively unsupported here is this: https://github.com/jspm/babel-plugin-transform-cjs-dew where retaining the __esModule export (which Node.js supports on real ES module objects as well) is critical to an ESM-based-ecosystem compatibility for legacy code.

@sokra
Copy link

sokra commented Sep 14, 2021

I added require() syntax to https://sokra.github.io/interop-test/ with the following results for require(esm):

(Note the first 4 test cases are normal cases and the others are edge cases)

x = require() babel

babel-js
esbuild rollup webpack

webpack-js
webpack4

webpack4-mjs
named-export-esm { [__esModule], named } 🟡 { [__esModule], named: [G] } 🟡 { [__esModule], named: [G] } 🟡 { [__esModule], named: [G], [Module] } { [__esModule], named: [G], [Module] }
default-export-esm { [__esModule], default } { [__esModule], default: [G] } { [__esModule], default: [G] } { [__esModule], default: [G], [Module] } { [__esModule], default, [Module] }
named-and-default-export-esm { [__esModule], named, default } { [__esModule], default: [G], named: [G] } { [__esModule], named: [G], default: [G] } { [__esModule], named: [G], default: [G], [Module] } { [__esModule], named: [G], default, [Module] }
order-esm { [__esModule], b, a, c } 🟡 { [__esModule], a: [G: 'a'], b: [G: 'b'], c: [G: 'c'] } 💎 { [__esModule], b: [G: 'b'], a: [G: 'a'], c: [G: 'c'] } 💎 { [__esModule], b: [G: 'b'], a: [G: 'a'], c: [G: 'c'], [Module] } { [__esModule], b: [G: 'b'], a: [G: 'a'], c: [G: 'c'], [Module] }
default-export-esModule-esm-reexport { [__esModule] } { [__esModule] } { [__esModule], default: [G], __moduleExports: [G] { [__esModule], default } } 💎 { [__esModule], [Module] } { [__esModule], [Module] }
named-and-default-export-esModule-esm-reexport { [__esModule], named: [G] } { [__esModule], named: [G] } { [__esModule], named: [G], default: [G], __moduleExports: [G] { [__esModule], named, default } } 💎 { [__esModule], named: [G], [Module] } { [__esModule], named: [G], [Module] }
named-and-default-export-esm-reexport { [__esModule], named: [G] } { [__esModule], named: [G] } { [__esModule], named: [G], default: [G], __moduleExports: [G] { named, default } } 💎 { [__esModule], named: [G], [Module] } { [__esModule], named: [G], [Module] }
single-string-export-esm-reexport { '0': [G: 's'], '1': [G: 'i'], '2': [G: 'n'], '3': [G: 'g'], '4': [G: 'l'], '5': [G: 'e'], [__esModule] } 🟡 { [__esModule] } 💎 { '0': [G: 's'], '1': [G: 'i'], '2': [G: 'n'], '3': [G: 'g'], '4': [G: 'l'], '5': [G: 'e'], [__esModule], __moduleExports: [G: 'single'] } 💎 { '0': [G: 's'], '1': [G: 'i'], '2': [G: 'n'], '3': [G: 'g'], '4': [G: 'l'], '5': [G: 'e'], [__esModule], [Module] } { '0': [G: 's'], '1': [G: 'i'], '2': [G: 'n'], '3': [G: 'g'], '4': [G: 'l'], '5': [G: 'e'], [__esModule], [Module] }
  • I omitted Node.js as it always errors with require(esm) is not allowed.
  • __esModule is more reliable than Symbol.toStringTag for detection since only webpack sets Symbol.toStringTag at all.
  • rollup sometimes leaks a __moduleExports property (when reexporting cjs via esm)
  • babel namespace objects are mutable from outside
  • only esbuild has correct order of exports in the namespace object
  • see https://github.com/sokra/interop-test/tree/main/modules for the test cases

@heypiotr
Copy link
Contributor Author

heypiotr commented Sep 15, 2021

If I import that with require() and bundle that with a version of esbuild with this anti-__esModule check removed, this will happen:

$ echo 'console.log(require("./MRE.js"))' | esbuild --bundle | node
[stdin]:13
      __defProp(target, name, { get: all[name], enumerable: true });
      ^

TypeError: Cannot redefine property: __esModule

Ah, I see: (I replaced __esModule with _esModule for testing, so that MRE.js bundles successfully)

(() => {
  var __defProp = Object.defineProperty;
  var __markAsModule = (target) => __defProp(target, "__esModule", { value: true });
  var __esm = (fn, res) => function __init() {
    return fn && (res = (0, fn[Object.keys(fn)[0]])(fn = 0)), res;
  };
  var __export = (target, all) => {
    __markAsModule(target);
    for (var name in all)
      __defProp(target, name, { get: all[name], enumerable: true });
  };

  // MRE.js
  var MRE_exports = {};
  __export(MRE_exports, {
    _esModule: () => _esModule,
    default: () => MRE_default
  });

  <snip>

So in __export , __markAsModule will define the __esModule marker, and then the __esModule export will clash with that in the for loop.

Would it be at all feasible to make __export + __markAsModule a bit smarter => if there's already a correct __esModule marker in all, just let it be? If it's incorrect, I guess the only option would be to throw a runtime error, which would be a slight regression compared to the current compile-time error. But maybe that's good enough?

I guess doing that "smart" check at compile time would be harder or impossible, because esbuild only parses the script and doesn't evaluate it, so it doesn't actually know what's the value of the __esModule export?

@blixt
Copy link

blixt commented Nov 26, 2021

I just wanted to see if there's any new perspectives to this issue now that it's been dormant for a while? To give some practical context on why we (Framer) care a lot about this:

Framer has introduced a feature called "Handshake" which lets users build a visual component which gets turned into React under the hood. They can then import its ESM module via a URL, which is now supported in Webpack 5 (and by extension, Next.js).

Now Framer is building two more features:

  1. The ability to construct an entire site visually and host it on a domain, which for performance reasons gets bundled with esbuild
  2. "Instant NPM" – basically a way to use NPM without installation, allowing copy/pasting of these components across different Framer projects and have them appear (load & evaluate) instantly without delay – this is done using import maps on top of the JSPM CDN (ga.jspm.io)

These two features together (bundling with esbuild and loading NPM packages via ga.jspm.io) very often fail on the error described in this issue. However, without this check they would (to the best of my knowledge) bundle and evaluate correctly. While I see there are a lot of edge cases to consider here, I also get the feeling that maybe we're not seeing the forest for the trees.

There must be a way for us to allow the legitimate use cases above while handling most edge cases (i.e. sacrifice handling some edge cases in favor of allowing legitimate use cases). I thought Piotr's PR was a good step in that direction – @evanw is there any reason we can't merge it?

@blixt
Copy link

blixt commented Dec 13, 2021

Does anyone in this thread have ideas for how to work around this issue with an esbuild plugin? I'm also looking into creating a forked version of esbuild now, but trying to find ways to not diverge from the official channel too much.

@evanw
Copy link
Owner

evanw commented Dec 14, 2021

I finally started looking into this. One reason why I haven't tackled it yet is that it's a huge interop mess with several entangled issues. I'd like to get them all fixed together so I have a holistic picture of the changes instead of addressing issues individually.

I have started putting together a set of test cases for what I think should be fixed in this cluster of issues: https://github.com/evanw/bundler-esm-cjs-tests. I'll also start working on getting esbuild to pass these test cases. I think this will unfortunately bloat the injected shim code even more, but that's likely unavoidable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants