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] Type-check Legendary commands #2918

Merged
merged 5 commits into from Aug 7, 2023
Merged

Conversation

CommandMC
Copy link
Collaborator

@CommandMC CommandMC commented Jul 27, 2023

This PR adds strict type checking to Legendary commands using Zod. Before a command can be ran, the caller of runLegendaryCommand has to verify that, for example, strings that should be non-empty are actually non-empty, numbers are integers & positive (if necessary), paths are valid paths, and so on

Details on how this is achieved / what the design decisions behind things were are in the commit messages

I've only tested this on Linux, I can test it on Windows in the coming days
Seems to be working fine on both Windows & Linux


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)

This wasn't actually used anywhere, and upcoming commits will make Legendary's
runRunnerCommand incompatible with this signature
This allows for stricter type checking (coming in the next commit).
The general idea behind this is:
- each command is represented by an object
- each of those objects has a `subcommand` property containing the name of the
  command, and other required properties for required parameters (for
  example, `launch` requires an `AppName`)
- optional parameters are represented by optional object properties with the
  only valid value of `true`

Base types (seen in `base.ts`) are general-purpose types (representing Paths,
AppNames, URLs, etc.). They don't necessarily have to be in the `legendary`
scope, should we increase our usage of Zod in the future
As mentioned in the previous commit, Legendary commands are now objects.
Representing this required small adaptations basically everywhere, along with
a new function, "commandToArgsArray". This function converts a "new" command
object to the "old" `string[]` representation, which can then be passed to
`callRunner`/used elsewhere.

There are a few rather unsightly things here, most of which stem from the fact
that backwards compatibility has to be kept. Assuming this approach works out,
we can look into applying Zod in other areas of the codebase as well.
@CommandMC CommandMC added the pr:testing This PR is in testing, don't merge. label Jul 27, 2023
@CommandMC CommandMC self-assigned this Jul 27, 2023
@CommandMC CommandMC added pr:ready-for-review Feature-complete, ready for the grind! :P and removed pr:testing This PR is in testing, don't merge. labels Jul 28, 2023
@CommandMC CommandMC requested review from a team, arielj, flavioislima, Nocccer and imLinguin and removed request for a team July 28, 2023 20:13
src/backend/launcher.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@arielj arielj left a comment

Choose a reason for hiding this comment

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

this looks great, I love the idea of not having to pass an array of strings and have the options and validations for each legendary command 👍

Copy link
Collaborator

@arielj arielj 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 to me 👍

@CommandMC CommandMC merged commit 86e2e11 into main Aug 7, 2023
13 checks passed
@CommandMC CommandMC deleted the tech/legendary-typecheck branch August 7, 2023 19:22
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

2 participants