Add missing imports for external dependencies in skipped assets

parcel-bundler · Jul 12, 2022 · 8bb4ce8 · 8bb4ce8
1 parent 99cf505
commit 8bb4ce8
Show file tree

Hide file tree

Showing 4 changed files with 89 additions and 59 deletions.
diff --git a/packages/core/integration-tests/test/integration/formats/esm-external/child.js b/packages/core/integration-tests/test/integration/formats/esm-external/child.js
@@ -0,0 +1 @@
+export {add} from 'lodash';
diff --git a/packages/core/integration-tests/test/integration/formats/esm-external/re-export-child.js b/packages/core/integration-tests/test/integration/formats/esm-external/re-export-child.js
@@ -0,0 +1 @@
+export {add} from './child';
diff --git a/packages/core/integration-tests/test/output-formats.js b/packages/core/integration-tests/test/output-formats.js
@@ -725,6 +725,24 @@ describe('output formats', function () {
       );
     });
 
+    it('should support esmodule output with external modules (re-export child)', async function () {
+      let b = await bundle(
+        path.join(
+          __dirname,
+          '/integration/formats/esm-external/re-export-child.js',
+        ),
+      );
+
+      await assertESMExports(
+        b,
+        3,
+        {
+          lodash: () => lodash,
+        },
+        ns => ns.add(1, 2),
+      );
+    });
+
     it('should support importing sibling bundles in library mode', async function () {
       let b = await bundle(
         path.join(__dirname, '/integration/formats/esm-siblings/a.js'),

diff --git a/packages/packagers/js/src/ScopeHoistingPackager.js b/packages/packagers/js/src/ScopeHoistingPackager.js
@@ -400,7 +400,15 @@ export class ScopeHoistingPackager {
       for (let dep of deps) {
         let resolved = this.bundleGraph.getResolvedAsset(dep, this.bundle);
         let skipped = this.bundleGraph.isDependencySkipped(dep);
-        if (!resolved || skipped) {
+        if (skipped) {
+          continue;
+        }
+
+        if (!resolved) {
+          if (!dep.isOptional) {
+            this.addExternal(dep);
+          }
+
           continue;
         }
 
@@ -605,62 +613,7 @@ ${code}
         !dep.isOptional &&
         !this.bundleGraph.isDependencySkipped(dep)
       ) {
-        let external = this.addExternal(dep);
-        for (let [imported, {local}] of dep.symbols) {
-          // If already imported, just add the already renamed variable to the mapping.
-          let renamed = external.get(imported);
-          if (renamed && local !== '*') {
-            replacements.set(local, renamed);
-            continue;
-          }
-
-          // 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 {
-            // 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);
-            }
-          }
-        }
+        this.addExternal(dep, replacements);
       }
 
       if (!resolved) {
@@ -712,7 +665,10 @@ ${code}
     return [depMap, replacements];
   }
 
-  addExternal(dep: Dependency): Map<string, string> {
+  addExternal(
+    dep: Dependency,
+    replacements?: Map<string, string>,
+  ): Map<string, string> {
     if (this.bundle.env.outputFormat === 'global') {
       throw new ThrowableDiagnostic({
         diagnostic: {
@@ -742,7 +698,61 @@ ${code}
       this.externals.set(dep.specifier, external);
     }
 
-    return external;
+    for (let [imported, {local}] of dep.symbols) {
+      // If already imported, just add the already renamed variable to the mapping.
+      let renamed = external.get(imported);
+      if (renamed && local !== '*' && replacements) {
+        replacements.set(local, renamed);
+        continue;
+      }
+
+      // 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 !== '*' && replacements) {
+          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 {
+        // 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) {
+          replacements.set(local, renamed);
+        }
+      }
+    }
   }
 
   getSymbolResolution(