-
-
Notifications
You must be signed in to change notification settings - Fork 530
[Tech] Make LibraryManagers and GameManagers actual classes #4889
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
base: main
Are you sure you want to change the base?
Conversation
Seems like the diff isn't great even with "Hide whitespace" turned on. I'll see if I can split those changes out into (1) code changes and (2) linting |
4e95557
to
a927d23
Compare
libraryManagerMap['legendary'].getGameOverride() | ||
) | ||
addHandler('getGameSdl', async (event, appName) => | ||
libraryManagerMap['legendary'].getGameSdl(appName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we can export each manager too, not just the hash, then we can import legendaryLibraryManager
to not have to import the hash and access with a hardcoded 'legendary'
string, it would make the code more declarative and easier to understand in my opinion (I know it's a small difference, but I think it will help since it's used so much throughout the app, there's many places we just have hardcoded runners)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this would be a good idea for now. I do have a concern: I'd like to eventually get rid of every one of these hardcoded runner-specific library manager invocations (my end goal is to dynamically enable/disable library integrations at runtime; to load them as "plugins"). Obviously exporting individual library managers would go against that goal
So, I can definitely do this for now (and if you'd like me to, I would). I just don't see it as that much of a problem right now, and given these areas will change relatively soon-ish anyways (I have a draft of this plugins idea around already) I hadn't bothered to change it yet
Note
Do not be alarmed by the huge amount of changed lines in this PR. Most of it is whitespace changes (since indentation changes from 2 to 4 spaces). The main changes in this PR are in bb0a140, whitespace changes are in a927d23 (so ignore that one)
Library & game managers were long used as pseudo-classes (having a defined structure & being used like a class instance, but not actually being a class). This causes various issues:
gameManagerMap
/libraryManagerMap
declaration (or worse, in some other file expecting some extra method), there's nothing pointing you to the error in the specific manager implementationI am looking to make core changes to the game & library managers (mainly getting rid of or at least significantly slimming down the
GameInfo
interface) to make it easier to implement more store providers. This PR is the first step towards thatI did make one "functional" change here: I've removed a test file for Legendary's save sync folder computation. I don't think this was working correctly (the test was too specialized, it might've helped more now that the helper binary stubs are in), and I want to change how save sync folders work soon anyways
Use the following Checklist if you have changed something on the Backend or Frontend: