Id: Type checking

[bhousel]

Lets look into using Flow to add some type safety to our code. We can start out with the geo and actions modules and expand to other modules. This will almost certainly catch bugs and improve development experience.

cc @kepta @jfirebaugh @tmcw for thoughts?

[HolgerJeromin]

Flow is very good in type guessing but sometimes you have to help him.
If you don't want to change the build pipeline Flow can use a "comment" type style where you can add type annotations into special comments.

[bhousel]

Flow is very good in type guessing but sometimes you have to help him.
If you don't want to change the build pipeline Flow can use a "comment" type style where you can add type annotations into special comments.

Since we already use rollup for building, we should be able to just use a plugin to remove the flow annotations, if I understand it correctly. So thankfully any change to our build process should be minimal.

[Psigio]

Hi - I've created a Proof Of Concept for this (PR at: https://github.com/openstreetmap/iD/pull/4837). I've only added the annotations to a couple of files and updated the build process to include flow but hopefully it should be enough to demonstrate the process. If you are happy with this approach, the annotations could gradually be added to the rest of the application.

[bhousel]

Thanks @Psigio for getting this started!

We can gradually add Flow annotations to the entire codebase. This is a great project for anyone who wants to contribute and learn more about the internals of iD (beginners are very welcome!) .

For anyone wants to help out with this, please tag yourself in a followup comment and claim a folder. 🤗

The work to do is:

1. Read up on Flow's type annotations.
2. For each source file,
   a. add the // @flow flag to the top of the file
   b. add Flow annotations to the functions in the file
