Migrate types from Flow to TypeScript (?) #3458
Comments
gnprice
changed the title
|
I still expect this will probably eventually be the best choice for us -- it's probably what the Zulip web frontend will end up on, and it'd certainly be good to be on the same toolchain there and here. But there are real downsides, applicable equally to the mobile app and the web frontend: most of all, TypeScript doesn't even try to be sound. I knew that already at a one-line level of detail, but recently realized it's much worse than I'd thought. Details in chat. In particular, if you routinely use the "user-defined type guards" feature in the intended way... then TS behaves less like "a type-checker" than "a pretty OK linter". 😞 Maybe if you just don't use that feature, things are better? Like C++. |
On the linked chat thread, @andersk devised a small helper which can be employed to safely use this feature. The helper itself remains unchecked, but there's only one of it; and user-defined type guards based on it contain type-checked proofs of their soundness. And I think the bulk of legitimate uses of this feature can be done cleanly atop that helper. That's not so bad, then. It makes the analogy with C++ rather closer than I thought: if this issue is representative, then it's possible to use TypeScript (and even some of its sharper-edged features) as a mostly-safe language in the same way as C++, by having someone on the team take the time to become an expert in (a) the hidden pitfalls and (b) the tricks for systematically avoiding them. Still disappointing. One reason it still leaves me concerned in general is that in the case of C++, the tricks for avoiding the pitfalls are generally well known in the community and seen as best practices, and I think were often specifically intended by the language designers when the pitfalls were introduced -- whereas in this case, Anders's solution feels to me like it's not in the spirit of TypeScript, not something the language designers would intend people to do. Which suggests they intend people to use this feature blatantly unsafely, and gave no warning of that in the docs... which is not promising when it comes to all the other choices they've made that we haven't yet studied in detail. (Also, "as sound as C++" is not exactly high praise for a type system designed from scratch this century.) |
|
For future reference: https://github.com/joarwilk/flowgen is a tool to automatically extract Flow interface files from TypeScript, which may be useful if we find ourselves in a world with both Flow and TypeScript files. ... which, as it happens is the world we're in anyway: several of our dependencies are TypeScript rather than Flow — likely soon including the Zulip server front-end: see (our) issue #3538, or its dual zulip/zulip#13253. |
|
I'm encouraged to see this update about Flow development -- though I'm nearly a year late in spotting it, so it actually predates my filing this issue:
This is exactly what I'd inferred from what's publicly visible. It's encouraging to see them acknowledge it frankly -- and the details in the post sound credible that they've made solid progress on those internal challenges, and may be largely through them by now (a year later). Perhaps Flow will have a future beyond Facebook after all. I'm also encouraged by the progress in the latter half of 2019 on a big cluster of the most annoying problems in Flow's type system, in object spreads: |
Do not use `remotedev-serialize` directly. In the next commit, we fix a security hole by wrapping this dependency and recommending the use of the wrapper. In an upcoming commit, we will set up a system of "replacer" and "reviver" transforms so that we can store non-serializable objects, such as ZulipVersion instances, in Redux, and have a serializable format be persisted to disk. This same system will be used for non-serializable objects besides ZulipVersion instances; see zulip#3949 and zulip#3950, which will be much easier after this series. For the Flow types for these modules: Thankfully, `immutable`, being much more popular, has Flow types out of the box. Some assembly is required for a libdef for `remotedev-serialize`, but it's important to get one working [1]. 1. Check to see if someone has already submitted a libdef to FlowTyped, with `flow-typed install remotedev-serialize`. There isn't; we get the output, '!! No [email protected] libdefs found in flow-typed for the explicitly requested libdefs. !! Consider using `flow-typed create-stub remotedev-serialize` to generate an empty libdef that you can fill in.'. 2. As that output suggests, run `flow-typed create-stub remotedev-serialize` . This creates `flow-typed/npm/remotedev-serialize_vx.x.x.js` and fills it with a template based on the directory structure in `node_modules/remotedev-serialize`. Move this to `flow-typed/remotedev-serialize_vx.x.x.js` (no `npm`) because we want to maintain it locally; we don't want local adjustments we make to get clobbered by an eventual libdef in the FlowTyped repo. Delete the metadata lines at the top; they work as a tag on libdef contents that come from the FlowTyped repo, which this one doesn't [2]. 3. Here, we could enter everything in manually, but it turns out that DefinitelyTyped has a TypeScript libdef for `remotedev-serialize` [3], which we can use as a starting point. So, copy that into a temporary local text file as, e.g., libdef.d.ts. 4. Flowgen [4] [5] [6] is a tool for translating from TypeScript to Flow. It isn't perfect, and it's transparent about that, which is good to see. We just need this single file translated, and it's small, so that increases our chances of success. Run `flowgen libdef.d.ts`. 5. That output isn't exactly in the form that we want, though. We want to put this information in `flow-typed/remotedev-serialize_vx.x.x.js` from step 2, in this block: ``` declare module 'remotedev-serialize' { declare module.exports: any; } ``` Copy it into that block, in any case, deleting the `declare module.exports: any;` line (we favor ES modules over CommonJS modules) and observe the errors. 6. The minimal set of changes to get it working is A) replace 'export' with 'declare export' [7] B) replace `typeof Immutable` with `any` and remove the Immutable import. You can't import types from other libdefs in a libdef [8]. 7. Step 2 created a lot of extra stubs in case we wanted to make a libdef for every single file in `node_modules/remotedev-serialize`. We never import directly from these other files, so it's fine to just put all the type information in a single libdef, as we did in the copy-and-paste in step 5. Delete those extra, unnecessary stubs. [1]: https://flow.org/en/docs/libdefs/#toc-general-best-practices [2]: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Android.20build.3A.20unimodules/near/859855 [3]: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/55ebcedca/types/remotedev-serialize/index.d.ts. [4]: https://github.com/joarwilk/flowgen [5]: zulip#3458 (comment) [6]: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Android.20build.3A.20unimodules/near/845802 [7]: https://flow.org/en/docs/libdefs/creation/ [8]: https://github.com/flow-typed/flow-typed/blob/master/CONTRIBUTING.md#dont-import-types-from-other-libdefs
|
So, we've come back to this issue in discussion today. It was about the lack of support for importing types from third-party code while composing a libdef. In this case, it was about the perfectly valid need to get the
|
The latest version FlowTyped has a libdef for is 6, unfortunately. Ah, well. This is a best effort at replicating the types at react-native-webview/react-native-webview@c4001338c, tagged as v7.6.0, the latest 7.x. An improvement over the FlowTyped libdef is that WebView's props are now exact. So, e.g., people will be warned not to use the `useWebKit` prop, which was removed in 7.0.0. We found it impractical to correctly express the fact that the `WebView` component also accepts all the props that the `View` component does. Certainly it seems impossible to do so by importing things from React Native [1]. So, a partial reimplementation that allows a `style` prop (typed as an inexact object). See the entry in `docs/howto/libdefs.md` added in this commit for more detail. [1]: zulip#3458 (comment)
The latest version FlowTyped has a libdef for is 6, unfortunately. Ah, well. This is a best effort at replicating the types at react-native-webview/react-native-webview@c4001338c, tagged as v7.6.0, the latest 7.x. An improvement over the FlowTyped libdef is that WebView's props are now exact. So, e.g., people will be warned not to use the `useWebKit` prop, which was removed in 7.0.0. We found it impractical to correctly express the fact that the `WebView` component also accepts all the props that the `View` component does. Certainly it seems impossible to do so by importing things from React Native [1]. So, a partial reimplementation that allows a `style` prop (typed as an inexact object). See the entry in `docs/howto/libdefs.md` added in this commit for more detail. [1]: zulip#3458 (comment)
The latest version FlowTyped has a libdef for is 6, unfortunately. Ah, well. This is a best effort at replicating the types at react-native-webview/react-native-webview@c4001338c, tagged as v7.6.0, the latest 7.x. An improvement over the FlowTyped libdef is that WebView's props are now exact. So, e.g., people will be warned not to use the `useWebKit` prop, which was removed in 7.0.0. We found it impractical to correctly express the fact that the `WebView` component also accepts all the props that the `View` component does. Certainly it seems impossible to do so by importing things from React Native [1]. So, a partial reimplementation that allows a `style` prop (typed as an inexact object). See the entry in `docs/howto/libdefs.md` added in this commit for more detail. [1]: zulip#3458 (comment)
We don't need TypeScript ourselves [1], but we want to upgrade `eslint-plugin-jest` soon, and a rather distant dependency has a peer dependency of TypeScript (as we also saw in 01593b3). Here's the output: ``` warning "eslint-plugin-jest > @typescript-eslint/experimental-utils > @typescript-eslint/typescript-estree > [email protected]" has unmet peer dependency "typescript@>=2.8.0 || >= 3.2.0-dev || >= 3.3.0-dev || >= 3.4.0-dev || >= 3.5.0-dev || >= 3.6.0-dev || >= 3.6.0-beta || >= 3.7.0-dev || >= 3.7.0-beta". ``` Ah well, at least we can put it in `devDependencies`. And `yarn why typescript` reveals that `prettier-eslint` and `prettier-eslint-cli` have both already been bringing it in as their dependency. [1] zulip#3458 is open to consider migrating from Flow to TypeScript, but that's not what this commit is about.
We don't need TypeScript ourselves [1], but we want to upgrade `eslint-plugin-jest` soon, and a rather distant dependency has a peer dependency of TypeScript (as we also saw in 01593b3). Here's the output: ``` warning "eslint-plugin-jest > @typescript-eslint/experimental-utils > @typescript-eslint/typescript-estree > [email protected]" has unmet peer dependency "typescript@>=2.8.0 || >= 3.2.0-dev || >= 3.3.0-dev || >= 3.4.0-dev || >= 3.5.0-dev || >= 3.6.0-dev || >= 3.6.0-beta || >= 3.7.0-dev || >= 3.7.0-beta". ``` Ah well, at least we can put it in `devDependencies`. And `yarn why typescript` reveals that `prettier-eslint` and `prettier-eslint-cli` have both already been bringing it in as their dependency. [1] zulip#3458 is open to consider migrating from Flow to TypeScript, but that's not what this commit is about.
We don't need TypeScript ourselves [1], but we want to upgrade `eslint-plugin-jest` soon, and a rather distant dependency has a peer dependency of TypeScript (as we also saw in 01593b3). Here's the output: ``` warning "eslint-plugin-jest > @typescript-eslint/experimental-utils > @typescript-eslint/typescript-estree > [email protected]" has unmet peer dependency "typescript@>=2.8.0 || >= 3.2.0-dev || >= 3.3.0-dev || >= 3.4.0-dev || >= 3.5.0-dev || >= 3.6.0-dev || >= 3.6.0-beta || >= 3.7.0-dev || >= 3.7.0-beta". ``` Ah well, at least we can put it in `devDependencies`. And `yarn why typescript` reveals that `prettier-eslint` and `prettier-eslint-cli` have both already been bringing it in as their dependency. Those libraries are using version 3.9.7, so we set the range to include that version, so we can avoid pulling in a new copy. [1] zulip#3458 is open to consider migrating from Flow to TypeScript, but that's not what this commit is about.
We don't need TypeScript ourselves [1], but we want to upgrade `eslint-plugin-jest` soon, and a rather distant dependency has a peer dependency of TypeScript (as we also saw in 01593b3). Here's the output: ``` warning "eslint-plugin-jest > @typescript-eslint/experimental-utils > @typescript-eslint/typescript-estree > [email protected]" has unmet peer dependency "typescript@>=2.8.0 || >= 3.2.0-dev || >= 3.3.0-dev || >= 3.4.0-dev || >= 3.5.0-dev || >= 3.6.0-dev || >= 3.6.0-beta || >= 3.7.0-dev || >= 3.7.0-beta". ``` Ah well, at least we can put it in `devDependencies`. And `yarn why typescript` reveals that `prettier-eslint` and `prettier-eslint-cli` have both already been bringing it in as their dependency. Those libraries are using version 3.9.7, so we set the range to include that version, so we can avoid pulling in a new copy. [1] zulip#3458 is open to consider migrating from Flow to TypeScript, but that's not what this commit is about.
We don't need TypeScript ourselves [1], but we want to upgrade `eslint-plugin-jest` soon, and a rather distant dependency has a peer dependency of TypeScript (as we also saw in 01593b3). Here's the output: ``` warning "eslint-plugin-jest > @typescript-eslint/experimental-utils > @typescript-eslint/typescript-estree > [email protected]" has unmet peer dependency "typescript@>=2.8.0 || >= 3.2.0-dev || >= 3.3.0-dev || >= 3.4.0-dev || >= 3.5.0-dev || >= 3.6.0-dev || >= 3.6.0-beta || >= 3.7.0-dev || >= 3.7.0-beta". ``` Ah well, at least we can put it in `devDependencies`. And `yarn why typescript` reveals that `prettier-eslint` and `prettier-eslint-cli` have both already been bringing it in as their dependency. Those libraries are using version 3.9.7, so we set the range to include that version, so we can avoid pulling in a new copy. [1] #3458 is open to consider migrating from Flow to TypeScript, but that's not what this commit is about.
…param. When we make our own types based on `StackNavigationProp` -- e.g., when we define `AppNavigationProp`, in an upcoming commit -- we'll want those types to be covariant in their own `RouteName` param. That's because we want, e.g., `AppNavigationProp<'realm'>` to be recognized as a subtype of `AppNavigationProp<'realm' | 'account' | …>`, i.e., `AppNavigationProp<$Keys<AppNavigatorParamList>>`. So, start by making `NavigationProp`, `StackNavigationProp`, and `InexactStackNavigationProp` covariant in their `RouteName` params, following discussion at zulip#4393 (comment). Also make the change for types of navigators other than stack navigators, across all the libdef files. There's a lot of repetition because libdef files can't import types from other files [1]. [1] zulip#3458 (comment)
…param. When we make our own types based on `StackNavigationProp` -- e.g., when we define `AppNavigationProp`, in an upcoming commit -- we'll want those types to be covariant in their own `RouteName` param. That's because we want, e.g., `AppNavigationProp<'realm'>` to be recognized as a subtype of `AppNavigationProp<'realm' | 'account' | …>`, i.e., `AppNavigationProp<$Keys<AppNavigatorParamList>>`. So, start by making `NavigationProp`, `StackNavigationProp`, and `InexactStackNavigationProp` covariant in their `RouteName` params, following discussion at zulip#4393 (comment). Also make the change for types of navigators other than stack navigators, across all the libdef files. There's a lot of repetition because libdef files can't import types from other files [1]. [1] zulip#3458 (comment)
…param. When we make our own types based on `StackNavigationProp` -- e.g., when we define `AppNavigationProp`, in an upcoming commit -- we'll want those types to be covariant in their own `RouteName` param. That's because we want, e.g., `AppNavigationProp<'realm'>` to be recognized as a subtype of `AppNavigationProp<'realm' | 'account' | …>`, i.e., `AppNavigationProp<$Keys<AppNavigatorParamList>>`. So, start by making `NavigationProp`, `StackNavigationProp`, and `InexactStackNavigationProp` covariant in their `RouteName` params, following discussion at zulip#4393 (comment). Also make the change for types of navigators other than stack navigators, across all the libdef files. There's a lot of repetition because libdef files can't import types from other files [1]. [1] zulip#3458 (comment)
…param. When we make our own types based on `StackNavigationProp` -- e.g., when we define `AppNavigationProp`, in an upcoming commit -- we'll want those types to be covariant in their own `RouteName` param. That's because we want, e.g., `AppNavigationProp<'realm'>` to be recognized as a subtype of `AppNavigationProp<'realm' | 'account' | …>`, i.e., `AppNavigationProp<$Keys<AppNavigatorParamList>>`. So, start by making `NavigationProp`, `StackNavigationProp`, and `InexactStackNavigationProp` covariant in their `RouteName` params, following discussion at zulip#4393 (comment). Also make the change for types of navigators other than stack navigators, across all the libdef files. There's a lot of repetition because libdef files can't import types from other files [1]. [1] zulip#3458 (comment)
We've figured for a long time that we'll eventually want to move to TypeScript; it's much better supported for the open-source community than Flow is, and seems to be increasingly the choice that people outside Facebook use and that outside tools best support.
For the Zulip webapp, there's long been a hope to migrate it from plain JS to TypeScript. When that work gets going in earnest, it'll open a quite valuable opportunity to share the maintenance of the types we have in
src/api/to describe the Zulip server's API. To do that effectively, we'll want to be using TypeScript ourselves.Fortunately, the tools for doing this have gotten better in recent years, and it should be a quite reasonable amount of work. The basic plan is:
*.tsfiles, and compiles them as TypeScript instead of Flow. (This only became possible with Babel 7.)tscas a type-checker, as well asflow. (Add it totools/test, and make sure our recommended VS Code config supports it well.)tscentirely.For converting a given file, or swath of files:
{| ... |}to{ ... }; and some low-volume things that are more manual.The text was updated successfully, but these errors were encountered: