Experiment with typescript by converting wonder-blocks-core to use it #382
Conversation
There was a problem hiding this comment.
Still lots of things to figure such as getting the jest tests running with typescript. As I was working on this I investigated a few tools for automatically converting flow types to typescript types and... they've all bit-rotted. Also, they didn't do anything to convert types that were named different, e.g. converting SyntheticMouseEvent to React.MouseEvent. Hopefully the syntax changes to flow are slowing down. If so, it might be a good time to build a conversion tool to handle both the different in syntax and the name of the types themselves.
| @@ -1,5 +1,5 @@ | |||
| [version] | |||
| 0.93.0 | |||
| 0.95.1 | |||
There was a problem hiding this comment.
I was also trying out the new batch-coverage command in flow 0.95.
| @@ -0,0 +1,25 @@ | |||
| /* eslint-disable import/no-commonjs */ | |||
| module.exports = { | |||
| presets: ["@babel/typescript", "@babel/preset-react", "@babel/preset-env"], | |||
There was a problem hiding this comment.
The only difference is swapping out @babel/typescript for @babel/flow.
| // type BaseCSSProperties = Foo & { | ||
| // WebkitRocketLauncher?: string; | ||
| // } | ||
| // } |
There was a problem hiding this comment.
A failed attempt at augmenting BaseCSSProperties to include custom properties. I ended up using OpenCSSProperties.
| @@ -0,0 +1,188 @@ | |||
| import * as React from "react"; | |||
There was a problem hiding this comment.
Apparently tsc doesn't provide a way to generate a single .d.ts file for a whole package. I'm investigating some ways to do this.
| @@ -72,7 +72,7 @@ type Props = {| | |||
| /** | |||
| * A function to be executed `onclick`. | |||
| */ | |||
| onClick?: (e: SyntheticEvent<>) => mixed, | |||
| onClick?: (e: React.MouseEvent) => unknown, | |||
There was a problem hiding this comment.
unknown is the equivalent of mixed.
|
|
||
| function rect(top, right, bottom, left) { | ||
| const mockedEnumerateScrollAncestors = enumerateScrollAncestors as jest.Mock; |
There was a problem hiding this comment.
This is probably something we could do in flow as well.
| "aria-controls"?: IdRefList, | ||
| "aria-describedat"?: string, // URI | ||
| "aria-describedby"?: IdRefList, | ||
| "aria-disabled"?: "false" | "true", | ||
| "aria-dropeffect"?: "copy" | "execute" | "link" | "move" | "none" | "popup", | ||
| "aria-expanded"?: "false" | "true" | "undefined", | ||
| "aria-expanded"?: "false" | "true" | boolean, |
There was a problem hiding this comment.
These changes were to bring these type definitions into line with TypeScript's aria type definitions. I wonder if there's a way we just reuse those type definitions instead of creating our own. 🤔
| "aria-readonly"?: "false" | "true", | ||
| "aria-relevant"?: void, // Not using aria-relevant is a best practice | ||
| "aria-relevant"?: "text" | "all" | "additions" | "additions text" | "removals", // Not using aria-relevant is a best practice |
There was a problem hiding this comment.
This was void before to prevent people from using this prop. We could instead use https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/forbid-component-props.md to forbid the use of this prop.
|
|
||
| ...EventHandlers, | ||
| }; | ||
| } & AriaProps & EventHandlers; |
There was a problem hiding this comment.
There's no spread operator for types. This should be okay in most cases. The only situation where we'd have to do something different is if we want to replace x: number with x: string. Replacing x: any with x: string is fine as is replacing x: string with x: "foo".
| @@ -1,8 +1,7 @@ | |||
| // @flow | |||
| import {StyleSheet, css} from "aphrodite"; | |||
| import type {CSSProperties} from "aphrodite"; | |||
| import {StyleSheet, CSSProperties, css} from "aphrodite"; | |||
There was a problem hiding this comment.
I like the fact that there isn't separate imports for types.
| type Falsy = false | 0 | null | void; | ||
|
|
||
| interface NestedArray<T> extends ReadonlyArray<T | NestedArray<T>> { }; |
There was a problem hiding this comment.
TypeScript requires the use of interfaces when writing recursive types, see microsoft/TypeScript#3496 (comment).
|
Closing since this isn't a real PR. It did provide some useful in about what kind of changes are necessary. |
I was curious to the benefits but also the challenges of switching to TypeScript.