Repository: alangpierce/sucrase
Pull request #804 (2023-07-08 09:21:31)

fix: handle specifier as default
Add `module.exports = exports.default;` for case:
```ts
export { x as default } from './foo'
export { x as default }
export { }
export type A = string
export { type A as B }
```

alangpierce: Also, adding a little context here for myself and future readers, since I was needing to remind myself how `add-module-exports` works:

When using `enableLegacyBabel5ModuleInterop`, analogous to `babel-plugin-add-module-exports`, the expected behavior is to add `module.exports = exports.default;` (making the whole CJS module object the default export) when there is a default export but no named exports. My original implementation missed some cases, which are fixed by this PR:
* Type-only exports should *not* count as a named export for these purposes. Fixed by rearranging some cases so `hadNamedExport` isn't set to `true`, and by considering each export individually in `export {...` lines.
* Explicitly exporting the name `default` should count as a default export, not a named export, which is fixed by doing a string match on the exported name.
github-actions[bot]: ## Benchmark results
**Before this PR:** 321 thousand lines per second
**After this PR:**  322.9 thousand lines per second

**Measured change:** 0.6% faster (0.75% slower to 1.5% faster)
**Summary:** Likely no significant difference

Patch:
diff --git a/src/transformers/CJSImportTransformer.ts b/src/transformers/CJSImportTransformer.ts
index 9972da6..6e74f68 100644
--- a/src/transformers/CJSImportTransformer.ts
+++ b/src/transformers/CJSImportTransformer.ts
@@ -292,35 +292,9 @@ export default class CJSImportTransformer extends Transformer {
       }
       this.processExportDefault();
       return true;
-    }
-    this.hadNamedExport = true;
-    if (
-      this.tokens.matches2(tt._export, tt._var) ||
-      this.tokens.matches2(tt._export, tt._let) ||
-      this.tokens.matches2(tt._export, tt._const)
-    ) {
-      this.processExportVar();
-      return true;
-    } else if (
-      this.tokens.matches2(tt._export, tt._function) ||
-      // export async function
-      this.tokens.matches3(tt._export, tt.name, tt._function)
-    ) {
-      this.processExportFunction();
-      return true;
-    } else if (
-      this.tokens.matches2(tt._export, tt._class) ||
-      this.tokens.matches3(tt._export, tt._abstract, tt._class) ||
-      this.tokens.matches2(tt._export, tt.at)
-    ) {
-      this.processExportClass();
-      return true;
     } else if (this.tokens.matches2(tt._export, tt.braceL)) {
       this.processExportBindings();
       return true;
-    } else if (this.tokens.matches2(tt._export, tt.star)) {
-      this.processExportStar();
-      return true;
     } else if (
       this.tokens.matches2(tt._export, tt.name) &&
       this.tokens.matchesContextualAtIndex(this.tokens.currentIndex() + 1, ContextualKeyword._type)
@@ -357,6 +331,32 @@ export default class CJSImportTransformer extends Transformer {
         removeMaybeImportAttributes(this.tokens);
       }
       return true;
+    }
+    this.hadNamedExport = true;
+    if (
+      this.tokens.matches2(tt._export, tt._var) ||
+      this.tokens.matches2(tt._export, tt._let) ||
+      this.tokens.matches2(tt._export, tt._const)
+    ) {
+      this.processExportVar();
+      return true;
+    } else if (
+      this.tokens.matches2(tt._export, tt._function) ||
+      // export async function
+      this.tokens.matches3(tt._export, tt.name, tt._function)
+    ) {
+      this.processExportFunction();
+      return true;
+    } else if (
+      this.tokens.matches2(tt._export, tt._class) ||
+      this.tokens.matches3(tt._export, tt._abstract, tt._class) ||
+      this.tokens.matches2(tt._export, tt.at)
+    ) {
+      this.processExportClass();
+      return true;
+    } else if (this.tokens.matches2(tt._export, tt.star)) {
+      this.processExportStar();
+      return true;
     } else {
       throw new Error("Unrecognized export syntax.");
     }
@@ -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();
@@ -798,14 +798,23 @@ export default class CJSImportTransformer extends Transformer {
       }
 
       const specifierInfo = getImportExportSpecifierInfo(this.tokens);
+
       while (this.tokens.currentIndex() < specifierInfo.endIndex) {
         this.tokens.removeToken();
       }
-      if (!specifierInfo.isType && !this.shouldElideExportedIdentifier(specifierInfo.leftName)) {
-        const localName = specifierInfo.leftName;
+
+      if (!specifierInfo.isType) {
         const exportedName = specifierInfo.rightName;
-        const newLocalName = this.importProcessor.getIdentifierReplacement(localName);
-        exportStatements.push(`exports.${exportedName} = ${newLocalName || localName};`);
+        if (exportedName === "default") {
+          this.hadDefaultExport = true;
+        } else {
+          this.hadNamedExport = true;
+        }
+        if (!this.shouldElideExportedIdentifier(specifierInfo.leftName)) {
+          const localName = specifierInfo.leftName;
+          const newLocalName = this.importProcessor.getIdentifierReplacement(localName);
+          exportStatements.push(`exports.${exportedName} = ${newLocalName || localName};`);
+        }
       }
 
       if (this.tokens.matches1(tt.braceR)) {
diff --git a/src/transformers/ESMImportTransformer.ts b/src/transformers/ESMImportTransformer.ts
index 41b814e..1768366 100644
--- 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


Test patch:
diff --git a/test/imports-test.ts b/test/imports-test.ts
index 266eede..d234f74 100644
--- a/test/imports-test.ts
+++ b/test/imports-test.ts
@@ -768,6 +768,24 @@ module.exports = exports.default;
 `,
       {transforms: ["imports"], enableLegacyBabel5ModuleInterop: true},
     );
+
+    assertResult(
+      `
+      export { x as default } from './foo'
+      export {}
+      export type A = string
+      export { type A as B }
+    `,
+      `"use strict";${ESMODULE_PREFIX}${CREATE_NAMED_EXPORT_FROM_PREFIX}
+      var _foo = require('./foo'); _createNamedExportFrom(_foo, 'default', 'x');
+      
+      
+      
+    
+module.exports = exports.default;
+`,
+      {transforms: ["imports", "typescript"], enableLegacyBabel5ModuleInterop: true},
+    );
   });
 
   it("does not add module exports suffix when there is a named export", () => {
diff --git a/test/typescript-test.ts b/test/typescript-test.ts
index 8c7447c..144f12e 100644
--- 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';
+    `,
+      },
+    );
+  });
 });