Pinterest contributions

NOTES.md

@@ -1,10 +1,15 @@
 # Notes
-## React.Node -> React.ReactElement
+## Strip React.Node from function component return types
 
 One issue we encountered was most codemods convert `React.Node` to `React.ReactNode` when they're a function return type. While the names are similar, they behave [differently](https://stackoverflow.com/questions/58123398/when-to-use-jsx-element-vs-reactnode-vs-reactelement?rq=1). In TypeScript, `React.ReactNode` is primarily used to type things that can be children of a React node, and it includes things like booleans or null. When we return `React.Node`, we're usually annotating a functional component that can be instantiated in JSX (`<Component />`). In TypeScript that's inferred [as React.ReactElement or JSX.Element](https://github.com/TypeScript-cheatsheets/react#function-components). This also needs to be done for the `render` method of class components as well, despite some of the [React types](https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/react/index.d.ts#L3087) suggesting otherwise. If you leave it as `React.ReactNode` you'll receive the error `'Component' cannot be used as a JSX component`. In addition, if the function returns `null`, we have to add a `| null` to allow it. Strings and numbers will throw an error if returned in TS.
 
 [TS Playground](https://www.TypeScriptlang.org/play?#code/JYWwDg9gTgLgBAJQKYEMDG8BmUIjgcilQ3wG4Aoc4AOxiSk3STgAUcwBnOAb3Ln7gcA-AC5BMKDQDmFAL6UkAD0iw4aCNQ7wAKki0BGOAF44ACgCUxgHw8+AojACuUanAA8AE2AA3K3S1uAPRevuTy5Eoq8OqaOnowAEzGZmDsHGJsEJyWRja8AnDAmClpAHQclvkF9khOLu4hVtypWRzlskGNdvzyBQ7OrtSOADbDYQrK0NEaWnC6WgDMyaYtnBlp5mLI6DCl2xgAchAezLm2BUUlreWV3dX99Z4+TattHB3Bz3e9NXWDI2NwpEpmoZnEtAAWZavdatTaIYi7fYwACiwyQICQtDgAB84ENRtZzgJLisyhVidU4A9XE9fM1yR8ugUfvwafiAXJKJQYrNMBAIMkAEIC9EoaimfTmCjkNDDFAcLjzGAAVjgSjo1A8XGRpQAwrhINQsfAqmysScoBYtoi9oi0RiTZTqqT+RBLOzTHS4IErNK7n1agMOaMKCy5DK5QqlfEAGzqxSa7UInb6w0aJ1m6kW+jWlMYO07B2Y7F4gnDZ0XYqmN0eoP1L0hH1+sNU7N-EPDVs9CM8sFwPUACyQaAA1sscnk7p6AwI3FZZ1S3MrDL7F9Vl-Ekmu2xvlUsd7u58qoYej+5lWqz0fN1p49eCkE-eNyEA)
 
+To address the issue, we remove the function component return type annotations and allow TypeScript to infer the return types.
+```ts
+const App = ({ message }: AppProps) => <div>{message}</div>;
+```
+
 ## {\[KeyType\]: ValueType} -> Partial<Record<KeyType, ValueType>>
 Object index types work pretty differently between Flow and TypeScript. Flow allows any type to be an index, while TypeScript supports only strings and numbers. One other difference I found is Flow marks all of the keys of the object as optional by default. To get this same behavior, we have to wrap the object in `Partial`.
 
@@ -83,7 +88,7 @@ All of the types are imported and namespaced, and the codemod automatically adds
 
 ## React.AbstractComponent
 
-[AbstractComponent](https://flow.org/en/docs/react/types/#toc-react-abstractcomponent) is a Flow type that helps you construct higher order components, and there is no direct TypeScript equivalent. The identical type is a `ComponentType` with the type of `ref` modified. That is a lot more verbose, so we added the `Flow.AbstractComponent` utility to keep things concise.
+[AbstractComponent](https://flow.org/en/docs/react/types/#toc-react-abstractcomponent) is a Flow type that helps you construct higher order components, and there is no direct TypeScript equivalent. We remove the annotations of `AbstractComponent` and allow TypeScript to infer them.
 
 ## export type * from './foo`
 
@@ -94,11 +99,11 @@ In many cases, it's ideal to keep `type` imports when TypeScript supports it. Th
 We've encountered some global Flow types like [`TimeoutID`](https://github.com/facebook/flow/issues/5627) that needed conversions.
 
 
-## React.ElementConfig -> JSX.LibraryManagedAttributes
+## React.ElementConfig -> React.ComponentProps
 
-Flow has a utility type which gets the props from a component. [TypeScript has a similar type](https://github.com/Microsoft/TypeScript/issues/26704) called `JSX.LibraryManagedAttributes` but there's some extra conversion needed to make the type parameters work.
+Flow has a utility type which gets the props from a component. TypeScript has a similar type called `ComponentProps`.
 
-[TS Playground example](https://www.TypeScriptlang.org/play?ts=4.4.2#code/JYWwDg9gTgLgBAJQKYEMDG8BmUIjgcilQ3wG4BYAKCuADsYkpN0k4AFHMAZzgG8q4cGAE8ANowBccAEYQI4lLQqUAvlSppRKLjwAqSLvCQAPBrQAmPZOhgA6AMK5ItJPQA8HCNwB8fAXDQIWkMoAFcMaAAKME4uKU9uAEo-SkFBLlCwRmjYxOVBNWpUuCILbMSpawxbKpgAOQhzVn5iwSIYUKhaOEj-NLg3c2AANzgAem8+uDz-QsL1SjoGJhY4R3Ag13gWwRivLgB+KUVhZXnKESy4fUMEngBeOAApAGUADVsAGWBpKBQoYQAWUUKAA5khzABBGAwKA-UIMLhuG4wAA01wMMAA2gAiPbcHEAXW8ykurF0YkYcEeKLuWPwInEUHwhNIQA)
+[TS Playground example](https://www.typescriptlang.org/play?ts=4.4.2#code/JYWwDg9gTgLgBAJQKYEMDG8BmUIjgcilQ3wG4BYAKCuADsYkpN0k4AFHMAZzgG8q4cGAE8ANowBccAEYQI4lLQqUAvlSppRKLjwAqSLvCQAPBrQAmPZOhgA6AMK5ItJPQA8HCNwB8fAXDQIWkMoAFcMaAAKME4uKU9uAEo-SkFBLlCwRmjYxOVBNWpUuCILbMSpawxbKpgAOQhzVn5iwSIYUKhaOEj-NLg3c2AANzgAem8+uDz-QsL1SjoGJhY4R3Ag13gWwRivLgB+KUVhZXnKESy4fUMEngBeRGI7dectu7dLpAhMa4MYbzKL7XMSMOCPG4wO4AbXwInEUHwAF1SEA)
 
 ## Utility types
 

package.json

@@ -65,7 +65,7 @@
     "jest-junit": "^12.0.0",
     "patch-package": "^6.4.7",
     "prettier": "^2.4.1",
-    "recast": "^0.20.4",
+    "recast": "0.23.4",
     "signale": "^1.4.0",
     "ts-jest": "^26.0.0",
     "ts-morph": "^13.0.2",

src/convert/declarations.test.ts

@@ -107,6 +107,12 @@ describe("transform declarations", () => {
       expectMigrationReporterMethodNotCalled(`importWithExtension`);
     });
 
+    it("drops React.AbstractComponent imports", async () => {
+      const src = `import {type AbstractComponent, forwardRef} from 'react';`;
+      const expected = `import {forwardRef} from 'react';`;
+      expect(await transform(src)).toBe(expected);
+    });
+
     describe("Flow to TypeScript React import transformations", () => {
       Object.entries(ReactTypes).forEach(([flowType, tsType]) => {
         it(`transforms type imports of ${flowType} from react`, async () => {
@@ -168,6 +174,48 @@ describe("transform declarations", () => {
         });
       });
     });
+
+    describe("change react-router-dom Location and RouterHistory imports", () => {
+      it("keep react-router-dom", async () => {
+        const src = dedent(`
+        import {type Location as L, type RouterHistory as H, useLocation} from 'react-router-dom';
+        const location: L | null = null;
+        const history: H | null = null;
+        `);
+        const expected = dedent(`
+        import {History as H, Location as L} from 'history';
+        import {useLocation} from 'react-router-dom';
+        const location: L | null = null;
+        const history: H | null = null;
+        `);
+        expect(await transform(src)).toBe(expected);
+      });
+
+      it("remove react-router-dom", async () => {
+        const src = dedent(`
+        import {type Location as L, type RouterHistory as H} from 'react-router-dom';
+        const location: L | null = null;
+        const history: H | null = null;
+        `);
+        const expected = dedent(`
+        import {History as H, Location as L} from 'history';
+        const location: L | null = null;
+        const history: H | null = null;
+        `);
+        expect(await transform(src)).toBe(expected);
+      });
+
+      it("do not insert history if one has already been found", async () => {
+        const src = dedent(`
+        import {type RouterHistory} from 'react-router-dom';
+        import {createMemoryHistory} from 'history';
+        `);
+        const expected = dedent(`
+        import {History as RouterHistory, createMemoryHistory} from 'history';
+        `);
+        expect(await transform(src)).toBe(expected);
+      });
+    });
   });
 
   /*
@@ -276,7 +324,7 @@ describe("transform declarations", () => {
   it("converts more complicated $Exact types", async () => {
     const src = dedent`type Test = $Exact<T | { foo: string }>;`;
     const expected = dedent`type Test = T | {
-      foo: string
+      foo: string;
     };`;
     expect(await transform(src)).toBe(expected);
   });
@@ -504,7 +552,7 @@ describe("transform declarations", () => {
   it("Converts React.Node to React.ReactNode in Props", async () => {
     const src = `type Props = {children?: React.Node};`;
     const expected = dedent`type Props = {
-      children?: React.ReactNode
+      children?: React.ReactNode;
     };`;
     expect(await transform(src)).toBe(expected);
   });
@@ -519,43 +567,33 @@ describe("transform declarations", () => {
     expect(await transform(src)).toBe(expected);
   });
 
-  it("Converts React.Node to React.ReactElement in render", async () => {
-    const src = dedent`class Foo extends React.Component {
-      render(): React.Node {return <div />};
-    };`;
-    const expected = dedent`class Foo extends React.Component {
-      render(): React.ReactElement {return <div />};
-    };`;
-    expect(await transform(src)).toBe(expected);
-  });
-
-  it("Adds null to React.ReactElement in render", async () => {
+  it("Converts React.Node to ReactNode in render", async () => {
     const src = dedent`class Foo extends React.Component {
       render(): React.Node {
         if (foo) return (<div />);
         return null;
       };
     };`;
     const expected = dedent`class Foo extends React.Component {
-      render(): React.ReactElement | null {
+      render(): ReactNode {
         if (foo) return (<div />);
         return null;
       };
     };`;
     expect(await transform(src)).toBe(expected);
   });
 
-  it("Converts React.Node to React.ReactElement for render in arrow", async () => {
+  it("Strips out React.Node for render in arrow", async () => {
     const src = dedent`class Foo extends React.Component {
       render = (): React.Node => {return <div />};
     };`;
     const expected = dedent`class Foo extends React.Component {
-      render = (): React.ReactElement => {return <div />};
+      render = () => {return <div />};
     };`;
     expect(await transform(src)).toBe(expected);
   });
 
-  it("Does not convert React.Node to React.ReactElement in non-render", async () => {
+  it("Does not convert React.Node to ReactNode in non-render", async () => {
     const src = dedent`class Foo extends React.Component {
       rendering(): React.Node {return <div />};
     };`;
@@ -565,6 +603,19 @@ describe("transform declarations", () => {
     expect(await transform(src)).toBe(expected);
   });
 
+  it("removes React.AbstractComponent<>", async () => {
+    const src = dedent`
+    // @flow
+    const C1: AbstractComponent<Props, Ref> = memo<Props>((props) => null);
+    const C2: AbstractComponent<Props, Ref> = memo<Props>(Comp);
+    `;
+    const expected = dedent`
+    const C1 = memo<Props>((props) => null);
+    const C2 = memo<Props>(Comp);
+    `;
+    expect(await transform(src)).toBe(expected);
+  });
+
   describe("untyped usestate", () => {
     it("Applies any and creates warning for untyped empty useState.", async () => {
       const src = `const test = React.useState();`;
@@ -739,21 +790,6 @@ describe("transform declarations", () => {
       `;
       expect(await transform(src)).toBe(expected);
     });
-    it("when a comment is in a type param declaration, it should preserve the newline", async () => {
-      const src = dedent`
-      const AThing: Array<
-      // FlowFixMe
-      number> = []
-      `;
-
-      const expected = dedent`
-      const AThing: Array<
-      // FlowFixMe
-      number> = []
-      `;
-
-      expect(await transform(src)).toBe(expected);
-    });
   });
 
   describe("for opaque types", () => {

src/convert/declarations.ts

@@ -62,6 +62,36 @@ const updateReactImports = (
   }
 };
 
+/**
+ * Rename React Router imports for TypeScript
+ */
+const updateReactRouterImports = (
+  node: t.ImportDeclaration,
+  specifier: t.ImportSpecifier
+) => {
+  if (
+    node.source.value === "react-router-dom" &&
+    (specifier.importKind === "type" || node.importKind === "type")
+  ) {
+    // `import type {Match} from 'react-router-dom'` => `import {match} from 'react-router-dom'`
+    if (
+      specifier.type === "ImportSpecifier" &&
+      specifier.imported.type === "Identifier" &&
+      specifier.imported.name === "Match"
+    ) {
+      specifier.imported.name = "match";
+    }
+    // `import {type Match} from 'react-router-dom'` => `import {match} from 'react-router-dom'`
+    if (
+      specifier.type === "ImportSpecifier" &&
+      specifier.local.type === "Identifier" &&
+      specifier.local.name === "Match"
+    ) {
+      specifier.local.name = "match";
+    }
+  }
+};
+
 export function transformDeclarations({
   reporter,
   state,
@@ -106,6 +136,7 @@ export function transformDeclarations({
             (specifier.importKind === "type" || path.node.importKind === "type")
           ) {
             updateReactImports(path.node, specifier);
+            updateReactRouterImports(path.node, specifier);
 
             // `import {type X} from` => `import {X} from`
             if (specifier.importKind === "type") {
@@ -114,6 +145,18 @@ export function transformDeclarations({
           }
         }
 
+        // `import {type AbstractComponent, ...} from 'react'` => `import {...} from 'react'`
+        if (path.node.source.value === "react") {
+          path.node.specifiers = path.node.specifiers.filter(
+            (s) =>
+              !(
+                s.type === "ImportSpecifier" &&
+                s.imported.type === "Identifier" &&
+                s.imported.name === "AbstractComponent"
+              )
+          );
+        }
+
         return;
       }
 
@@ -260,6 +303,19 @@ export function transformDeclarations({
     },
 
     VariableDeclarator(path) {
+      // `const c: AbstractComponent<Props, RefType> =` → `const c =`
+      if (
+        path.node.id.type === "Identifier" &&
+        path.node.id.typeAnnotation?.type === "TypeAnnotation" &&
+        path.node.id.typeAnnotation.typeAnnotation.type ===
+          "GenericTypeAnnotation" &&
+        path.node.id.typeAnnotation.typeAnnotation.id.type === "Identifier" &&
+        path.node.id.typeAnnotation.typeAnnotation.id.name ===
+          "AbstractComponent"
+      ) {
+        path.node.id.typeAnnotation = null;
+      }
+
       if (
         path.parent.type === "VariableDeclaration" &&
         path.parentPath.parent.type !== "ForStatement" &&