esperantojs / esperanto

DEPRECATED: An easier way to convert ES6 modules to AMD and CommonJS
http://esperantojs.org
234 stars 21 forks source link

Issue with re-assigning exported local variable after declaration. #130

Closed rwjblue closed 9 years ago

rwjblue commented 9 years ago

Input:

var foo = function() { }

if (false) {
  foo = function () { };  
}

export {
 foo 
}

Output:

define(['exports'], function (exports) {

  'use strict';

  var foo = function() { }

  if (false) {
    exports.foo = foo = function () { };  
  }

});

This causes some issues in Ember, and we have locked Esperanto down to 0.6.17 temporarily (in https://github.com/emberjs/ember.js/pull/10751).

Rich-Harris commented 9 years ago

Ah, yep, I think I see the problem. Thinking out loud: it's trying to avoid doing more exports.foo = ... than is necessary - this line is being run, because isTopLevelNode is true but shouldn't be. The problematic line is here - it's determining whether it's a top level node by seeing if it's in the top level scope, which obviously isn't the case since it's in a block. Should be an easy fix - thanks for raising it

eventualbuddha commented 9 years ago

I believe the expected output should include two exports.foo assignments, one inside the if, the other outside it.

rwjblue commented 9 years ago

@eventualbuddha - OK, I definitely defer that to y'all. My main issue is that there is no export.foo in the current output (because it is inside the if). I'll just remove the expected output....

Rich-Harris commented 9 years ago

Have opened #131, which fixes this but changes the approach slightly. Previously, esperanto would keep track of any assignments to exported bindings, so that it didn't need the final exports.foo = foo. A simpler approach in my view is to always have exports.foo = foo at the end, and not bother with changing the foo = ... into exports.foo = foo = ... inside the if block.

The one exception to this is when foo is assigned to inside a function body, since that could happen late. So for example this becomes this.

Does that sound right to everyone?

Rich-Harris commented 9 years ago

Released in 0.6.23