Fix live bindings and this of external CommonJS modules

packages/core/integration-tests/test/integration/formats/commonjs-live-externals/a.js

@@ -0,0 +1,5 @@
+const external = require('external');
+
+output(external.foo);
+external.setFoo(2);
+output(external.foo);

packages/core/integration-tests/test/integration/formats/commonjs-live-externals/package.json

@@ -0,0 +1,8 @@
+{
+  "main": "dist/test.js",
+  "targets": {
+  	"main": {
+	  "includeNodeModules": false
+	}
+  }
+}

packages/core/integration-tests/test/output-formats.js

@@ -128,8 +128,6 @@ describe('output formats', function() {
         path.join(__dirname, '/integration/formats/commonjs-external/named.js'),
       );
 
-      let dist = await outputFS.readFile(b.getBundles()[0].filePath, 'utf8');
-      assert(/var {add: \s*\$.+?\$add\s*} = require\("lodash"\)/.test(dist));
       assert.equal((await run(b)).bar, 3);
     });
 
@@ -141,17 +139,6 @@ describe('output formats', function() {
         ),
       );
 
-      let dist = await outputFS.readFile(b.getBundles()[0].filePath, 'utf8');
-      assert(
-        /var {assign: \s*\$.+?\$assign\s*} = require\("lodash\/fp"\)/.test(
-          dist,
-        ),
-      );
-      let match = dist.match(
-        /var {\s*assign:\s*(.*)\s*} = require\("lodash"\)/,
-      );
-      assert(match);
-      assert.notEqual(match[1], 'assign');
       assert.equal((await run(b)).bar, true);
     });
 
@@ -177,7 +164,7 @@ describe('output formats', function() {
       );
 
       let dist = await outputFS.readFile(b.getBundles()[0].filePath, 'utf8');
-      assert(dist.includes('= $parcel$interopDefault(require("lodash"))'));
+      assert(dist.includes('$parcel$interopDefault'));
       assert.equal((await run(b)).bar, 3);
     });
 
@@ -204,8 +191,6 @@ describe('output formats', function() {
         ),
       );
 
-      let dist = await outputFS.readFile(b.getBundles()[0].filePath, 'utf8');
-      assert(dist.includes('= require("lodash").add'));
       assert.equal((await run(b)).bar, 3);
     });
 
@@ -217,9 +202,6 @@ describe('output formats', function() {
         ),
       );
 
-      let dist = await outputFS.readFile(b.getBundles()[0].filePath, 'utf8');
-      assert(dist.includes('= require("lodash/fp").assign;'));
-      assert(dist.includes('= require("lodash").assign;'));
       assert.equal((await run(b)).bar, true);
     });
 
@@ -231,10 +213,6 @@ describe('output formats', function() {
         ),
       );
 
-      let dist = await outputFS.readFile(b.getBundles()[0].filePath, 'utf8');
-      assert(dist.includes('= require("lodash")'));
-      assert(dist.includes('= temp.add'));
-      assert(dist.includes('= temp.subtract'));
       assert.equal((await run(b)).bar, 2);
     });
 
@@ -246,8 +224,7 @@ describe('output formats', function() {
         ),
       );
 
-      let dist = await outputFS.readFile(b.getBundles()[0].filePath, 'utf8');
-      assert(dist.includes('= require("lodash").add'));
+      assert.equal((await run(b, {require})).bar, 3);
     });
 
     it('should support commonjs output with old node without destructuring (multiple)', async function() {
@@ -258,10 +235,7 @@ describe('output formats', function() {
         ),
       );
 
-      let dist = await outputFS.readFile(b.getBundles()[0].filePath, 'utf8');
-      assert(dist.includes('= require("lodash")'));
-      assert(dist.includes('= temp.add'));
-      assert(dist.includes('= temp.subtract'));
+      assert.equal((await run(b, {require})).bar, 2);
     });
 
     it('should support importing sibling bundles in library mode', async function() {
@@ -547,6 +521,32 @@ describe('output formats', function() {
         ),
       );
     });
+
+    it('should support live binding of external modules', async function() {
+      let b = await bundle(
+        path.join(
+          __dirname,
+          '/integration/formats/commonjs-live-externals/a.js',
+        ),
+      );
+
+      let external = {
+        foo: 1,
+        setFoo(f) {
+          this.foo = f;
+        },
+      };
+
+      let out = [];
+      await run(b, {
+        require: () => external,
+        output(o) {
+          out.push(o);
+        },
+      });
+
+      assert.deepEqual(out, [1, 2]);
+    });
   });
 
   describe('esmodule', function() {

packages/packagers/js/src/CJSOutputFormat.js

@@ -4,19 +4,6 @@ import type {
   OutputFormat,
 } from './ScopeHoistingPackager';
 
-// List of engines that support object destructuring syntax
-const DESTRUCTURING_ENGINES = {
-  chrome: '51',
-  edge: '15',
-  firefox: '53',
-  safari: '10',
-  node: '6.5',
-  ios: '10',
-  samsung: '5',
-  opera: '38',
-  electron: '1.2',
-};
-
 export class CJSOutputFormat implements OutputFormat {
   packager: ScopeHoistingPackager;
 
@@ -29,70 +16,12 @@ export class CJSOutputFormat implements OutputFormat {
     let lines = 0;
 
     for (let [source, specifiers] of this.packager.externals) {
-      let properties = [];
-      let categories = new Set();
-      for (let [imported, symbol] of specifiers) {
-        if (imported === '*') {
-          categories.add('namespace');
-        } else if (imported === 'default') {
-          categories.add('default');
-        } else {
-          categories.add('named');
-          properties.push({
-            key: imported,
-            value: symbol,
-          });
-        }
-      }
-
-      let specifiersWildcard = specifiers.get('*');
-      let specifiersDefault = specifiers.get('default');
-
-      // Attempt to combine require calls as much as possible. Namespace, default, and named specifiers
-      // cannot be combined, so in the case where we have more than one type, assign the require() result
-      // to a variable first and then create additional variables for each specifier based on that.
-      // Otherwise, if just one category is imported, just assign and require all at once.
-      if (categories.size > 1) {
-        let name = specifiersWildcard || this.packager.getTopLevelName(source);
-        res += `var ${name} = require(${JSON.stringify(source)});\n`;
-        lines++;
-
-        if (specifiersDefault) {
-          res += `var ${specifiersDefault} = $parcel$interopDefault(${name});\n`;
-          lines++;
-          this.packager.usedHelpers.add('$parcel$interopDefault');
-        }
-
-        if (properties.length > 0) {
-          let [r, l] = this.generateDestructuringAssignment(
-            properties,
-            name,
-            true,
-          );
-
-          res += r;
-          lines += l;
-        }
-      } else if (specifiersDefault) {
-        res += `var ${specifiersDefault} = $parcel$interopDefault(require(${JSON.stringify(
-          source,
-        )}));\n`;
-        lines++;
-        this.packager.usedHelpers.add('$parcel$interopDefault');
-      } else if (specifiersWildcard) {
-        res += `var ${specifiersWildcard} = require(${JSON.stringify(
-          source,
-        )});\n`;
+      // CJS only supports the namespace symbol. This ensures that all accesses
+      // are live and the `this` binding is correct.
+      let namespace = specifiers.get('*');
+      if (namespace) {
+        res += `var ${namespace} = require(${JSON.stringify(source)});\n`;
         lines++;
-      } else if (properties.length > 0) {
-        let [r, l] = this.generateDestructuringAssignment(
-          properties,
-          `require(${JSON.stringify(source)})`,
-          false,
-        );
-
-        res += r;
-        lines += l;
       } else {
         res += `require(${JSON.stringify(source)});\n`;
         lines++;
@@ -107,47 +36,6 @@ export class CJSOutputFormat implements OutputFormat {
     return [res, lines];
   }
 
-  generateDestructuringAssignment(
-    specifiers: Array<{|key: string, value: string|}>,
-    value: string,
-    isIdentifier: boolean,
-  ): [string, number] {
-    let res = '';
-    let lines = 0;
-
-    // If destructuring is not supported, generate a series of variable declarations
-    // with member expressions for each property.
-    if (!this.packager.bundle.env.matchesEngines(DESTRUCTURING_ENGINES)) {
-      if (!isIdentifier && specifiers.length > 1) {
-        let name = this.packager.getTopLevelName('temp');
-        res += `var ${name} = ${value};\n`;
-        lines++;
-        value = name;
-      }
-
-      for (let specifier of specifiers) {
-        res += `var ${specifier.value} = ${value}.${specifier.key};\n`;
-        lines++;
-      }
-
-      return [res, lines];
-    }
-
-    let s = specifiers.map(specifier => {
-      let s = specifier.key;
-      if (specifier.value !== specifier.key) {
-        s += `: ${specifier.value}`;
-      }
-
-      return s;
-    });
-
-    res += `var {${s.join(', ')}} = ${value};\n`;
-    lines++;
-
-    return [res, lines];
-  }
-
   buildBundlePostlude(): [string, number] {
     return ['', 0];
   }

packages/packagers/js/src/ScopeHoistingPackager.js

@@ -336,6 +336,14 @@ export class ScopeHoistingPackager {
     return name + count;
   }
 
+  getPropertyAccess(obj: string, property: string): string {
+    if (IDENTIFIER_RE.test(property)) {
+      return `${obj}.${property}`;
+    }
+
+    return `${obj}[${JSON.stringify(property)}]`;
+  }
+
   visitAsset(asset: Asset): [string, ?SourceMap, number] {
     invariant(!this.seenAssets.has(asset.id), 'Already visited asset');
     this.seenAssets.add(asset.id);
@@ -569,24 +577,51 @@ ${code}
             continue;
           }
 
-          // Rename the specifier so that multiple local imports of the same imported specifier
-          // are deduplicated. We have to prefix the imported name with the bundle id so that
-          // local variables do not shadow it.
-          if (this.exportedSymbols.has(local)) {
-            renamed = local;
-          } else if (imported === 'default' || imported === '*') {
-            renamed = this.getTopLevelName(
-              `$${this.bundle.publicId}$${dep.specifier}`,
-            );
+          // For CJS output, always use a property lookup so that exports remain live.
+          // For ESM output, use named imports which are always live.
+          if (this.bundle.env.outputFormat === 'commonjs') {
+            renamed = external.get('*');
+            if (!renamed) {
+              renamed = this.getTopLevelName(
+                `$${this.bundle.publicId}$${dep.specifier}`,
+              );
+
+              external.set('*', renamed);
+            }
+
+            if (local !== '*') {
+              let replacement;
+              if (imported === '*') {
+                replacement = renamed;
+              } else if (imported === 'default') {
+                replacement = `$parcel$interopDefault(${renamed})`;
+                this.usedHelpers.add('$parcel$interopDefault');
+              } else {
+                replacement = this.getPropertyAccess(renamed, imported);
+              }
+
+              replacements.set(local, replacement);
+            }
           } else {
-            renamed = this.getTopLevelName(
-              `$${this.bundle.publicId}$${imported}`,
-            );
-          }
+            // Rename the specifier so that multiple local imports of the same imported specifier
+            // are deduplicated. We have to prefix the imported name with the bundle id so that
+            // local variables do not shadow it.
+            if (this.exportedSymbols.has(local)) {
+              renamed = local;
+            } else if (imported === 'default' || imported === '*') {
+              renamed = this.getTopLevelName(
+                `$${this.bundle.publicId}$${dep.specifier}`,
+              );
+            } else {
+              renamed = this.getTopLevelName(
+                `$${this.bundle.publicId}$${imported}`,
+              );
+            }
 
-          external.set(imported, renamed);
-          if (local !== '*') {
-            replacements.set(local, renamed);
+            external.set(imported, renamed);
+            if (local !== '*') {
+              replacements.set(local, renamed);
+            }
           }
         }
       }
@@ -759,11 +794,7 @@ ${code}
         this.usedHelpers.add('$parcel$interopDefault');
         return `(/*@__PURE__*/$parcel$interopDefault(${obj}))`;
       } else {
-        if (IDENTIFIER_RE.test(exportSymbol)) {
-          return `${obj}.${exportSymbol}`;
-        }
-
-        return `${obj}[${JSON.stringify(exportSymbol)}]`;
+        return this.getPropertyAccess(obj, exportSymbol);
       }
     } else if (!symbol) {
       invariant(false, 'Asset was skipped or not found.');