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] Make IPC typing simpler & more expandable #3819

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

CommandMC
Copy link
Collaborator

@CommandMC CommandMC commented Jun 13, 2024

IPC type-checking has always been a bit of a hack job, this is a POC to improve it (for both us and our IDEs). Details are in the first commit message.

This will have issues right now, since I did not go through and convert everything to the new system (figured I'd get some opinions first).
This should work fine, if the entire changelist is too overwhelming, you might want to only read until 6915bcb


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)

"new" here is a bit loose, as this inherits most of its type definition from the
.d.ts file, just changing how TS views it.

This replaces our old approach (overwriting electron's types) with a layer on
top of the existing IPC. This should play nicer with IDE intellisense, and
allows us to make changes to IPC very easily in the future (we can now for
example move the `event` parameter to the end of the parameter list passed to
the backend, or pass an abort controller to every handler)

In the process of moving the actual type definitions, I've also corrected a few
mistakes (which weren't caught before, as TS doesn't check .d.ts files)
@CommandMC CommandMC added pr:wip WIP, don't merge. pr:ready-for-review Feature-complete, ready for the grind! :P labels Jun 13, 2024
@CommandMC CommandMC requested a review from a team June 13, 2024 10:53
@CommandMC CommandMC self-assigned this Jun 13, 2024
@CommandMC CommandMC requested review from arielj, flavioislima, Etaash-mathamsetty, Nocccer and imLinguin and removed request for a team June 13, 2024 10:53
`removeApp` does the same thing as `uninstall`, so this can just be a handler
invoker for `uninstall`
This also removes `removeApp`
- This is where they should be (the preload should be as simple as possible)
- This was hiding that the current `updateGame` handler was actually unused
The Frontend already filters this to only registered appNames
Ideally this would be refactored to go in a Zustand store, but one thing at a
time
@CommandMC CommandMC removed the pr:wip WIP, don't merge. label Jul 2, 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.

None yet

1 participant