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

[Tech] Update to Typescript 5.6.3 #3908

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[Tech] Update to Typescript 5.6.3 #3908

wants to merge 1 commit into from

Conversation

CommandMC
Copy link
Collaborator

This required a bunch of little changes across the codebase.
Some were easy (a missing property in a record),
some tedious (adding probably 30 @ts-expect-errors to faulty test types) and
some a little confusing (making something a Record with a literal type as its key allows you to only index into it with values that are of that literal type)

Everything seems to typecheck properly, tests all pass, app works OK as far as I can tell (not that that should be affected at all)


Use the following Checklist if you have changed something on the Backend or Frontend:

  • Tested the feature and it's working on a current and clean install.
  • Tested the main App features and they are still working on a current and clean install. (Login, Install, Play, Uninstall, Move games, etc.)
  • Created / Updated Tests (If necessary)
  • Created / Updated documentation (If necessary)

@CommandMC CommandMC added the pr:ready-for-review Feature-complete, ready for the grind! :P label Aug 7, 2024
@CommandMC CommandMC self-assigned this Aug 7, 2024
@flavioislima
Copy link
Member

This makes me wonder until when we should keep using typescript or maybe just stick to a version forever, updating it without adding value to the type checks might be a wasted time and effort on our end.
Type just for typing I mean, doesnt make sense, it needs to be useful and do not add a lot of complexity and make the code less readable.

@CommandMC
Copy link
Collaborator Author

I do not agree at all. Everything fixed/@ts-expect-error'd here are actual errors of some kind made in the past. Mocks made for tests should not add functionality, the settings system is not type-safe at all, functions not handling inputs correctly that just happened to not happen right now will at some point come back to bite us

Not blaming anyone for those errors of course (heck I bet some of them were made by me), but they are real & should be fixed (or at least checked for & explicitly ignored)

@flavioislima
Copy link
Member

I do not agree at all. Everything fixed/@ts-expect-error'd here are actual errors of some kind made in the past. Mocks made for tests should not add functionality, the settings system is not type-safe at all, functions not handling inputs correctly that just happened to not happen right now will at some point come back to bite us

Not blaming anyone for those errors of course (heck I bet some of them were made by me), but they are real & should be fixed (or at least checked for & explicitly ignored)

Those 'errors' exists because we have TS and it is becoming more "aggressive" with each version, that is what I mean.
type errors not necessary means that it will have build errors or the app will break. It might help catch them ofc, but not always makes sense to write a lot more boilerplate code even though we know that that part won't break .
Only unit and manual tests will be close to 100% safe code.

Sometimes we get ourselves putting a lot of time in figuring out the correct way of typing something and that is what I mean about 'worth' to have it or not.

@CommandMC
Copy link
Collaborator Author

TypeScript is preventing real runtime errors. Let's take an example from this PR: RunnerToLogPrefixMap
This object was missing a nile key, meaning indexing it with a Runner would give you an any instead of a LogPrefix (indexing it at all only is possible because we had suppressImplicitAnyIndexErrors on, but that's besides the point).
Because of that, auto-updating games from Amazon will currently log (xx:xx:xx) INFO: [Backend]: Auto-Updating ..., instead of its intended Nile prefix being used.
Sure, that's the tiniest of problems, but that's just a coincidence. If this would've been a value passed to some 3rd party function instead, it might throw an exception (since undefined is most likely not the value it was expecting to get)

I understand however that sometimes, you might just need to tell TS that you know what you're doing & whatever that might be, it's fine to do. That's why I just @ts-expect-error'd all the test issues in this PR. TS can't know that we use a mock to add functionality that isn't defined on the type, but we know, and we can tell it to ignore the issue.
I do hope that I can reduce that kind of "boilerplate" to a minimum with future improvements (for example, #3927 already makes some of this PR obsolete), but please keep in mind that you are often preventing real errors by writing that extra code (even if these errors might only happen in the future, and even if they may be insignificant)

@arielj
Copy link
Collaborator

arielj commented Aug 11, 2024

I agree on keeping typescript updated and being open to add some magic comment to ignore something when the typing wants to go out of hands (and who knows, maybe someone else wants to address the ignore comment in the future and it's fine too).

In my experience, locking something to an old version eventually create more problems somewhere else, then if we wait months or years to update and at some point we must do it it's way harder in general cause we missed doing it more gradual.

Copy link
Member

@flavioislima flavioislima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Probably the version is outdated by now though.

@CommandMC CommandMC changed the title [Tech] Update to Typescript 5.5.4 [Tech] Update to Typescript 5.6.3 Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:ready-for-review Feature-complete, ready for the grind! :P
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants