buxlabs / amd-to-es6

convert amd to es
MIT License
35 stars 17 forks source link

AMD module with top-level conditional produces duplicate default export #113

Closed anandthakker closed 5 years ago

anandthakker commented 5 years ago

A module that executes an if statement and also sets exports.default is converted to an invalid ES6 module with two default exports.

Steps to reproduce:

Run amdtoes6 on the following AMD module:

define(["require", "exports"], function (require, exports) {
    "use strict";
    if (false) {}
    exports.default = function foo() {};
});

Expected result:

(function () {
  if (false) {}
})();
var a = function foo() {};
export {a as default};

(or something equivalent)

Actual result:

export default (function () {
  if (false) {}
})();
var a = function foo() {};
export {a as default};
emilos commented 5 years ago

hey @anandthakker, thanks for the detailed bug report. I've published v0.14.0 that includes a fix for this case. Could you please check if it helps? I'll close the issue, please feel free to reopen if anything else goes wrong.

mikehaverstock commented 5 years ago

I think this fixes the reported issue but now we just need to add one additional step to introduce another issue:

Start with

define(["require", "exports"], function (require, exports) {
    "use strict";
    if (false) {}
    var Foo = function () {};
    exports.default = Foo;
});

Actual Results

(function () {
  if (false) {}
  var Foo = function () {};
})();
var a = Foo;
export {a as default};

Notice that var Foo is declared within the IIFE so it cannot be accessed in this way.

Expected results

I'm not exactly sure what the expected results should be. Maybe:


var a;
(function () {
  if (false) {}
  var Foo = function () {};
  a = Foo
})();
export {a as default};
emilos commented 5 years ago

Thanks, makes sense, I wasn't sure what's the real life scenario here. It's probably fine to drop the iife completely in this case. I'll push an another fix tomorrow

emilos commented 5 years ago

@mikehaverstock I've pushed the other fix and released as v0.14.1. It's not ideal but it should hopefully handle this case a bit better.