-
-
Notifications
You must be signed in to change notification settings - Fork 538
[Tech] No longer use GameManagers as singletons #4894
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: tech/storemanager-classes
Are you sure you want to change the base?
Conversation
arielj
left a comment
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 like this to remove a lot of boilerplate, I left a comment to hopefully remove more
| const { appName, runner } = currentElement.params | ||
| const { folder_name } = gameManagerMap[runner].getGameInfo(appName) | ||
| const { folder_name } = libraryManagerMap[runner] | ||
| .getGame(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 have a helper getGame(runner, appName), because we do this libraryManagerMap[runner].getGame(appName) a looooot
that would also help decoupling how we get the store manager throughout the app, we wouldn't need to "know" that there's a libraryManagerMap hash
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 we do this a lot, but I'd like to resolve this a little differently (possibly in future PRs):
- For everything in the Backend, pass around the
Gameinstead ofappName, runner - For calls from Frontend to Backend, I was thinking that we add something like a special "type" to our IPC wrappers that, if passed from FE to BE, automatically does the equivalent of
getGame(appName, runner)and passes thatGameto the actual listener/handler. Call it aGameHandlemaybe?
15d2bbe to
b5990ed
Compare
Invoking a method on one of the game managers was always a little verbose: Every method took not just its own parameters, but also parameters describing the game's state (effectively that's only an AppName right now, but that may change for other store providers). Instead, there's now one
Gameclass instance for every distinct game in the user's libraryIn practice, code changes in the following way:
Before:
After:
As I mentioned in #4889, I plan on offloading a lot of
GameInfoproperties into individual methods (so there's not as much a burden on game managers to provide everything ingetGameInfoall at once). Increasing the amount of method calls quickly leads to our before approach (the current code) becoming unreadable due to there being so much duplicate work.Requires #4889
Use the following Checklist if you have changed something on the Backend or Frontend: