Repository: alangpierce/sucrase
Pull request #807 (2023-07-11 19:05:55)

Fix some additional enableLegacyBabel5ModuleInterop cases
Follow-up from #804.
* When exporting `{T as default}`, treat it as a type-only export that doesn't count as a real default export.
* When explicitly doing `export default T`, treat it as a type-only export.
* Treat `export enum` as a named export.

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/alangpierce/sucrase/pull/807?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Alan+Pierce) Report
> Merging [#807](https://app.codecov.io/gh/alangpierce/sucrase/pull/807?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Alan+Pierce) (b3f6ea2) into [main](https://app.codecov.io/gh/alangpierce/sucrase/commit/631f64ca67837c8e5985808b521608c0f3a9d8d2?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Alan+Pierce) (631f64c) will **increase** coverage by `0.05%`.
> The diff coverage is `100.00%`.

```diff
@@            Coverage Diff             @@
##             main     #807      +/-   ##
==========================================
+ Coverage   88.53%   88.59%   +0.05%     
==========================================
  Files          55       56       +1     
  Lines        6047     6058      +11     
  Branches     1434     1435       +1     
==========================================
+ Hits         5354     5367      +13     
+ Misses        424      423       -1     
+ Partials      269      268       -1     
```


| [Impacted Files](https://app.codecov.io/gh/alangpierce/sucrase/pull/807?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Alan+Pierce) | Coverage Δ | |
|---|---|---|
| [src/transformers/CJSImportTransformer.ts](https://app.codecov.io/gh/alangpierce/sucrase/pull/807?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Alan+Pierce#diff-c3JjL3RyYW5zZm9ybWVycy9DSlNJbXBvcnRUcmFuc2Zvcm1lci50cw==) | `90.09% <100.00%> (+0.16%)` | :arrow_up: |
| [src/transformers/ESMImportTransformer.ts](https://app.codecov.io/gh/alangpierce/sucrase/pull/807?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Alan+Pierce#diff-c3JjL3RyYW5zZm9ybWVycy9FU01JbXBvcnRUcmFuc2Zvcm1lci50cw==) | `95.40% <100.00%> (-0.11%)` | :arrow_down: |
| [src/util/isExportFrom.ts](https://app.codecov.io/gh/alangpierce/sucrase/pull/807?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Alan+Pierce#diff-c3JjL3V0aWwvaXNFeHBvcnRGcm9tLnRz) | `100.00% <100.00%> (ø)` | |

... and [1 file with indirect coverage changes](https://app.codecov.io/gh/alangpierce/sucrase/pull/807/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Alan+Pierce)

:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Alan+Pierce)

github-actions[bot]: ## Benchmark results
**Before this PR:** 347 thousand lines per second
**After this PR:**  346.6 thousand lines per second

**Measured change:** 0.11% slower (2.75% slower to 1.01% faster)
**Summary:** Likely no significant difference

Patch:
diff --git a/src/transformers/CJSImportTransformer.ts b/src/transformers/CJSImportTransformer.ts
index 6e74f68..c69f430 100644
--- a/src/transformers/CJSImportTransformer.ts
+++ b/src/transformers/CJSImportTransformer.ts
@@ -11,6 +11,7 @@ import getDeclarationInfo, {
   EMPTY_DECLARATION_INFO,
 } from "../util/getDeclarationInfo";
 import getImportExportSpecifierInfo from "../util/getImportExportSpecifierInfo";
+import isExportFrom from "../util/isExportFrom";
 import {removeMaybeImportAttributes} from "../util/removeMaybeImportAttributes";
 import shouldElideDefaultExport from "../util/shouldElideDefaultExport";
 import type ReactHotLoaderTransformer from "./ReactHotLoaderTransformer";
@@ -280,12 +281,13 @@ export default class CJSImportTransformer extends Transformer {
       this.tokens.matches2(tt._export, tt._enum) ||
       this.tokens.matches3(tt._export, tt._const, tt._enum)
     ) {
+      this.hadNamedExport = true;
       // Let the TypeScript transform handle it.
       return false;
     }
     if (this.tokens.matches2(tt._export, tt._default)) {
-      this.hadDefaultExport = true;
       if (this.tokens.matches3(tt._export, tt._default, tt._enum)) {
+        this.hadDefaultExport = true;
         // Flow export default enums need some special handling, so handle them
         // in that tranform rather than this one.
         return false;
@@ -488,6 +490,7 @@ export default class CJSImportTransformer extends Transformer {
   }
 
   private processExportDefault(): void {
+    let exportedRuntimeValue = true;
     if (
       this.tokens.matches4(tt._export, tt._default, tt._function, tt.name) ||
       // export default async function
@@ -523,6 +526,7 @@ export default class CJSImportTransformer extends Transformer {
       // If the exported value is just an identifier and should be elided by TypeScript
       // rules, then remove it entirely. It will always have the form `export default e`,
       // where `e` is an identifier.
+      exportedRuntimeValue = false;
       this.tokens.removeInitialToken();
       this.tokens.removeToken();
       this.tokens.removeToken();
@@ -540,6 +544,9 @@ export default class CJSImportTransformer extends Transformer {
       this.tokens.copyToken();
       this.tokens.appendCode(" =");
     }
+    if (exportedRuntimeValue) {
+      this.hadDefaultExport = true;
+    }
   }
 
   private copyDecorators(): void {
@@ -790,6 +797,8 @@ export default class CJSImportTransformer extends Transformer {
     this.tokens.removeInitialToken();
     this.tokens.removeToken();
 
+    const isReExport = isExportFrom(this.tokens);
+
     const exportStatements: Array<string> = [];
     while (true) {
       if (this.tokens.matches1(tt.braceR)) {
@@ -803,18 +812,19 @@ export default class CJSImportTransformer extends Transformer {
         this.tokens.removeToken();
       }
 
-      if (!specifierInfo.isType) {
+      const shouldRemoveExport =
+        specifierInfo.isType ||
+        (!isReExport && this.shouldElideExportedIdentifier(specifierInfo.leftName));
+      if (!shouldRemoveExport) {
         const exportedName = specifierInfo.rightName;
         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};`);
-        }
+        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 1768366..cc541e7 100644
--- a/src/transformers/ESMImportTransformer.ts
+++ b/src/transformers/ESMImportTransformer.ts
@@ -11,6 +11,7 @@ import getDeclarationInfo, {
 } from "../util/getDeclarationInfo";
 import getImportExportSpecifierInfo from "../util/getImportExportSpecifierInfo";
 import {getNonTypeIdentifiers} from "../util/getNonTypeIdentifiers";
+import isExportFrom from "../util/isExportFrom";
 import {removeMaybeImportAttributes} from "../util/removeMaybeImportAttributes";
 import shouldElideDefaultExport from "../util/shouldElideDefaultExport";
 import type ReactHotLoaderTransformer from "./ReactHotLoaderTransformer";
@@ -330,7 +331,7 @@ export default class ESMImportTransformer extends Transformer {
     this.tokens.copyExpectedToken(tt._export);
     this.tokens.copyExpectedToken(tt.braceL);
 
-    const isReExport = this.isReExport();
+    const isReExport = isExportFrom(this.tokens);
     let foundNonTypeExport = false;
     while (!this.tokens.matches1(tt.braceR)) {
       const specifierInfo = getImportExportSpecifierInfo(this.tokens);
@@ -369,21 +370,6 @@ export default class ESMImportTransformer extends Transformer {
     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/src/util/isExportFrom.ts b/src/util/isExportFrom.ts
new file mode 100644
index 0000000..00307ef
--- /dev/null
+++ b/src/util/isExportFrom.ts
@@ -0,0 +1,18 @@
+import {ContextualKeyword} from "../parser/tokenizer/keywords";
+import {TokenType as tt} from "../parser/tokenizer/types";
+import type TokenProcessor from "../TokenProcessor";
+
+/**
+ * Starting at `export {`, look ahead and return `true` if this is an
+ * `export {...} from` statement and `false` if this is a plain multi-export.
+ */
+export default function isExportFrom(tokens: TokenProcessor): boolean {
+  let closeBraceIndex = tokens.currentIndex();
+  while (!tokens.matches1AtIndex(closeBraceIndex, tt.braceR)) {
+    closeBraceIndex++;
+  }
+  return (
+    tokens.matchesContextualAtIndex(closeBraceIndex + 1, ContextualKeyword._from) &&
+    tokens.matches1AtIndex(closeBraceIndex + 2, tt.string)
+  );
+}


Test patch:
diff --git a/test/imports-test.ts b/test/imports-test.ts
index d234f74..ae738ec 100644
--- a/test/imports-test.ts
+++ b/test/imports-test.ts
@@ -768,7 +768,9 @@ module.exports = exports.default;
 `,
       {transforms: ["imports"], enableLegacyBabel5ModuleInterop: true},
     );
+  });
 
+  it("properly treats `as default` as a default export when adding module exports suffix", () => {
     assertResult(
       `
       export { x as default } from './foo'
@@ -788,6 +790,34 @@ module.exports = exports.default;
     );
   });
 
+  it("properly ignores regular default-exported types when deciding to add module exports suffix", () => {
+    assertResult(
+      `
+      type T = number;
+      export default T;
+    `,
+      `"use strict";${ESMODULE_PREFIX}
+      
+      ;
+    `,
+      {transforms: ["imports", "typescript"], enableLegacyBabel5ModuleInterop: true},
+    );
+  });
+
+  it("properly ignores `{T as default}` when deciding to add module exports suffix", () => {
+    assertResult(
+      `
+      type T = number;
+      export { T as default };
+    `,
+      `"use strict";${ESMODULE_PREFIX}
+      
+      
+    `,
+      {transforms: ["imports", "typescript"], enableLegacyBabel5ModuleInterop: true},
+    );
+  });
+
   it("does not add module exports suffix when there is a named export", () => {
     assertResult(
       `
@@ -802,6 +832,20 @@ module.exports = exports.default;
     );
   });
 
+  it("properly treats exported TS enums as a named export when adding module exports suffix", () => {
+    assertResult(
+      `
+      export enum E {}
+      export default 4;
+    `,
+      `"use strict";${ESMODULE_PREFIX}
+      var E; (function (E) {})(E || (exports.E = E = {}));
+      exports. default = 4;
+    `,
+      {transforms: ["imports", "typescript"], enableLegacyBabel5ModuleInterop: true},
+    );
+  });
+
   it("does not modify object keys matching import names", () => {
     assertResult(
       `