Skip to content

Commit f59f982

Browse files
jackhsu978markjm
andauthored
Pinterest contributions (#7)
* [Dep] upgrade to Recast from 0.20.4 to 0.23.4 **What?** Upgrade [recast](https://github.com/benjamn/recast) from `0.20.4` to `0.23.4` for more modern parsers. See [changes](benjamn/[email protected]). As part of the upgrade, the `[email protected]` patch is removed as it is no longer needed. **Why?** When the codemod with `[email protected]` removes comments, the parentheses can be incorrectly removed. Upgrading to `[email protected]` fixes this. For example, the following Flow type: ```js const a = // $FlowFixMe (1 + 1) * 2; ``` was incorrectly converted to: ```ts const a = 1 + 1 * 2; ``` With the new version, it gets correctly converted to: ```ts const a = (1 + 1) * 2; ``` Also, codemod with `[email protected]` keeps the following annotation as is: ```js type Props = { it: string, foo: number }; ```` With [this fix](benjamn/recast#1157) from v0.21.2, the Flow syntax gets correctly converted to the expected TypeScript syntax: ```ts type Props = { it: string; foo: number; }; ``` Lastly, v0.23.4 correctly handles unary expressions by [wrapping unary expressions in parens](benjamn/recast#1361) * Handle indexed access, `$Partial`, `$ReadOnlySet`, and `$ReadOnlyMap` **What** Add the following support (Flow syntax --> TypeScript syntax): - `T[K]` --> `T[K]` - `T?.[K]` --> `NonNullable<T>[K] | null | undefined` - `$Partial<T>` --> `Partial<T>` - `$ReadOnlySet<T>` --> `ReadonlySet<T>` - `$ReadOnlyMap<K, V>` --> `ReadonlyMap<K, V>` **Why** To support more Flow syntax. * Fix bugs with maybe function types and interaction types **Maybe function types** Flow code ```js ?() => void ``` was incorrectly transformed to: ```ts () => void | null | undefined ``` It is now correctly transformed to: ```ts (() => void) | null | undefined ``` **Intersection types** Flow code ```js (A | B) & (C | D) ``` was incorrectly transformed to: ```ts (A | B & C | D) ``` It is now correctly transformed to: ```ts ((A | B) & (C | D)) ``` * Fix false positives of privates types **What** Fix cases where `A$B` are public types instead of private types: 1. Relay generated types such as `PinRep_pin$data`` 2. Type Alias with prefix '$IMPORTED_TYPE$' **Why** To avoid false positives in the private types detection * Add override to force JSX through comment **What** If a file contain the comment `@jsx`, treat it as a JSX file. **Why** A mock file for a `.tsx` file must have the `.tsx` file extension for jest to find it. However, if the mock file does not contain any JSX, the codemod would use the file extension `.ts`. As a workaround, we introduce the ability to add a `@jsx` comment to the mock file to force the codemod to treat it as a JSX file. This ensures the mock file us es the `.tsx` extension, even if it doesn't contain any JSX. * Strip flow comments **What** - Remove flow-specific ESLint error suppression comments - Make sure to retain non-flow comments at the top of files **Why** The flow-related comments are no longer needed. Co-authored-by: Jack Hsu <[email protected]> * [react-router-dom] Improve handling of react-router-dom types **What** The following Flow code: ```js import { type Location, type Match, type RouterHistory } from 'react-router-dom'; type Props = { match: Match }; ``` gets correctly converted to TypeScript code: ```ts import { Location, History as RouterHistory } from 'history'; import { match } from 'react-router-dom'; type Props = { match: match<{ [key: string]: string | undefined }> }; ``` **Why** For the following types from `react-router-dom`: - The `Location` type is moved to `history`. - The `RouterHistory` type is moved to `history`, and is aliased to `History` instead. - The `Match` type becomes a generic type `match`. * [react] improve handling of React types 1. Handle named imports In addition to supporting `React.*` types: ```js import * as React from 'react'; type T = React.Node | React.Element | React.ChildrenArray; ``` We also support named imports: ```js import { type Node as ReactNode, type Element as ReactElement, type ChildrenArray } from 'react'; type T = ReactNode | ReactElement | ChildrenArray; ``` NOTE: To avoid name conflicts with the DOM `Node` and `Element` type, it is important to alias `React.Node` as `ReactNode` and `React.Element` as `ReactElement`. 2. Strip type annotations from React function component return types in favor of inference Flow: ```js const App = ({ message }: AppProps): React.Node => <div>{message}</div>; ``` TypeScript: ```ts const App = ({ message }: AppProps) => <div>{message}</div>; ``` Why not simply change `React.Node` to `React.ReactNode`? If we leave it as `React.ReactNode`, we'll receive the error `'Component' cannot be used as a JSX component`. To address this, we simply strip out the type annotation from the return type of the function component and allow TypeScript to infer it. 3. Strip out `AbstractComponent` in favor of inference Flow: ```js // @flow const C: React.AbstractComponent<Props, mixed> = React.forwardRef<Props, Ref>(Comp); export default (forwardRef<Props, Ref>(Comp): React.AbstractComponent<Props, mixed>); ``` TypeScript: ```ts const C = forwardRef<Ref, Props>(Comp); export default forwardRef<Ref, Props>(Comp); ``` Why? Because there is no `React.AbstractComponent` equivalent in TypeScript. We can simply strip it out and allow TypeScript to infer the type. 4. Reverse params for `forwardRef` Flow: ```js forwardRef<Props, Ref>(Comp); ``` TypeScript: ```ts forwardRef<Ref, Props>(Comp); ``` Why? Because the arguments are swapped in TypeScript. 5. Rename `ElementConfig`, `ElementProps`, `Portal`, and `StatelessFunctionalComponent` - `ElementConfig` --> `ComponentProps` - `ElementProps` --> `ComponentProps` - `Portal` --> `PortalProps` - `StatelessFunctionalComponent` --> `FC` --------- Co-authored-by: Mark Molinaro <[email protected]>
1 parent 078e7d4 commit f59f982

25 files changed

+969
-236
lines changed

NOTES.md

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,15 @@
11
# Notes
2-
## React.Node -> React.ReactElement
2+
## Strip React.Node from function component return types
33

44
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.
55

66
[TS Playground](https://www.TypeScriptlang.org/play?#code/JYWwDg9gTgLgBAJQKYEMDG8BmUIjgcilQ3wG4Aoc4AOxiSk3STgAUcwBnOAb3Ln7gcA-AC5BMKDQDmFAL6UkAD0iw4aCNQ7wAKki0BGOAF44ACgCUxgHw8+AojACuUanAA8AE2AA3K3S1uAPRevuTy5Eoq8OqaOnowAEzGZmDsHGJsEJyWRja8AnDAmClpAHQclvkF9khOLu4hVtypWRzlskGNdvzyBQ7OrtSOADbDYQrK0NEaWnC6WgDMyaYtnBlp5mLI6DCl2xgAchAezLm2BUUlreWV3dX99Z4+TattHB3Bz3e9NXWDI2NwpEpmoZnEtAAWZavdatTaIYi7fYwACiwyQICQtDgAB84ENRtZzgJLisyhVidU4A9XE9fM1yR8ugUfvwafiAXJKJQYrNMBAIMkAEIC9EoaimfTmCjkNDDFAcLjzGAAVjgSjo1A8XGRpQAwrhINQsfAqmysScoBYtoi9oi0RiTZTqqT+RBLOzTHS4IErNK7n1agMOaMKCy5DK5QqlfEAGzqxSa7UInb6w0aJ1m6kW+jWlMYO07B2Y7F4gnDZ0XYqmN0eoP1L0hH1+sNU7N-EPDVs9CM8sFwPUACyQaAA1sscnk7p6AwI3FZZ1S3MrDL7F9Vl-Ekmu2xvlUsd7u58qoYej+5lWqz0fN1p49eCkE-eNyEA)
77

8+
To address the issue, we remove the function component return type annotations and allow TypeScript to infer the return types.
9+
```ts
10+
const App = ({ message }: AppProps) => <div>{message}</div>;
11+
```
12+
813
## {\[KeyType\]: ValueType} -> Partial<Record<KeyType, ValueType>>
914
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`.
1015

@@ -83,7 +88,7 @@ All of the types are imported and namespaced, and the codemod automatically adds
8388

8489
## React.AbstractComponent
8590

86-
[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.
91+
[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.
8792

8893
## export type * from './foo`
8994

@@ -94,11 +99,11 @@ In many cases, it's ideal to keep `type` imports when TypeScript supports it. Th
9499
We've encountered some global Flow types like [`TimeoutID`](https://github.com/facebook/flow/issues/5627) that needed conversions.
95100

96101

97-
## React.ElementConfig -> JSX.LibraryManagedAttributes
102+
## React.ElementConfig -> React.ComponentProps
98103

99-
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.
104+
Flow has a utility type which gets the props from a component. TypeScript has a similar type called `ComponentProps`.
100105

101-
[TS Playground example](https://www.TypeScriptlang.org/play?ts=4.4.2#code/JYWwDg9gTgLgBAJQKYEMDG8BmUIjgcilQ3wG4BYAKCuADsYkpN0k4AFHMAZzgG8q4cGAE8ANowBccAEYQI4lLQqUAvlSppRKLjwAqSLvCQAPBrQAmPZOhgA6AMK5ItJPQA8HCNwB8fAXDQIWkMoAFcMaAAKME4uKU9uAEo-SkFBLlCwRmjYxOVBNWpUuCILbMSpawxbKpgAOQhzVn5iwSIYUKhaOEj-NLg3c2AANzgAem8+uDz-QsL1SjoGJhY4R3Ag13gWwRivLgB+KUVhZXnKESy4fUMEngBeOAApAGUADVsAGWBpKBQoYQAWUUKAA5khzABBGAwKA-UIMLhuG4wAA01wMMAA2gAiPbcHEAXW8ykurF0YkYcEeKLuWPwInEUHwhNIQA)
106+
[TS Playground example](https://www.typescriptlang.org/play?ts=4.4.2#code/JYWwDg9gTgLgBAJQKYEMDG8BmUIjgcilQ3wG4BYAKCuADsYkpN0k4AFHMAZzgG8q4cGAE8ANowBccAEYQI4lLQqUAvlSppRKLjwAqSLvCQAPBrQAmPZOhgA6AMK5ItJPQA8HCNwB8fAXDQIWkMoAFcMaAAKME4uKU9uAEo-SkFBLlCwRmjYxOVBNWpUuCILbMSpawxbKpgAOQhzVn5iwSIYUKhaOEj-NLg3c2AANzgAem8+uDz-QsL1SjoGJhY4R3Ag13gWwRivLgB+KUVhZXnKESy4fUMEngBeRGI7dectu7dLpAhMa4MYbzKL7XMSMOCPG4wO4AbXwInEUHwAF1SEA)
102107

103108
## Utility types
104109

package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@
6565
"jest-junit": "^12.0.0",
6666
"patch-package": "^6.4.7",
6767
"prettier": "^2.4.1",
68-
"recast": "^0.20.4",
68+
"recast": "0.23.4",
6969
"signale": "^1.4.0",
7070
"ts-jest": "^26.0.0",
7171
"ts-morph": "^13.0.2",

patches/recast+0.20.4.patch

Lines changed: 0 additions & 17 deletions
This file was deleted.

src/convert/add-imports.ts

Lines changed: 0 additions & 24 deletions
This file was deleted.

src/convert/declarations.test.ts

Lines changed: 68 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,12 @@ describe("transform declarations", () => {
107107
expectMigrationReporterMethodNotCalled(`importWithExtension`);
108108
});
109109

110+
it("drops React.AbstractComponent imports", async () => {
111+
const src = `import {type AbstractComponent, forwardRef} from 'react';`;
112+
const expected = `import {forwardRef} from 'react';`;
113+
expect(await transform(src)).toBe(expected);
114+
});
115+
110116
describe("Flow to TypeScript React import transformations", () => {
111117
Object.entries(ReactTypes).forEach(([flowType, tsType]) => {
112118
it(`transforms type imports of ${flowType} from react`, async () => {
@@ -168,6 +174,48 @@ describe("transform declarations", () => {
168174
});
169175
});
170176
});
177+
178+
describe("change react-router-dom Location and RouterHistory imports", () => {
179+
it("keep react-router-dom", async () => {
180+
const src = dedent(`
181+
import {type Location as L, type RouterHistory as H, useLocation} from 'react-router-dom';
182+
const location: L | null = null;
183+
const history: H | null = null;
184+
`);
185+
const expected = dedent(`
186+
import {History as H, Location as L} from 'history';
187+
import {useLocation} from 'react-router-dom';
188+
const location: L | null = null;
189+
const history: H | null = null;
190+
`);
191+
expect(await transform(src)).toBe(expected);
192+
});
193+
194+
it("remove react-router-dom", async () => {
195+
const src = dedent(`
196+
import {type Location as L, type RouterHistory as H} from 'react-router-dom';
197+
const location: L | null = null;
198+
const history: H | null = null;
199+
`);
200+
const expected = dedent(`
201+
import {History as H, Location as L} from 'history';
202+
const location: L | null = null;
203+
const history: H | null = null;
204+
`);
205+
expect(await transform(src)).toBe(expected);
206+
});
207+
208+
it("do not insert history if one has already been found", async () => {
209+
const src = dedent(`
210+
import {type RouterHistory} from 'react-router-dom';
211+
import {createMemoryHistory} from 'history';
212+
`);
213+
const expected = dedent(`
214+
import {History as RouterHistory, createMemoryHistory} from 'history';
215+
`);
216+
expect(await transform(src)).toBe(expected);
217+
});
218+
});
171219
});
172220

173221
/*
@@ -276,7 +324,7 @@ describe("transform declarations", () => {
276324
it("converts more complicated $Exact types", async () => {
277325
const src = dedent`type Test = $Exact<T | { foo: string }>;`;
278326
const expected = dedent`type Test = T | {
279-
foo: string
327+
foo: string;
280328
};`;
281329
expect(await transform(src)).toBe(expected);
282330
});
@@ -504,7 +552,7 @@ describe("transform declarations", () => {
504552
it("Converts React.Node to React.ReactNode in Props", async () => {
505553
const src = `type Props = {children?: React.Node};`;
506554
const expected = dedent`type Props = {
507-
children?: React.ReactNode
555+
children?: React.ReactNode;
508556
};`;
509557
expect(await transform(src)).toBe(expected);
510558
});
@@ -519,43 +567,33 @@ describe("transform declarations", () => {
519567
expect(await transform(src)).toBe(expected);
520568
});
521569

522-
it("Converts React.Node to React.ReactElement in render", async () => {
523-
const src = dedent`class Foo extends React.Component {
524-
render(): React.Node {return <div />};
525-
};`;
526-
const expected = dedent`class Foo extends React.Component {
527-
render(): React.ReactElement {return <div />};
528-
};`;
529-
expect(await transform(src)).toBe(expected);
530-
});
531-
532-
it("Adds null to React.ReactElement in render", async () => {
570+
it("Converts React.Node to ReactNode in render", async () => {
533571
const src = dedent`class Foo extends React.Component {
534572
render(): React.Node {
535573
if (foo) return (<div />);
536574
return null;
537575
};
538576
};`;
539577
const expected = dedent`class Foo extends React.Component {
540-
render(): React.ReactElement | null {
578+
render(): ReactNode {
541579
if (foo) return (<div />);
542580
return null;
543581
};
544582
};`;
545583
expect(await transform(src)).toBe(expected);
546584
});
547585

548-
it("Converts React.Node to React.ReactElement for render in arrow", async () => {
586+
it("Strips out React.Node for render in arrow", async () => {
549587
const src = dedent`class Foo extends React.Component {
550588
render = (): React.Node => {return <div />};
551589
};`;
552590
const expected = dedent`class Foo extends React.Component {
553-
render = (): React.ReactElement => {return <div />};
591+
render = () => {return <div />};
554592
};`;
555593
expect(await transform(src)).toBe(expected);
556594
});
557595

558-
it("Does not convert React.Node to React.ReactElement in non-render", async () => {
596+
it("Does not convert React.Node to ReactNode in non-render", async () => {
559597
const src = dedent`class Foo extends React.Component {
560598
rendering(): React.Node {return <div />};
561599
};`;
@@ -565,6 +603,19 @@ describe("transform declarations", () => {
565603
expect(await transform(src)).toBe(expected);
566604
});
567605

606+
it("removes React.AbstractComponent<>", async () => {
607+
const src = dedent`
608+
// @flow
609+
const C1: AbstractComponent<Props, Ref> = memo<Props>((props) => null);
610+
const C2: AbstractComponent<Props, Ref> = memo<Props>(Comp);
611+
`;
612+
const expected = dedent`
613+
const C1 = memo<Props>((props) => null);
614+
const C2 = memo<Props>(Comp);
615+
`;
616+
expect(await transform(src)).toBe(expected);
617+
});
618+
568619
describe("untyped usestate", () => {
569620
it("Applies any and creates warning for untyped empty useState.", async () => {
570621
const src = `const test = React.useState();`;
@@ -739,21 +790,6 @@ describe("transform declarations", () => {
739790
`;
740791
expect(await transform(src)).toBe(expected);
741792
});
742-
it("when a comment is in a type param declaration, it should preserve the newline", async () => {
743-
const src = dedent`
744-
const AThing: Array<
745-
// FlowFixMe
746-
number> = []
747-
`;
748-
749-
const expected = dedent`
750-
const AThing: Array<
751-
// FlowFixMe
752-
number> = []
753-
`;
754-
755-
expect(await transform(src)).toBe(expected);
756-
});
757793
});
758794

759795
describe("for opaque types", () => {

src/convert/declarations.ts

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,36 @@ const updateReactImports = (
6262
}
6363
};
6464

65+
/**
66+
* Rename React Router imports for TypeScript
67+
*/
68+
const updateReactRouterImports = (
69+
node: t.ImportDeclaration,
70+
specifier: t.ImportSpecifier
71+
) => {
72+
if (
73+
node.source.value === "react-router-dom" &&
74+
(specifier.importKind === "type" || node.importKind === "type")
75+
) {
76+
// `import type {Match} from 'react-router-dom'` => `import {match} from 'react-router-dom'`
77+
if (
78+
specifier.type === "ImportSpecifier" &&
79+
specifier.imported.type === "Identifier" &&
80+
specifier.imported.name === "Match"
81+
) {
82+
specifier.imported.name = "match";
83+
}
84+
// `import {type Match} from 'react-router-dom'` => `import {match} from 'react-router-dom'`
85+
if (
86+
specifier.type === "ImportSpecifier" &&
87+
specifier.local.type === "Identifier" &&
88+
specifier.local.name === "Match"
89+
) {
90+
specifier.local.name = "match";
91+
}
92+
}
93+
};
94+
6595
export function transformDeclarations({
6696
reporter,
6797
state,
@@ -106,6 +136,7 @@ export function transformDeclarations({
106136
(specifier.importKind === "type" || path.node.importKind === "type")
107137
) {
108138
updateReactImports(path.node, specifier);
139+
updateReactRouterImports(path.node, specifier);
109140

110141
// `import {type X} from` => `import {X} from`
111142
if (specifier.importKind === "type") {
@@ -114,6 +145,18 @@ export function transformDeclarations({
114145
}
115146
}
116147

148+
// `import {type AbstractComponent, ...} from 'react'` => `import {...} from 'react'`
149+
if (path.node.source.value === "react") {
150+
path.node.specifiers = path.node.specifiers.filter(
151+
(s) =>
152+
!(
153+
s.type === "ImportSpecifier" &&
154+
s.imported.type === "Identifier" &&
155+
s.imported.name === "AbstractComponent"
156+
)
157+
);
158+
}
159+
117160
return;
118161
}
119162

@@ -260,6 +303,19 @@ export function transformDeclarations({
260303
},
261304

262305
VariableDeclarator(path) {
306+
// `const c: AbstractComponent<Props, RefType> =` → `const c =`
307+
if (
308+
path.node.id.type === "Identifier" &&
309+
path.node.id.typeAnnotation?.type === "TypeAnnotation" &&
310+
path.node.id.typeAnnotation.typeAnnotation.type ===
311+
"GenericTypeAnnotation" &&
312+
path.node.id.typeAnnotation.typeAnnotation.id.type === "Identifier" &&
313+
path.node.id.typeAnnotation.typeAnnotation.id.name ===
314+
"AbstractComponent"
315+
) {
316+
path.node.id.typeAnnotation = null;
317+
}
318+
263319
if (
264320
path.parent.type === "VariableDeclaration" &&
265321
path.parentPath.parent.type !== "ForStatement" &&

0 commit comments

Comments
 (0)