Fix hoisting of "var" variables on wrapped modules

src/scope-hoisting/hoist.js

@@ -135,6 +135,36 @@ module.exports = {
         ) {
           scope.push({id: exportsIdentifier, init: t.objectExpression([])});
         }
+
+        // Move all "var" variables to the top-level to prevent out of order definitions when wrapped.
+        for (let name in scope.bindings) {
+          let binding = scope.getBinding(name);
+
+          if (binding.path.scope !== scope && binding.kind === 'var') {
+            let {parentPath} = binding.path;
+
+            if (!parentPath.removed) {
+              if (parentPath.node.declarations.length) {
+                binding.path.getStatementParent().insertBefore(
+                  parentPath.node.declarations
+                    .map(decl => {
+                      binding.scope.removeBinding(decl.id.name);
+                      scope.push({id: decl.id});
+
+                      return decl.init
+                        ? t.assignmentExpression('=', decl.id, decl.init)
+                        : null;
+                    })
+                    .filter(decl => decl !== null)
+                );
+              }
+
+              parentPath.remove();
+            }
+
+            binding.path.remove();
+          }
+        }
       }
 
       path.stop();
@@ -280,8 +310,9 @@ module.exports = {
     }
 
     if (t.isIdentifier(callee, {name: 'require'})) {
+      let source = args[0].value;
       // Ignore require calls that were ignored earlier.
-      if (!asset.dependencies.has(args[0].value)) {
+      if (!asset.dependencies.has(source)) {
         return;
       }
 
@@ -296,9 +327,11 @@ module.exports = {
           p.isSequenceExpression()
       );
       if (!parent.isProgram() || bail) {
-        asset.dependencies.get(args[0].value).shouldWrap = true;
+        asset.dependencies.get(source).shouldWrap = true;
       }
 
+      asset.cacheData.imports['$require$' + source] = [source, '*'];
+
       // Generate a variable name based on the current asset id and the module name to require.
       // This will be replaced by the final variable name of the resolved asset in the packager.
       path.replaceWith(

test/integration/scope-hoisting/commonjs/hoist-vars/a.js

@@ -0,0 +1,3 @@
+const foo = require('./b')
+
+output = foo()

test/integration/scope-hoisting/commonjs/hoist-vars/b.js

@@ -0,0 +1,13 @@
+if(process.env.NODE_ENV === 'test') {
+    var c = require('./c')()
+
+    for(var i = 0, {length} = c, out = ''; i < length; i++) {
+        out += c[i]
+    }
+
+    module.exports = function() {
+        if(c != out) throw new Error()
+
+        return out
+    }
+}

test/integration/scope-hoisting/commonjs/hoist-vars/c.js

@@ -0,0 +1,12 @@
+module.exports = doFoo
+
+function doFoo() {
+  return foo
+}
+
+if(process.env.NODE_ENV === 'test') {
+  var foo = 'bar'
+}
+else {
+  foo = 'foo'
+}

test/integration/scope-hoisting/commonjs/side-effects-false/a.js

@@ -0,0 +1 @@
+output = require('bar').foo(3)

test/scope-hoisting.js

@@ -816,5 +816,24 @@ describe('scope hoisting', function() {
       let output = await run(b);
       assert.deepEqual(output, 3);
     });
+
+    it('should correctly hoist "var" on wrapped modules', async function() {
+      let b = await bundle(
+        __dirname + '/integration/scope-hoisting/commonjs/hoist-vars/a.js'
+      );
+
+      let output = await run(b);
+      assert.deepEqual(output, 'bar');
+    });
+
+    it('should support sideEffects: false', async function() {
+      let b = await bundle(
+        __dirname +
+          '/integration/scope-hoisting/commonjs/side-effects-false/a.js'
+      );
+
+      let output = await run(b);
+      assert.deepEqual(output, 9);
+    });
   });
 });