Sometimes having non-top-level things named the same as top-level things causes imports to be named incorrectly in bundles

[evs-chris]

I haven't been able to reproduce this in simple test cases, but I have with the Ractive source. I'm not sure exactly why at this point, but (after fiddling about with merging _Mustache.js into one file) Section$bubble was being included in the bundle unchanged, but when it was imported in _Section.js, it was being mangled to bubble__Section$bubble. The ostensible reason for that is that a few other modules have a declared bubble variable/function in them somewhere (fireEvents.js, in my case). Modifying otherModulesDeclare to only look at top-level identifiers clears that up. Is there a reason to look at all of the identifiers instead of only those in the "global" namespace? No PR because I'm not entirely sure what is or should be going on, and there's probably a more appropriate fix :)

index d55fc90..2673d1c 100644
--- a/src/bundler/combine/populateIdentifierReplacements.js
+++ b/src/bundler/combine/populateIdentifierReplacements.js
@@ -14,17 +14,17 @@ export default function populateIdentifierReplacements ( bundle ) {
        // then figure out what identifiers need to be created
        // for default exports
        bundle.modules.forEach( mod => {
-               var prefix, x;
+               var prefix, x, other;

                prefix = bundle.uniqueNames[ mod.id ];

                if ( x = mod.defaultExport ) {
                        if ( x.hasDeclaration && x.name ) {
-                               mod.identifierReplacements.default = hasOwnProp.call( conflicts, x.name ) || otherModulesDeclare( mod, prefix ) ?
+                               mod.identifierReplacements.default = hasOwnProp.call( conflicts, x.name ) || ( other = otherModulesDeclare( mod, prefix ) ) ?
                                        prefix + '__' + x.name :
                                        x.name;
                        } else {
-                               mod.identifierReplacements.default = hasOwnProp.call( conflicts, prefix ) || otherModulesDeclare( mod, prefix ) ?
+                               mod.identifierReplacements.default = hasOwnProp.call( conflicts, prefix ) || ( other = otherModulesDeclare( mod, prefix ) ) ?
                                        prefix + '__default' :
                                        prefix;
                        }
@@ -127,9 +127,9 @@ export default function populateIdentifierReplacements ( bundle ) {
                                continue;
                        }

-                       if ( hasOwnProp.call( otherMod.ast._declared, replacement ) ) {
-                               return true;
+                       if ( hasOwnProp.call( otherMod.ast._topLevelNames, replacement ) ) {
+                               return otherMod;
                        }
                }
        }
-}
\ No newline at end of file
+}

[Rich-Harris]

It is necessary to check all declarations to prevent imports being shadowed (#18), but off the top of my head I'm not sure if that applies here.

ast._topLevelNames is an array, not an object - changing the code like so...

- if ( hasOwnProp.call( otherMod.ast._declared, replacement ) ) {
+ if ( ~otherMod.ast._topLevelNames.indexOf( replacement ) ) {

...doesn't break any tests, though I can't guarantee it doesn't cause a variant of #18 to reappear - will need to check. Does Ractive build correctly if you make the change above?

[evs-chris]

ast._topLevelNames is an array

D'oh!

Does Ractive build correctly if you make the change above?

No, it still gets some imports prefixed incorrectly because there are some top-level identifiers that overlap too. Perhaps I'm approaching this from the wrong side... if the export was renamed to match its identifierReplacement, that should fix it, but I haven't yet stumbled over where to achieve that.

[Rich-Harris]

Hmm. Is there a branch somewhere I can clone? Would be keen to take a look and maybe try to extract a test case

[evs-chris]

I pushed a temp branch here https://github.com/evs-chris/esperanto/tree/temp

It's basically a fresh checkout of esperanto with some mild abuse to populateIdentifierReplacements. Then, src from a fresh checkout of Ractive was copied to test/bundle/input/29 and the generator tweaked to pick up the _config.js for grabbing the entry point. Running the generator produces a broken bundle for UMD. Search for Section$bubble.

[Rich-Harris]

@evs-chris am kicking myself...

- ... || otherModulesDeclare( mod, mod.name ) ?
+ ... || otherModulesDeclare( mod, x.name ) ?

Have published 0.6.1 with this fix.

[evs-chris]

👍 Ractive builds again!

Kicking myself is three eighths of my exercise regimen.