Fix types and linting errors

[ivan-aksamentov]

Although type checking and linting was implemented in the app from the beginning, we disabled it early so that scientists could iterate faster without being bothered too much with the latest web dev novelties.

We need to apply fixes incrementally, a few files a day. Preferably as a part of the routine PRs.

At some point we can re-enable linting on build-time, but currently you can use these commands to run eslint stylelint tsc and tslint all together in a separate terminal instance:

yarn lint
yarn lint:fix
yarn lint:watch
yarn lint:fix:watch

Or yarn <tool_name>:[fix|watch|fix:watch] for a specific tool.

Current output:

Eslint
512 warnings found.
400 warnings potentially fixable with the `--fix` option.

tsc:
Found 85 errors.

Note that my linting tastes are somewhat peculiar and controversial: there is a whole lot of eslint and tslint plugins and tsc is configured very strictly. I like it this way, but most developers find it annoying.

Feel free to post your thoughts. We can relax the default config, but I'd like to keep my hard-mode one at leas as an option.

PLEASE do not submit one giant merge conflict. It could not be merged and is a waste of everyone's time. ⏰

[ruisaraiva19]

@ivan-aksamentov before starting to re-enable linting on build-time/development-time, we may want to migrate from TSLint to ESLint (TSLint is deprecated).

Regarding linting tastes, I think we can use a more standard config without much nitpicking to be easy for everyone to contribute to the project.

Integrating these tools with the IDE is one easy way to auto-fix issues and improve the developer experience as much as possible.

[ivan-aksamentov]

@ruisaraiva19 The migration is already there. I mean I am using @typescript-eslint/* family of packages in eslint:

https://github.com/neherlab/covid19_scenarios/blob/4df9bd6905e3892b97e8df295594609f025bc141/.eslintrc.js#L24-L26 if that's what you mean.

But I still keep tslint around because @typescript-eslint/* don't yet cover some of the checks that require type information: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/ROADMAP.md

[ivan-aksamentov]

When relaxing the configs make sure you understand that this is a somewhat mission critical project and in the realities of an unrolling pandemic, specialists may in one way or another rely on this tool's predictions. It would be nice for it to be of a slightly higher quality than a typical web app.

Now of course, currently it's a mess. But I would prefer to clean the mess than just disable the checks.

[ruisaraiva19]

By running both ESLint and TSLint, we are gonna have conflicts when we start to apply fixes.

[jsmith]

For type errors, I noticed that d3 types hadn't been included yet. I added these types on a branch and fixed the associated type errors. This reduced the type errors to 63! I've confirmed that yarn dev|prod|test still work and created a PR: #123

[jsmith]

Currently, there is a src/algorithms/models.js file. Would converting this file to a .ts file be included in the scope of this issue?

[ivan-aksamentov]

@jsmith Absolutely! But careful with moving things around. Maybe tests first? Correctness of the results is of utter importance for this project.

[patrikvarga]

Hey @ivan-aksamentov, I see you removed the "help wanted" label 2 hours ago. Is help still needed here? If yes, do you have any suggestions where to start? (As in, which areas/folders would have priority?)

[ivan-aksamentov]

@patrikvarga help is absolutely needed! Make sure you check PRs first though to not do duplicate work.

I removed "help wanted" because all the issues got marked, so the label became useless. But now I see it isn't. Do you know if this label is globally discoverable on GitHub?

How do I mark particularly important issues, like import #11 and autorun #17 so that people start picking those up?

[ivan-aksamentov]

@patrikvarga Regarding priority, no particuar priority other than I will prefer merging a feature or a bugfix PR that also addresses linting and types compared to a PR that just addresses linting and types. I mean in terms or merge conflicts. Lint and type fixes tend to be quite noisy.

[patrikvarga]

@ivan-aksamentov

Do you know if this label is globally discoverable on GitHub?

Yes, they are: https://github.com/issues?q=is%3Aopen+is%3Aissue+label%3A"help+wanted" I suppose this might be one reason to keep the label?

Note: to me it didn't make a difference: I was looking at this repo to see where to help, and I found this repo via GitHub topic search. Only when I was looking at issues here did I notice the "help wanted" labels, so I thought those are more important to you & maintainers.

How do I mark particularly important issues, like import #11 and autorun #17 so that people start picking those up?

I suggest a simple red label "important" :) The title prefix that you're already doing works too, I think. People who don't want to miss it won't. :)

[patrikvarga]

@patrikvarga Regarding priority, no particuar priority other than I will prefer merging a feature or a bugfix PR that also addresses linting and types compared to a PR that just addresses linting and types. I mean in terms or merge conflicts. Lint and type fixes tend to be quite noisy.

@ivan-aksamentov Thanks, that makes sense. As a "good first issue", I'll be probably looking at easy wins, i.e. still linting-only/typing-only changes (small ones of course).

[patrikvarga]

@ivan-aksamentov See a few trivial PRs that I opened as to what I meant by "small changes", trying to not interfere with any other PRs or anyone's feature work.

[ivan-aksamentov]

@patrikvarga Thanks Patrick! I really enjoyed your focused PRs. It's a pleasure to see a green merge button on a PR, and yours were all green :) Also these have improved quality of code significantly. I am looking forward to see more of your contributions!