-
-
Notifications
You must be signed in to change notification settings - Fork 449
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
[Feat/Tech] Migration system #4255
base: main
Are you sure you want to change the base?
Conversation
The generic "Structure" was unnecessary and would've made things harder in the following commits
const migrationComponent = new MigrationComponent() | ||
await migrationComponent.applyMigrations() | ||
|
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.
This is a little awkward, and is a result of porting this to "non-components-system" Heroic. If components would be in main right now, you'd not see any of this here (the component would instead be initialized automatically)
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 have some questions about the components, but there's no PR for that right? to not ask the questions here
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.
There's no PR yet, no. I do have a WIP branch, but it's a little outdated
The reason I haven't made a PR yet is that to properly show what components can do (and to get them some actual use, instead of just being an unused system), I need some preliminary steps (which this PR is one of)
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.
How do you imagine migrations working for a user the opens Heroic for the first time and there are migrations in the list? from the code I understand it will try to run the migrations and then the migration itself has to check if it needs to do something or not?
Because most migration systems I've used tend to have an option that, if you start the project for the first time, you can get the correct state right away and all migrations are marked as already applied since they are not really needed when there's nothing to migrate.
const migrationComponent = new MigrationComponent() | ||
await migrationComponent.applyMigrations() | ||
|
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 have some questions about the components, but there's no PR for that right? to not ask the questions here
Yes, correct. I don't think this will actually be that much of an issue. Heroic is already so modular that basically everything a migration might do would require a check for whether it has to do that task first. |
e5d76e5
to
a5e8778
Compare
Originally part of my components branch, this is a generic system for running something only once per machine. Say we change something persistent (game settings format, file locations, etc.) and now need to update old instances of thing: We'd add a new migration that does the actual upgrade, give it a unique name, add it to the list of migrations, and everything else is taken care of.
I've not gone hunting for use cases of this in the codebase yet, but I can already think of a couple (which I'll add to this PR once implemented, if nothing else than to give an idea on how one might use the system, and to not merge a PR doing nothing)
I would still like a review of the system itself though
Use the following Checklist if you have changed something on the Backend or Frontend: