~~Upgrade Flow~~ Migrate to TypeScript

[ysbaddaden]

The currently version of Flow (0.71) lags far behind the current release (0.162). We may consider upgrading to a more recent version.

The upgrade process probably won't be smooth. A quick try at Flow 0.162 for example leads to 915 errors, mostly because Flow now expects almost everything to be statically typed (e.g. full function signatures, including anonymous functions).

Alternative: consider TypeScript, with an automated translation.

[ysbaddaden]

Hey, I was able to convert everything under web/static/js to TypeScript using the flow-to-ts utility with only one warning :shrug:

I'll try to add TypeScript support to Webpack, then check how the type validation goes. It's supposed to be incremental and such, so it may be easier than Flow.

[ysbaddaden]

Adding typescript support to webpack is easy (just need to pin ts-loader to v3.5.0), translating JS with Flow to TS is also easy thanks to flow-to-ts, but getting typescript to check types... is much harder.

I got a few packages installed for TS to know about libraries (e.g. @types/react and @types/lodash), but I had an issue with the web/static/js/decls folder that declares global types for the project in Flow, but TS seems to require per-type .d.ts (or maybe a single global.d.ts)? I don't know, and I couldn't find enough information about solving this in the short allotted time allotted, so I stopped there.

[ysbaddaden]

I spent a little time following some typescript tutorials, such as Migrating from JavaScript. It helped to understand how to handle external type definitions: just use import :facepalm:

I translated a single file, namely actions/panelSurvey.js to TS, then tried to get it to pass TS validation with tsc --noEmit actions/panelSurvey.ts. It involved translating decls/store.js, decls/survey.js and , decls/survey.js. I used flow-to-ts to automate the translation, and TS quickly stopped complaining about missing types (yay).

It then immediately pointed me to an error in the Store type definition: we define store.panelSurvey has being a PanelSurvey when it's actually a DataStore<PanelSurvey>!

Looking closer, the Store type definition is completely out of date (many attributes are missing) and sometimes just plain wrong. It's no wonder that recent versions of Flow started complaining loudly :loudspeaker: There are probably some bugs that Flow or TS can help catch, but I think most of the errors are because of outdated/wrong types.

I'll try again to see if Flow can help us in the same vein that TS did?

[ysbaddaden]

I quickly tried with Flow 0.162 and... it's been an awful experience after trying TS:

1. Flow won't check and report problems for a single file. It analyses and reports everything for the whole project. In order to analyze issues for a single file, I must execute yarn flow --show-all-errors --color=always | less -r then search for the file name. Not nice.

2. Flow complains that each and every functions (including anonymous) don't have an explicit return type. This is likely because of https://flow.org/en/docs/lang/types-first/ leading to the requirement to write awful annotations everywhere (e.g. returning a lambda that returns a promise), even when the return type could be easily inferred.

   TS never asked to specify any return type, and infers the function signature.

Flow eventually reported the same issue that TS reported (Store's outdated type definition), but the information is sadly buried among a bunch of errors, which made it hard to notice, and doesn't help catch errors —unless we manually fix all the issues.

I have a feeling that migrating from Flow to TS would be a nicer and faster experience than upgrading Flow, especially if we can translate the web/static/js/decls folder as a type library, so the types would be globally available, which would avoid having to import them manually in each and every file, that won't be fun —thought maybe we could write a quick & dirty script to automatically inject the imports...

[diegoliberman]

@ysbaddaden excellent analysis! As it seems this will require quite some effort, my only recommendation is to analyze it in the context of the rest of the upgrades that we want to do. Perhaps it's more convenient to start with a different upgrade from the ones that we have in the tech debt backlog.

[diegoliberman]

Removed pending estimation: 32h