Fix TS import elision for JSX fragments and custom pragmas (#403)

Fixes #395 Previously, we added the `React` identifier as non-type identifier when seeing any JSX name, but this meant that fragment syntax didn't keep that import. We also shouldn't hard-code React, and instead use whatever the pragma base values are.
alangpierce · Jan 13, 2019 · 83f1178 · 83f1178
1 parent b7b9ad1
commit 83f1178
Show file tree

Hide file tree

Showing 9 changed files with 115 additions and 32 deletions.
diff --git a/src/CJSImportProcessor.ts b/src/CJSImportProcessor.ts
@@ -1,3 +1,4 @@
+import {Options} from "./index";
 import NameManager from "./NameManager";
 import {isDeclaration} from "./parser/tokenizer";
 import {ContextualKeyword} from "./parser/tokenizer/keywords";
@@ -40,6 +41,7 @@ export default class CJSImportProcessor {
     readonly nameManager: NameManager,
     readonly tokens: TokenProcessor,
     readonly enableLegacyTypeScriptModuleInterop: boolean,
+    readonly options: Options,
   ) {}
 
   getPrefixCode(): string {
@@ -97,7 +99,7 @@ export default class CJSImportProcessor {
    * bare imports.
    */
   pruneTypeOnlyImports(): void {
-    const nonTypeIdentifiers = getNonTypeIdentifiers(this.tokens);
+    const nonTypeIdentifiers = getNonTypeIdentifiers(this.tokens, this.options);
     for (const [path, importInfo] of this.importInfoByPath.entries()) {
       if (
         importInfo.hasBareImport ||

diff --git a/src/index.ts b/src/index.ts
@@ -135,6 +135,7 @@ function getSucraseContext(code: string, options: Options): SucraseContext {
       nameManager,
       tokenProcessor,
       enableLegacyTypeScriptModuleInterop,
+      options,
     );
     importProcessor.preprocessTokens();
     // We need to mark shadowed globals after processing imports so we know that the globals are,

diff --git a/src/transformers/ESMImportTransformer.ts b/src/transformers/ESMImportTransformer.ts
@@ -1,3 +1,4 @@
+import {Options} from "../index";
 import NameManager from "../NameManager";
 import {ContextualKeyword} from "../parser/tokenizer/keywords";
 import {TokenType as tt} from "../parser/tokenizer/types";
@@ -18,10 +19,11 @@ export default class ESMImportTransformer extends Transformer {
     readonly nameManager: NameManager,
     readonly reactHotLoaderTransformer: ReactHotLoaderTransformer | null,
     readonly isTypeScriptTransformEnabled: boolean,
+    options: Options,
   ) {
     super();
     this.nonTypeIdentifiers = isTypeScriptTransformEnabled
-      ? getNonTypeIdentifiers(tokens)
+      ? getNonTypeIdentifiers(tokens, options)
       : new Set();
   }
 

diff --git a/src/transformers/JSXTransformer.ts b/src/transformers/JSXTransformer.ts
@@ -4,6 +4,7 @@ import NameManager from "../NameManager";
 import XHTMLEntities from "../parser/plugins/jsx/xhtml";
 import {TokenType as tt} from "../parser/tokenizer/types";
 import TokenProcessor from "../TokenProcessor";
+import getJSXPragmaInfo, {JSXPragmaInfo} from "../util/getJSXPragmaInfo";
 import RootTransformer from "./RootTransformer";
 import Transformer from "./Transformer";
 
@@ -14,10 +15,7 @@ export default class JSXTransformer extends Transformer {
   lastLineNumber: number = 1;
   lastIndex: number = 0;
   filenameVarName: string | null = null;
-  readonly jsxPragmaBase: string;
-  readonly jsxPragmaSuffix: string;
-  readonly jsxFragmentPragmaBase: string;
-  readonly jsxFragmentPragmaSuffix: string;
+  readonly jsxPragmaInfo: JSXPragmaInfo;
 
   constructor(
     readonly rootTransformer: RootTransformer,
@@ -27,20 +25,7 @@ export default class JSXTransformer extends Transformer {
     readonly options: Options,
   ) {
     super();
-    [this.jsxPragmaBase, this.jsxPragmaSuffix] = this.splitPragma(
-      options.jsxPragma || "React.createElement",
-    );
-    [this.jsxFragmentPragmaBase, this.jsxFragmentPragmaSuffix] = this.splitPragma(
-      options.jsxFragmentPragma || "React.Fragment",
-    );
-  }
-
-  private splitPragma(pragma: string): [string, string] {
-    let dotIndex = pragma.indexOf(".");
-    if (dotIndex === -1) {
-      dotIndex = pragma.length;
-    }
-    return [pragma.slice(0, dotIndex), pragma.slice(dotIndex)];
+    this.jsxPragmaInfo = getJSXPragmaInfo(options);
   }
 
   process(): boolean {
@@ -219,21 +204,23 @@ export default class JSXTransformer extends Transformer {
   }
 
   processJSXTag(): void {
-    const {jsxPragmaBase, jsxPragmaSuffix, jsxFragmentPragmaBase, jsxFragmentPragmaSuffix} = this;
+    const {jsxPragmaInfo} = this;
     const resolvedPragmaBaseName = this.importProcessor
-      ? this.importProcessor.getIdentifierReplacement(jsxPragmaBase) || jsxPragmaBase
-      : jsxPragmaBase;
+      ? this.importProcessor.getIdentifierReplacement(jsxPragmaInfo.base) || jsxPragmaInfo.base
+      : jsxPragmaInfo.base;
     const firstTokenStart = this.tokens.currentToken().start;
     // First tag is always jsxTagStart.
-    this.tokens.replaceToken(`${resolvedPragmaBaseName}${jsxPragmaSuffix}(`);
+    this.tokens.replaceToken(`${resolvedPragmaBaseName}${jsxPragmaInfo.suffix}(`);
 
     if (this.tokens.matches1(tt.jsxTagEnd)) {
       // Fragment syntax.
       const resolvedFragmentPragmaBaseName = this.importProcessor
-        ? this.importProcessor.getIdentifierReplacement(jsxFragmentPragmaBase) ||
-          jsxFragmentPragmaBase
-        : jsxFragmentPragmaBase;
-      this.tokens.replaceToken(`${resolvedFragmentPragmaBaseName}${jsxFragmentPragmaSuffix}, null`);
+        ? this.importProcessor.getIdentifierReplacement(jsxPragmaInfo.fragmentBase) ||
+          jsxPragmaInfo.fragmentBase
+        : jsxPragmaInfo.fragmentBase;
+      this.tokens.replaceToken(
+        `${resolvedFragmentPragmaBaseName}${jsxPragmaInfo.fragmentSuffix}, null`,
+      );
       // Tag with children.
       this.processChildren();
       while (!this.tokens.matches1(tt.jsxTagEnd)) {

diff --git a/src/transformers/RootTransformer.ts b/src/transformers/RootTransformer.ts
@@ -78,6 +78,7 @@ export default class RootTransformer {
           this.nameManager,
           reactHotLoaderTransformer,
           transforms.includes("typescript"),
+          options,
         ),
       );
     }

diff --git a/src/util/getJSXPragmaInfo.ts b/src/util/getJSXPragmaInfo.ts
@@ -0,0 +1,22 @@
+import {Options} from "../index";
+
+export interface JSXPragmaInfo {
+  base: string;
+  suffix: string;
+  fragmentBase: string;
+  fragmentSuffix: string;
+}
+
+export default function getJSXPragmaInfo(options: Options): JSXPragmaInfo {
+  const [base, suffix] = splitPragma(options.jsxPragma || "React.createElement");
+  const [fragmentBase, fragmentSuffix] = splitPragma(options.jsxFragmentPragma || "React.Fragment");
+  return {base, suffix, fragmentBase, fragmentSuffix};
+}
+
+function splitPragma(pragma: string): [string, string] {
+  let dotIndex = pragma.indexOf(".");
+  if (dotIndex === -1) {
+    dotIndex = pragma.length;
+  }
+  return [pragma.slice(0, dotIndex), pragma.slice(dotIndex)];
+}
diff --git a/src/util/getNonTypeIdentifiers.ts b/src/util/getNonTypeIdentifiers.ts
@@ -1,9 +1,12 @@
+import {Options} from "../index";
 import {IdentifierRole} from "../parser/tokenizer";
 import {TokenType, TokenType as tt} from "../parser/tokenizer/types";
 import TokenProcessor from "../TokenProcessor";
 import {startsWithLowerCase} from "../transformers/JSXTransformer";
+import getJSXPragmaInfo from "./getJSXPragmaInfo";
 
-export function getNonTypeIdentifiers(tokens: TokenProcessor): Set<string> {
+export function getNonTypeIdentifiers(tokens: TokenProcessor, options: Options): Set<string> {
+  const jsxPragmaInfo = getJSXPragmaInfo(options);
   const nonTypeIdentifiers: Set<string> = new Set();
   for (let i = 0; i < tokens.tokens.length; i++) {
     const token = tokens.tokens[i];
@@ -17,8 +20,18 @@ export function getNonTypeIdentifiers(tokens: TokenProcessor): Set<string> {
     ) {
       nonTypeIdentifiers.add(tokens.identifierNameForToken(token));
     }
+    if (token.type === tt.jsxTagStart) {
+      nonTypeIdentifiers.add(jsxPragmaInfo.base);
+    }
+    if (
+      token.type === tt.jsxTagStart &&
+      i + 1 < tokens.tokens.length &&
+      tokens.tokens[i + 1].type === tt.jsxTagEnd
+    ) {
+      nonTypeIdentifiers.add(jsxPragmaInfo.base);
+      nonTypeIdentifiers.add(jsxPragmaInfo.fragmentBase);
+    }
     if (token.type === tt.jsxName && token.identifierRole === IdentifierRole.Access) {
-      nonTypeIdentifiers.add("React");
       const identifierName = tokens.identifierNameForToken(token);
       // Lower-case single-component tag names like "div" don't count.
       if (!startsWithLowerCase(identifierName) || tokens.tokens[i + 1].type === TokenType.dot) {

diff --git a/test/identifyShadowedGlobals-test.ts b/test/identifyShadowedGlobals-test.ts
@@ -10,7 +10,9 @@ function assertHasShadowedGlobals(code: string, expected: boolean): void {
   const tokenProcessor = new TokenProcessor(code, file.tokens, false);
   const nameManager = new NameManager(tokenProcessor);
   nameManager.preprocessNames();
-  const importProcessor = new CJSImportProcessor(nameManager, tokenProcessor, false);
+  const importProcessor = new CJSImportProcessor(nameManager, tokenProcessor, false, {
+    transforms: [],
+  });
   importProcessor.preprocessTokens();
   assert.strictEqual(
     hasShadowedGlobals(tokenProcessor, importProcessor.getGlobalNames()),

diff --git a/test/typescript-test.ts b/test/typescript-test.ts
@@ -4,7 +4,7 @@ import {
   IMPORT_WILDCARD_PREFIX,
   JSX_PREFIX,
 } from "./prefixes";
-import {assertResult} from "./util";
+import {assertResult, devProps} from "./util";
 
 function assertTypeScriptResult(code: string, expectedResult: string): void {
   assertResult(code, expectedResult, {transforms: ["jsx", "imports", "typescript"]});
@@ -1048,6 +1048,59 @@ describe("typescript transform", () => {
     );
   });
 
+  it("does not elide a React import when the file contains a JSX fragment", () => {
+    assertTypeScriptResult(
+      `
+      import React from 'react';
+      function render(): JSX.Element {
+        return <>Hello</>;
+      }
+    `,
+      `"use strict";${IMPORT_DEFAULT_PREFIX}
+      var _react = require('react'); var _react2 = _interopRequireDefault(_react);
+      function render() {
+        return _react2.default.createElement(_react2.default.Fragment, null, "Hello");
+      }
+    `,
+    );
+  });
+
+  it("correctly takes JSX pragmas into account avoiding JSX import elision", () => {
+    assertResult(
+      `
+      import {A, B, C, D} from 'foo';
+      function render(): JSX.Element {
+        return <>Hello</>;
+      }
+    `,
+      `
+      import {A, C,} from 'foo';
+      function render() {
+        return A.B(C.D, null, "Hello");
+      }
+    `,
+      {transforms: ["typescript", "jsx"], jsxPragma: "A.B", jsxFragmentPragma: "C.D"},
+    );
+  });
+
+  it("correctly takes JSX pragmas into account avoiding JSX import elision with fragments unused", () => {
+    assertResult(
+      `
+      import {A, B, C, D} from 'foo';
+      function render(): JSX.Element {
+        return <span />;
+      }
+    `,
+      `const _jsxFileName = "";
+      import {A,} from 'foo';
+      function render() {
+        return A.B('span', {${devProps(4)}} );
+      }
+    `,
+      {transforms: ["typescript", "jsx"], jsxPragma: "A.B", jsxFragmentPragma: "C.D"},
+    );
+  });
+
   it("handles TypeScript exported enums in ESM mode", () => {
     assertTypeScriptESMResult(
       `