Consider migrating from Flow to TypeScript

[gregtatum]

This project started when this was a more difficult choice, but at this time TypeScript is the clear winner in the community for support. It would be great to migrate this project to TypeScript, although it would be a lot of work and difficult to do. My preference would to be on TypeScript, especially as in Gecko, we're using it in places.

This issue is to gather ideas and document how to handle this migration if we should ever decide to do it.

┆Issue is synchronized with this Jira Task

[gregtatum]

Intermediate migration using Flow

I recently did this on a bigger personal project and I wanted to share some insights on how to handle it. I think there are some opportunities to clean up our Flow types that would then make it fairly trivial to switch to TypeScript.

Read-only objects

Typescript doesn't support the + for read only properties. In Flow this is { +foo: 'bar' }. In TypeScript, this is { readonly foo: 'bar' }

Change in Flow types: {+foo: 'bar'} To: $ReadOnly<{foo: bar}>

This is will then be trivial to change to typescript's Readonly<{foo: bar}>

I think handling this manually would make us more explicit on how we're handling read only information. Plus, there might be opportunities to make something read only in part of the pipeline where it makes sense. For instance, we could make things read only on the connected props only.

Migrate type spread

{...A} is not supported by TypeScript. See https://github.com/microsoft/TypeScript/issues/10727. We could use the & operator and get around this.

Change:

{
  ...A,
  ...B,
}

To:

A & B

Clean-up Object types, and {}

Object is bad, and {} means something different in TypeScript. We should migrate these to:

Flow:

export type MixedObject = { [key: string]: mixed };
export type EmptyObject = { [key: string]: empty };

TypeScript:

export type MixedObject = { [key: string]: unknown };
export type EmptyObject = { [key: string]: never };

Consider removing higher order components

I haven't verified this, but WithSize and WithViewport might be quite hard to translate the types. We could migrate those to hooks, which I would assume would be much easier to type.

Remove explicit connect

I wrote that piece of machinery before better type options existed. The built-in typing for connect can handle these use-cases on the TypeScript side. I bet they can on the Flow side as well. We should migrate our types to only use connect. I'll add another comment with information on how that could happen.

Remove use of the * type

This doesn't exist in TypeScript.

Figure out a solution for the per-thread selectors

I don't think these will survive a migration, and are a bit broken right now anyway.

[gregtatum]

Migration notes

Flow	TypeScript
mixed	unknown
<T: Constraint>	<T extends Constraint>
*	I mostly changed these to unknown but there is the infer property for generics. ReturnType<T> can infer something, which you can try to do explicitly.
Open { foo: 'bar' }	Open { foo: 'bar', [key: string]: unknown }

Flow: (this broke the table formatting) Closed {| foo: 'bar' |}

Typescript Closed { foo: 'bar' }

[gregtatum]

Migrating connected components

Here is a code dump with a solution for TypeScript. We could implement this first in Flow, it should be fairly similar. I couldn't get the $ObjMap equivalent in TypeScript to work over unions of types.

Store types

/**
 * Selectors always take the root state, and return some part of it.
 */
export type Selector<Returns> = (state: State) => Returns;

/**
 * Provide a mechanism to easily define reducers that are bound to the current
 * set of Actions, and enforce the constraint that the first parameter must be
 * the same as the return value.
 *
 * See src/reducers for practical examples of how this is used.
 */
export type Reducer<S> = (state: S, action: Action) => S;

export type Thunk<Returns> = (
  dispatch: Dispatch,
  getState: () => State
) => Returns;

/**
 * The rest of these pre-fill Redux with all of the configured Actions and middlewares.
 */
type ThunkDispatch = <Returns>(action: Thunk<Returns>) => Returns;
type PlainDispatch = (action: Action) => Action;
export type GetState = () => State;
export type Dispatch = PlainDispatch & ThunkDispatch;
export type Store = {
  dispatch: Dispatch;
  getState(): State;
  subscribe(listener: () => void): unknown;
  replaceReducer(nextReducer: Reducer<State>): void;
};

