zulip / zulip-mobile

Zulip mobile apps for Android and iOS.
https://zulip.com/apps/
Apache License 2.0
1.28k stars 643 forks source link

Migrate types from Flow to TypeScript (?) #3458

Open gnprice opened 5 years ago

gnprice commented 5 years ago

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:

For converting a given file, or swath of files:

gnprice commented 5 years ago

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". :disappointed:

Maybe if you just don't use that feature, things are better? Like C++.

gnprice commented 5 years ago

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.)

rk-for-zulip commented 4 years ago

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.

gnprice commented 4 years ago

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: https://medium.com/flow-type/what-the-flow-team-has-been-up-to-54239c62004f

The simple and honest explanation is that we were too heads-down on addressing Facebook-internal challenges, and felt too resource-constrained to do much else.

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: https://medium.com/flow-type/coming-soon-changes-to-object-spreads-73204aef84e1 https://medium.com/flow-type/spreads-common-errors-fixes-9701012e9d58

chrisbobbe commented 4 years ago

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 ViewProps type from React Native, to use in a react-native-webview libdef. The WebView component's interface allows passing any props you can pass to a View, and that's impossible to express without duplicating tons of RN code from node_modules. The View props are documented here.

@gnprice said:

So:

  • if you import from outside the declare module, it basically just silently doesn't work, behaving like any
  • if you import from inside the declare module, it gives you [an error] unless the thing you're importing is itself defined by a libdef