Properly elide type-only re-export statements when targeting ESM (#806)

Fixes #803 Fixes #805 When handling TypeScript, fix two issues involving statements starting with `export {`: * Re-exports need to remove the module import if all exported names are explicitly elided. * Re-exports should NOT do the implicit export elision that's needed for normal multi-exports.
alangpierce · Jul 11, 2023 · 6854479 · 6854479
1 parent 7bbfc73
commit 6854479
Show file tree

Hide file tree

Showing 3 changed files with 87 additions and 4 deletions.
diff --git a/src/transformers/CJSImportTransformer.ts b/src/transformers/CJSImportTransformer.ts
@@ -790,7 +790,7 @@ export default class CJSImportTransformer extends Transformer {
     this.tokens.removeInitialToken();
     this.tokens.removeToken();
 
-    const exportStatements = [];
+    const exportStatements: Array<string> = [];
     while (true) {
       if (this.tokens.matches1(tt.braceR)) {
         this.tokens.removeToken();

diff --git a/src/transformers/ESMImportTransformer.ts b/src/transformers/ESMImportTransformer.ts
@@ -314,8 +314,14 @@ export default class ESMImportTransformer extends Transformer {
   }
 
   /**
-   * In TypeScript, we need to remove named exports that were never declared or only declared as a
-   * type.
+   * Handle a statement with one of these forms:
+   * export {a, type b};
+   * export {c, type d} from 'foo';
+   *
+   * In both cases, any explicit type exports should be removed. In the first
+   * case, we also need to handle implicit export elision for names declared as
+   * types. In the second case, we must NOT do implicit named export elision,
+   * but we must remove the runtime import if all exports are type exports.
    */
   private processNamedExports(): boolean {
     if (!this.isTypeScriptTransformEnabled) {
@@ -324,9 +330,14 @@ export default class ESMImportTransformer extends Transformer {
     this.tokens.copyExpectedToken(tt._export);
     this.tokens.copyExpectedToken(tt.braceL);
 
+    const isReExport = this.isReExport();
+    let foundNonTypeExport = false;
     while (!this.tokens.matches1(tt.braceR)) {
       const specifierInfo = getImportExportSpecifierInfo(this.tokens);
-      if (specifierInfo.isType || this.shouldElideExportedName(specifierInfo.leftName)) {
+      if (
+        specifierInfo.isType ||
+        (!isReExport && this.shouldElideExportedName(specifierInfo.leftName))
+      ) {
         // Type export, so remove all tokens, including any comma.
         while (this.tokens.currentIndex() < specifierInfo.endIndex) {
           this.tokens.removeToken();
@@ -336,6 +347,7 @@ export default class ESMImportTransformer extends Transformer {
         }
       } else {
         // Non-type export, so copy all tokens, including any comma.
+        foundNonTypeExport = true;
         while (this.tokens.currentIndex() < specifierInfo.endIndex) {
           this.tokens.copyToken();
         }
@@ -345,9 +357,33 @@ export default class ESMImportTransformer extends Transformer {
       }
     }
     this.tokens.copyExpectedToken(tt.braceR);
+
+    if (isReExport && !foundNonTypeExport) {
+      // This is a type-only re-export, so skip evaluating the other module. Technically this
+      // leaves the statement as `export {}`, but that's ok since that's a no-op.
+      this.tokens.removeToken();
+      this.tokens.removeToken();
+      removeMaybeImportAttributes(this.tokens);
+    }
+
     return true;
   }
 
+  /**
+   * Starting at `export {`, look ahead and return `true` if this is an
+   * `export {...} from` statement and `false` if this is a plain multi-export.
+   */
+  private isReExport(): boolean {
+    let closeBraceIndex = this.tokens.currentIndex();
+    while (!this.tokens.matches1AtIndex(closeBraceIndex, tt.braceR)) {
+      closeBraceIndex++;
+    }
+    return (
+      this.tokens.matchesContextualAtIndex(closeBraceIndex + 1, ContextualKeyword._from) &&
+      this.tokens.matches1AtIndex(closeBraceIndex + 2, tt.string)
+    );
+  }
+
   /**
    * ESM elides all imports with the rule that we only elide if we see that it's
    * a type and never see it as a value. This is in contrast to CJS, which

diff --git a/test/typescript-test.ts b/test/typescript-test.ts
@@ -3736,4 +3736,51 @@ describe("typescript transform", () => {
       {transforms: ["typescript"]},
     );
   });
+
+  it("fully elides type-only re-exports", () => {
+    assertTypeScriptImportResult(
+      `
+      export {A} from 'module1';
+      export type {B} from 'module2';
+      export {type C} from 'module3';
+      export {} from 'module4';
+      export {D, type E} from 'module5';
+    `,
+      {
+        expectedCJSResult: `"use strict";${ESMODULE_PREFIX}${CREATE_NAMED_EXPORT_FROM_PREFIX}
+      var _module1 = require('module1'); _createNamedExportFrom(_module1, 'A', 'A');
+      ;
+      
+      
+      var _module5 = require('module5'); _createNamedExportFrom(_module5, 'D', 'D');
+    `,
+        expectedESMResult: `
+      export {A} from 'module1';
+      ;
+      export {};
+      export {};
+      export {D,} from 'module5';
+    `,
+      },
+    );
+  });
+
+  it("does not elide re-exports of names that happen to be type names", () => {
+    assertTypeScriptImportResult(
+      `
+      type T = number;
+      export {T} from 'module1';
+    `,
+      {
+        expectedCJSResult: `"use strict";${ESMODULE_PREFIX}${CREATE_NAMED_EXPORT_FROM_PREFIX}
+      
+      var _module1 = require('module1'); _createNamedExportFrom(_module1, 'T', 'T');
+    `,
+        expectedESMResult: `
+      
+      export {T} from 'module1';
+    `,
+      },
+    );
+  });
 });