Pinterest contributions #7
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
**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)
**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.
**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)) ```
**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
**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.
**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]>
**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`.
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`
|
Thank you for sharing this! We'll take a look |
|
Is this PR ready for review if we want to merge it? I think it would be fine to merge once we've had a chance to test it out. |
@tylerkrupicka-stripe Yes, it's ready for review now! We really appreciate your time and your flexibility, especially given that this project has been archived! Thank you in advance! 🙏 |
|
Tested this out locally and it all looks good! |
|
I added a thank you in the Readme. Thanks again! |
|
@tylerkrupicka-stripe Thank you for mentioning us in the Readme! FYI: we have just published a blog post about our migration experience, Migrating 3.7 Million Lines of Flow Code to TypeScript. It's interesting that we migrated a similar number of lines of code—over 3.7 million lines—as you did. |
|
Great write up! That's so funny that our largest PRs ended up being similar -- glad it all worked out! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
At Pinterest, we used
flow-to-typescript-codemodto migrate 3.7 million lines of code from Flow to TypeScript. Along the way, we learned a lot and greatly benefited from the open source community, and it's our turn to give back.These are the improvements we made in that effort:
0.20.4to0.23.4for more modern parsers (see changes)T[K]andT?.[K]$Partial,$ReadOnlySet,$ReadOnlyMap?()=>void(A | B) & (C | D)$IMPORTED_TYPE$Fooand Relay type (e.g.PinRep_pin$data) are mistakenly treated as private typesreact-router-dom: handleMatchandRouterHistoryreactimport { type Foo } from 'react'in addition toReact.FooAbstractComponentin favor of inferenceforwardRef<>ElementConfig,ElementProps,Portal, andStatelessFunctionalComponentTip
This pull request consists of multiple commits. For more details, click on the ellipsis
...of each commit like this:Note
We will share our blog post on the TypeScript migration soon.
cc @tylerkrupicka-stripe