Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Website: fix any types #3451

Open
fengelniederhammer opened this issue Dec 17, 2024 · 3 comments
Open

Website: fix any types #3451

fengelniederhammer opened this issue Dec 17, 2024 · 3 comments
Labels
codesmell For warnings/lints without immediate harm but to be fixed eventually refactoring Code requires refactoring website Tasks related to the web application

Comments

@fengelniederhammer
Copy link
Contributor

fengelniederhammer commented Dec 17, 2024

#3450 introduced a number of eslint ignore comments for the rule @typescript-eslint/no-explicit-any. any is harmful, because it essentially disables Typescript checks. You can access any method or field without Typescript complaining. In the long term it generates code that is harder to maintain (adding new features and refactoring becomes more dangerous because the type system can't guide you).

All of those should be fixed:

  • If the type is really unknown, we should use unknown and make sure that the variable has a certain type before using it.
  • If the type can be improved, we should replace any by a concrete type that is more useful.

See TODO comments in the code that reference this issue. Possibly split this issue into smaller subtasks.

@fengelniederhammer fengelniederhammer added website Tasks related to the web application refactoring Code requires refactoring codesmell For warnings/lints without immediate harm but to be fixed eventually labels Dec 17, 2024
@corneliusroemer
Copy link
Contributor

In terms of priority, I think this isn't super high up, while it's nice to have everything fully typed, if it's not easy to fix then I don't think we should invest a lot of time.

I can't think of any bugs/runtime type errors we've had due to use of any.

Good point to consider unknown and narrowing as escape hatch though.

@fhennig
Copy link
Contributor

fhennig commented Dec 17, 2024

I had a look at it and saw some things that I touched recently and quickly fixed a few things here: #3464

The remaining things are almost all in the SearchFullUI, and related to the lapisSearchParameters.

I can't think of any bugs/runtime type errors we've had due to use of any.

maybe not, but I think this point is still quite valid:

In the long term it generates code that is harder to maintain (adding new features and refactoring becomes more dangerous because the type system can't guide you).

I find reasoning about the lapisSearchParameters not so easy, because it's not really clear what's happening, there's even this:
https://github.com/loculus-project/loculus/blob/fix-some-any/website/src/utils/search.ts#L253 where the property changes type from string to string[] 😅 So you can't even be sure that a property you thought you knew the type of will have this same type at a different place in the code. I think this is the real, underlying thing to worry about; that missing types (that are not trivial to add) often indicate that the code itself might be difficult to think about, maintain and modify to add features.

I can't speak to the priority of this, just wanted to emphasize the maintainability aspect again (as opposed to a strictly runtime-error-focused PoV).

The unknown type is cool, I didn't know about it before!

@corneliusroemer
Copy link
Contributor

Thanks for fixing some, that's definitely good!

For lapis search parameters, the real issue, I think, is the fact that Zodios spits out opaque types that are hard to work with. It's easy to look at LAPIS docs/swagger and see what the URL should be. It's hard to get a get/post to that URL with Zodios, it's not because of lack of typing, it's often precisely because of typing that it feels hard (at least to me). Zodios seems like a nice experiment but not really something to use in production, we should go with simple react query hooks for which there is plenty of documentation (instead of Zodios which is unmaintained and not too widely used).

Here's an example: Zodios technically just wraps around react query, but in such a weird way that it's easy to forget. Because it's not clear that it's actually react query under the hood we get patterns like this on the left: https://github.com/loculus-project/loculus/pull/3449/files#diff-5c10907a31647a2beff344b171ce8b089c4d29f436e83c764767ae9947d64c93

It's actually strange we need to use useEffect at all here, there shouldn't be any need to do so with react query.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
codesmell For warnings/lints without immediate harm but to be fixed eventually refactoring Code requires refactoring website Tasks related to the web application
Projects
None yet
Development

No branches or pull requests

3 participants