// ThunkActions don't type easily with react/redux, so provide a means to "de-thunk" them.
export type DeThunk0<T> = T extends () => (
  dispatch: Dispatch,
  getState: GetState
) => infer R
  ? () => R
  : () => any;

export type DeThunk1<T, A1> = T extends (
  a1: A1
) => (dispatch: Dispatch, getState: GetState) => infer R
  ? (a1: A1) => R
  : (a1: A1) => any;

export type DeThunk2<T, A1, A2> = T extends (
  a1: A1,
  a2: A2
) => (dispatch: Dispatch, getState: GetState) => infer R
  ? (a1: A1, a2: A2) => R
  : (a1: A1, a2: A2) => any;

Utility functions

/**
 * Wrap thunk actions with 0-N arguments with these functions when passing them to
 * mapDispatchToProps. This will make them typecheck correctly.
 */
export function coerceThunk0<T>(thunk: () => Thunk<T>): DeThunk0<Thunk<T>> {
  return thunk as any;
}
export function coerceThunk1<
  A1,
  Returns,
  Fn extends (a1: A1) => Thunk<Returns>
>(thunk: Fn): DeThunk1<Thunk<Returns>, A1> {
  return thunk as any;
}
export function coerceThunk2<
  A1,
  A2,
  Returns,
  Fn extends (a1: A1, a2: A2) => Thunk<Returns>
>(thunk: Fn): DeThunk2<Thunk<Returns>, A1, A2> {
  return thunk as any;
}

A connected component

import React, { PureComponent } from 'react';
import { connect } from 'react-redux';
import * as selectors from '../../selectors';
import * as actions from '../../actions';
import { State, Items, DeThunk1 } from '../../types';
import { coerceThunk1 } from '../../utils';

type StateProps = {
  myData: null | string;
  items: Items[];
  // ...
};

type DispatchProps = {
  // DeThunk1 will take a thunk with 1 argument parameter, and remove the
  // (Dispatch, GetState) part of it.
  fetchEntries: DeThunk1<typeof actions.fetchEntries, string>;
};

type Props = Readonly<StateProps & DispatchProps>;

class MyComponentImpl extends PureComponent<Props> { ... }

export const MyComponent = connect<StateProps, DispatchProps>(
  // mapStateToProps
  (state: State): StateProps => ({
    myData: selectors.getMyData(state),
    items: selectors.getItems(state),
    // ...etc
  }),
  // mapDispatchToProps
  {
    // This function returns the same function, but the type is modified to match
    // a de-thunked version.
    fetchEntries: coerceThunk1(A.fetchEntries),
  }
)(
  // Component
  MyComponentImpl
);

[gregtatum]

Automated tool: https://github.com/Khan/flow-to-ts

I tried this on our own repo awhile back, and I still had lots of errors afterwards.

[ghost]

@gregtatum this is a great move. Can i get started with this? Also I have a suggestion. Rather than an entire overhaul of the entire project from flow to typescript, would it not be better to start slowly? Maybe with the less core files? Then move upwards? I'm just suggesting. But i would really want to give this a go.

[qmya]

Hey, @KarenEfereyan Are you working on this?

[ghost]

Hey, @KarenEfereyan Are you working on this?

No

[gregtatum]

@KarenEfereyan This is not ready to be worked on, it's just a proposal at this point, plus it most likely will be done by core team members, as it's a bit of a risky operation.

[ghost]

@KarenEfereyan This is not ready to be worked on, it's just a proposal at this point, plus it most likely will be done by core team members, as it's a bit of a risky operation.

Noted, sir

[gregtatum]

Since I'm moving to work on Internationalization, here is the current status of this work:

I was hoping to do this in bits on the side, but I think it requires a bit more concentrated effort to accomplish. I think there are still opportunities to do this incrementally, but there needs to be at least some point where things like CI get paused and a big migration event should occur.

