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

Allow ES Modules to export __esModule #1622

Closed
wants to merge 2 commits into from

Conversation

heypiotr
Copy link
Contributor

@heypiotr heypiotr commented Sep 20, 2021

This is my stab at fixing #1591, as described in #1591 (comment):

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?

This PR removes the "__esModule is reserved" check, and updates __export + __markAsModule to only define __esModule if it doesn't exist yet.

I think this might only be problematic if the author of the script uses the __esModule export "incorrectly" (i.e., as anything other than an ESM marker). Originally, as stated in my comment, I was hoping to have __markAsModule detect that (__esModule exists, but evaluated to anything but true) and throw a runtime error... But turns out at the time of __export, we don't yet know the value of __esModule:

$ cat MRE.js
export const __esModule = "foo"

$ echo 'console.log(require("./MRE.js"))' | ./esbuild --bundle
(() => {
  var __defProp = Object.defineProperty;
  var __hasOwnProp = Object.prototype.hasOwnProperty;
  var __markAsModule = (target) => !__hasOwnProp.call(target, "__esModule") && __defProp(target, "__esModule", { value: true });
  var __require = typeof require !== "undefined" ? require : (x) => {
    throw new Error('Dynamic require of "' + x + '" is not supported');
  };
  var __esm = (fn, res) => function __init() {
    return fn && (res = (0, fn[Object.keys(fn)[0]])(fn = 0)), res;
  };
  var __export = (target, all) => {
    for (var name in all)
      __defProp(target, name, { get: all[name], enumerable: true });
    __markAsModule(target);
  };

  // MRE.js
  var MRE_exports = {};
  __export(MRE_exports, {
    __esModule: () => __esModule
  });
  var __esModule;
  var init_MRE = __esm({
    "MRE.js"() {
      __esModule = "foo";
    }
  });

  // <stdin>
  console.log((init_MRE(), MRE_exports));
})();

You can see __export happens before init_MRE(), and it's init_MRE that actually sets the value of __esModule.

So no runtime error for now, and if we still want it, I could use a hint/ideas on how to approach that.

@vovacodes
Copy link

@evanw are there concerns preventing this PR from being merged, it helps tremendously in adoption of URL imports and Import Maps (dependencies provided by jspm.io) by allowing folks opting-in for bundling (using esbuild) while keeping the unbundled code valid in the browser. We are actively using this workflow at Framer and find it extremely compelling.

I read the whole discussion in the original issue and while the topic is extremely controversial, it doesn't look like the solution proposed by @heypiotr in this PR is hurting any runtime compatibility.

WDYT?

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 this pull request may close these issues.

2 participants