3. You can do npm run flow to check the code for any issues. Fix the issues if you want to (or leave them if you're not sure).
4. You can do npm run flow:coverage to see a report about well we're doing.
5. Commit your work, using a commit message like:
   Added Flow annotations to functions in the `xxxx` submodule (re: #3744)
6. Submit a Pull Request when you are finished your folder (or a few folders, if you want to do several).

✋ Don't be afraid to ask for help if you're not sure about something - I've never used Flow before so I am also a beginner.

TODO:

• [ ] actions/ @bhousel
• [ ] behavior/
• [ ] core/
• [x] geo/ @bhousel
• [ ] lib/
• [ ] modes/
• [ ] operations/
• [ ] osm/
• [ ] presets/ @Xavier-J-Ortiz
• [ ] renderer/
• [ ] services/ @Xavier-J-Ortiz
• [ ] svg/
• [ ] ui/
• [ ] ui/fields
• [ ] ui/panels
• [ ] ui/intro
• [ ] util/
• [ ] validations/

[Psigio]

Just to add to this, I was looking at setting up Flow on a couple of the heavily-used types, in particular graph.js, but ran into a few issues due to the use of the constructor function approach. A stackoverflow post suggests that importing the type to other files will not work, and an issue on Flow's github page says:

We recommend using ES6 class syntax

I know that there are many differing opinions on ES6 classes so let's not discuss that here :smile: - I'll see if I can make any headway with the graph.js file as-is with Flow. If anyone else has any experience with this, please feel free to respond!

(EDIT: I originally had entity.js as the name of the file, but I was mistaken - it was graph.js I was trying to annotate, but the net effect is pretty much the same)

[tmcw]

(obviously not really a stakeholder here anymore, but) - though Flow is very nice and works well with the Babel toolchain, I've really seen TypeScript take the lead in the last year or two, and it may well be the better option here, with more potential contributors and better tooling.

[kepta]

I love typescript too and find flow a little immature. But moving to typescript is a significant lift and might need more hands for to help out.

[HolgerJeromin]

As every js file is valid Typescript it can be converted with the same strategy.
The editor support is impressive (i use visualstudio, but the language service is the same in vs code).

[Psigio]

There is a babel plugin that can allow piecemeal conversion of a project to Typescript (https://www.npmjs.com/package/babel-preset-typescript) but I've never used it, so can't really comment further. There's also a rollup plugin which handles running babel on the files (https://github.com/rollup/rollup-plugin-babel). The Flow annotations I've added so far are largely compatible with Typescript (I think that the Type Alias won't work, but they aren't necessary for the Typescript transpiler), so it might be possible just to rename to .ts and run with the babel plugin.

However, we'd still probably be faced with the same issue with constructor functions, so we'd need to also refactor the existing approach to be more class-based. If it ever were decided that this were the right direction to take, then it might make sense to skip Flow and go directly to the Typescript/Javascript hybrid with babel.

[kepta]

I think we should really try hard to avoid babel at all costs. There should be no need for us to rely on Babel and other bloat that comes with facebook-react ecosystem.

The reason I recommend typescript is that it comes with really powerful tooling, which includes the typescript complier. You can pretty much avoid any other tooling and be good with just using it.

[HolgerJeromin]

Why use babel to remove type info and not typescript compiler itself?
https://github.com/Microsoft/TypeScript-React-Conversion-Guide/blob/master/README.md

[Psigio]

I've just tried with the rollup-plugin-typescript (no babel involved!) and it seemed relatively easy to convert the existing Flow annotated file. You can take a look at the diff on my fork at: https://github.com/Psigio/iD/commit/8ce8b03023d7df230b44b74f6ad0703a1e007a9e

This is only a quick Proof Of Concept, but it seems just as easy to implement as the Flow annotations were.

EDIT: The build process doesn't quite work as Type errors don't stop the build (although my editor highlights them, as does running tsc from the command line directly) - I'll try to spend some more time to understand why this is the case.

[Psigio]

I've got a bit further with this - it turns out that the rollup-plugin-typescript didn't quite do what I expected, but I found that https://github.com/ezolenko/rollup-plugin-typescript2 did seem to work better. I've got a work-in-progress branch on my fork, you can see the diff compared to master: https://github.com/Psigio/iD/compare/master...Psigio:wip-1

It works in pretty much the same way as the Flow annotations did, although the error messages aren't quite as nicely formatted in the build output (although they are still sufficient to identify the problem, and if you are using VSCode the editor will automatically highlight the errors before the build is run).

If this was decided to be the way to go, I could clean this up and submit a PR for it. It would probably make sense to include a tslint config and wire that in to the npm run lint task to ensure that as TypeScript files are added they comply with the existing standards. Obviously this could potentially be a big change for the project and would probably entail converting some files to the class approach in order to get the most benefit out of the typing, so the owners might want to have a think about this.

[HolgerJeromin]

You could try "--pretty" compiler option.

Perhaps we should move the discussion to a separate ts issue to avoid spamming this issue if ts is rejected.

[bhousel]

Hey I've had a week to think about this.
I'd like to avoid TypeScript / Babel / ES6 transpilation for now.

I was going to write up a longer response why, but I don't have time. Basically:

• I don't have time to learn a new thing that...
• doesn't affect our users in any way and...
• makes the toolchain even more brittle than it already is (see #4874)

In the end I need to maintain this thing. Nobody has stepped up to add Flow annotations, so let's be honest, nobody will step up to switch the code over to TypeScript either. And if they did, it would bring us no new capability.

The only reason to use TypeScript today is to catch type bugs, which Flow should be able to do (and hasn't yet).

[Psigio]

Makes complete sense - once #4874 is merged back (and when I have some time) I'll see if I can add any more Flow annotations to the files in the utils folder.

[Xavier-J-Ortiz]

@bhousel I'd like to have a crack at this. Will be able to dive in this evening after work as well as weekend.

Should I "claim" a folder or two from the TODO list? If so, I'm just randomly going to choose presets and services.

I'll be brand new to Flow as well, so I'm sure there will be questions from my end.

[bhousel]

Should I "claim" a folder or two from the TODO list? If so, I'm just randomly going to choose presets and services.

Great @Xavier-J-Ortiz - sounds good!

[bhousel]

So, after spending the last week or so on working on adding Flow annotations... I'm pretty ok if we just don't do it.

I'm spending way too much time fighting with the Flow compiler to understand even simple code like this:

type Member = { id: string, type: string, role: string };
type IndexedMember = { id: string, type: string, role: string, index: number };

I was expecting that adding Flow annotations would be like maybe 5 minutes per file, not hours. The list of stuff to do is too long and it's only going to get more complicated from here.

Any serious objections if I just remove the Flow annotations, close this, and return to working on issues that matter to our users?

[Xavier-J-Ortiz]

@bhousel I'm ok with this.

[Psigio]

I'd agree - I've spent a bit of time looking at adding Flow annotations to the tooltip.js file and unfortunately it didn't look like there was an easy way to do it (or at least not one I could see!). I think that the first files I started on were on the simpler side of things so it appeared a lot easier than it might actually be to add annotations to the rest of the project.

[bhousel]

Thanks @Xavier-J-Ortiz and @Psigio 👍

[HolgerJeromin]

I dont know flow in the usage, but the benefit of using typescript is to help programming not finding bugs.
visual studio code knows and not guess what properties the object might have and what functions (own, api, browser) i can use in my code.
I dont know what editors you use and what it helps with js programming.
For me its the difference like having a debugger and using console output only for debugging.