Probably the toughest piece is getting the React components migrated, which I believe should use the built-in connect feature. I started this work with #3063, and #3064. The idea is to write 2 sets of tests. One using Flow's types, and another using TypeScript's. Ultimately, they should agree on the API using the simple connect. This way, we can do the invasive change of migrating connect in Flow types, then the switch to TypeScript can just use those.

I think the "wip" part of #3063 and #3064 is verifying the two sets of tests agree on the API, and are "correct enough". I don't think I have time to handle review feedback on those right now. The TypeScript changes (#3064) could probably landed as-is since they are very low risk, and then any changes could be followed-up. I would recommend finishing (#3063) to be correct in agreement with the TypeScript work since it's touching real components. Although, src/test/types/react-redux-connect.js could be trivially landed, and then follow-up with the final real changes.

[gregtatum]

The following is my recommended order for accomplishing this migration.

Re-work Flow to be closer to TypeScript

Some of these may be hard to grep, and may be worth doing after the migration scripts.

• [ ] Finish #3063 and #3064 to agree on a TypeScript/Flow compatible API for connected components.
• [ ] Migrate all connected components to the new API.
• [ ] Remove all read-only { +prop: Type } syntax, and migrate to $ReadOnly<>
• [ ] Migrate type spread from { prop1: Type1, ...ObjType} to { prop1: Type1 } & { prop2: Type2 }.
• [ ] Fix named arguments, e.g. TemporaryError => void to (error: TemporaryError) => void
• [ ] Fix named unnamed keys e.g. { [ThreadsKey]: ThreadViewOptions } to { [key: ThreadsKey]: ThreadViewOptions }

Create global type aliases in a .d.ts file

These can optionally be removed AFTER the migration, but it's less upfront work to do it this way.

• [ ] $ReadOnly<T> to Readonly<T>
• [ ] $Shape<T> to Partial<T>.
• [ ] empty to never

Write migration scripts

A significant amount of the migration can happen in bash scripts. I would recommend writing and landing these incrementally. Finally as a last step, these scripts can be run without actually needing a human reviewer to meticulously review the results.

• [ ] Search and replace {| to { and |} to }. These are fairly unique characters.
• [ ] Remove flow libdefs
• [ ] Rename .js files to .ts files
• [ ] Install @types definitions
• [ ] Rename import type { to import {
• [ ] Maybe run the flow-to-ts tool. I didn't have great results the first time I ran it, but perhaps handling the "Re-work Flow to be closer to TypeScript" section would give better results.

Manual migrations that should probably happen after the scripts work.

• [ ] Fix templating syntax, e.g. Fn<Obj: { [key: string]: Value }> to Fn<Obj extends { [key: string]: Value }>

Finally, some these steps may be easier to write scripts for if we can write custom lint rules, or AST-aware tools. I spent a brief amount of time looking at the latter, but couldn't figure them out without investing more time than I was willing to.

[gregtatum]

For reference, here is a local attempt to handle the migration. It's useful for identifying the class of errors you get. There were still lots of errors with it, but I think most were syntax errors that I've outlined above.

https://github.com/firefox-devtools/profiler/compare/main...gregtatum:flow-to-ts-2020?expand=1

I can't remember if I ran flow-to-ts or not on it.

The tsconfig.json is probably useful to point out.

[julienw]

https://github.com/sindresorhus/type-fest could be a useful typescript-related project with utility types. Especially their Merge utility type can be slightly more correct than the intersection to replace the spread operator (where the type for one overridden property is changed by the second type). Other utility types seem useful too.

[amilajack]

I'm interested in helping with this migration. Would a PR for the migration be welcome?

[gregtatum]

The code base is too large to do in one PR. I wrote up an action plan above, which breaks it down into smaller chunks. I'm sure a PR per checkbox would be welcome. Just comment on the individual piece you would like to take on.