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

[Request] Loose CommonJS exports #1079

Open
lukeed opened this issue Mar 29, 2021 · 4 comments
Open

[Request] Loose CommonJS exports #1079

lukeed opened this issue Mar 29, 2021 · 4 comments

Comments

@lukeed
Copy link
Contributor

lukeed commented Mar 29, 2021

Hey,

I know the CommonJS wrappers/helpers code has come up in a few discussions & that the gist of your position is that it's all needed to properly emulate ESM exports behavior.

While I certainly understand that viewpoint and that need -- and while I agree it should probably remain as the default behavior -- I think it would be worthwhile to add a "loose"/non-strict mode that produces the CJS variant using solely the module.exports = { foo, bar } form. Even though this allows the exports to be mutable, this has a few benefits:

  1. For library authors, this allows the module to be smaller -- especially if/when there's no concern of mutation.
  2. For library consumers, existing Rollup/webpack/Parcel systems can treeshake the exports. (I'm 90% sure that this is only done thru static analysis, which means esbuild's runtime helpers can't be understood.)
  3. This allows consumers/authors to incorporate mutation as part of their CJS code, which has been possible for all of Node's timeline.

Separately, this strictness could be applies to imports too.

I've noticed that importing library code (which only has CJS) into my bundle: true, format: esm, splitting: false build will include all of the CJS helper code even though the library may just have 2 simple function exports.

I was initially going to request that the exports be statically analyzed and inlined (matching Rollup's commonjs plugin behavior), but I think that's a big ask -- and I think esbuild is already capable of this if it wasn't auto-applying a glass box around any CJS code.

@evanw
Copy link
Owner

evanw commented Mar 29, 2021

I'm not sure if something like this is good for the ecosystem. CommonJS and ESM both have well-defined semantics and tools and codebases that deviate from those are causing compatibility and portability problems that are holding the ecosystem back. Using one syntax but having it mean the other syntax, especially in a non-standard tool-specific way, just seems like asking for trouble in the future to me.

CommonJS imports and exports should work fine with esbuild. If you want module.exports = { foo, bar } you can just write that code yourself and it will be passed through to the output file unmodified.

@jeasonstudio
Copy link

jeasonstudio commented Apr 12, 2021

ESBuild at least allows plugins to READ the AST tree, in this way, plugins can do static analysis for named exports itself. Or we have to use other tools (same as Rollup's commonjs plugin behavior).
@evanw

@Utsav2
Copy link

Utsav2 commented May 26, 2021

If I understand the issue correctly, this is also useful for Typescript projects that set "module": "commonjs". It would be nice to not need require and module.export calls in a Typescript codebase.

Uniformity with tsc also makes it easier to migrate an existing large project to esbuild. In our case, we saw test failures since sinon stubs quietly stopped working for exported functions that we tried to stub using import * as X; sinon.stub(X, 'foo'). The workaround suggested in #412 worked (use module.exports and require to force commonjs behavior), but it's surprising to need that with --format=cjs. I understand the rationale, but it's a little unfortunate.

@lukeed
Copy link
Contributor Author

lukeed commented Mar 8, 2022

Sorry for the long delay on a reply here 🙈

I'm not sure if something like this is good for the ecosystem.

This is the existing ecosystem. CommonJS was the sole/primary format for years (we all know this) and existed as mutable exports.foo = and module.exports = declarations. This was — and still is — the primary way CommonJS is written & delivered today. I think it's a safe bet to say that the only CJS code on npm that exists with the export-semantics of ESM is CommonJS code generated by esbuild. I mean this, specifically:

var __create = Object.create;
var __defProp = Object.defineProperty;
var __getOwnPropDesc = Object.getOwnPropertyDescriptor;
var __getOwnPropNames = Object.getOwnPropertyNames;
var __getProtoOf = Object.getPrototypeOf;
var __hasOwnProp = Object.prototype.hasOwnProperty;
var __markAsModule = (target) => __defProp(target, "__esModule", { value: true });
var __export = (target, all) => {
  __markAsModule(target);
  for (var name in all)
    __defProp(target, name, { get: all[name], enumerable: true });
};
var __reExport = (target, module2, desc) => {
  if (module2 && typeof module2 === "object" || typeof module2 === "function") {
    for (let key of __getOwnPropNames(module2))
      if (!__hasOwnProp.call(target, key) && key !== "default")
        __defProp(target, key, { get: () => module2[key], enumerable: !(desc = __getOwnPropDesc(module2, key)) || desc.enumerable });
  }
  return target;
};

// ...

__export(exports, {
  foo: () => foo,
  bar: () => bar,
  // ...
});

I am not saying that this is incorrect or bad. It's great & admirable that you've done this & included it as the default. I don't want and am not suggesting that any of this change.

Instead, this ticket is requesting that there be some type of --loose flag that, when targeting CommonJS output, does not apply esbuild's default level of ESM->CommonJS strictness. With this mode/flag enabled, it should write the "looser" exports.foo = and exports.bar = equivalent.

You can see that this is what Rollup does, for example.

Of course, Rollup offers other options (eg, strict, freeze, esModule, etc) to further control this output, but even at its most-configured, the output isn't as verbose.

CommonJS imports and exports should work fine with esbuild.

You're right – they totally do! That's not the problem though.

The current CommonJS output works great for one-off, individual cases. But when you're in a situation that require()s lots of CJS packages that happen to have been built by esbuild, all this extra wrapper/utility code adds up.

I believe it also affects other tools' abilities to read/treeshake CommonJS (which you could argue is a fool's errand because of the nature of CommonJS, but alas, some try) because common patterns are gone & the whole module has to be evaluated in order to see exports (which, again, I recognize is/was technically always true).

you can just write that code yourself

Again, this is true for an end-user scenario. This ticket is from the standpoint of a library author looking to distribute a package in ESM and CommonJS formats. Publishing a CommonJS version that includes the same 20 lines of unwanted (and often unneeded) precautionary boilerplate isn't ideal for mass consumption in other users' projects, where the same 20 lines may repeat multiple times from other pkgs.


In CLI usage, here's what the request boils down to:

$ esbuild src/index.ts --format=cjs
#=> no change, strict `__export(...)` helpers

$ esbuild src/index.ts --format=cjs --loose
#=> use `exports.foo =` and/or `module.exports =` decls

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

No branches or pull requests

4 participants