Remove dexie version bump requirement for new game additions #1373
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
anttimaki
reviewed
Jun 19, 2024
anttimaki
reviewed
Jun 24, 2024
There was a problem hiding this comment.
I know this is a bit late on the second review round, but did you consider using Version.upgrade to do all this in one fell swoop, thus not requiring run-time checkups?
anttimaki
approved these changes
Jun 24, 2024
anttimaki
requested changes
Jun 25, 2024
There was a problem hiding this comment.
For me this doesn't work as expected, see the comments. I guess valid approaches would be:
- Fix this one way or another to actually persist existing settings upon migration
- Clean out the ineffective data migration code, effectively just resetting the game specific settings
- Fall back using the older approach which didn't use
Version.upgrade()
ethangreen-dev
force-pushed
the
dexie-store-improvements
branch
from
87df8bc
to
49e9b84
Compare
This commit primarily implements a way to track settings without requiring each game to exist as an independent table. This is done by, simply, moving everything to a new `games` table, by which each game is a row, indexable by each's setting identifier. This commit also removes code that migrated the legacy settings store to v2, but don't fret, I've replaced it with migration code that converts v2 game tables to the new v3 row entries. feat: add legacy table clear, remove old migration code chore: resolve PR review comments - Replace occurences of `==` with customary `===`. - Remove transaction wrapper from `applyV2ToV3Migration` - Invert logic of `getLatestGameSpecific` to reduce nesting and catch case where `game === undefined` and `applyV2ToV3Migration === true`. feat: move migration code to `Version.upgrade` This should ideally handle the migration from < 75 versions to 75 automatically at runtime, thus removing the need to integrate the migration routine into `getLatestGameSpecific`. Legacy game tables are not entirely removed, but they are cleared and, thanks to `Version.upgrade`, should not cause any future problems. chore: code cleanup - remove trailing whitespace, imports, debug log
ethangreen-dev
force-pushed
the
dexie-store-improvements
branch
from
49e9b84
to
ffa61c8
Compare
anttimaki
approved these changes
Jun 27, 2024
| const identifier = this.activeGame.settingsIdentifier; | ||
| const game = await this.games.get(identifier); | ||
|
|
||
| if (game !== undefined) { |
There was a problem hiding this comment.
Nitpick: NGL I'm a tiny bit irked that the gameSpecific checks !== undefined while the corresponding code for global settings checks === undefined 🙈
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR primarily implements a way to track settings without requiring each game to exist as an independent table. This is done by, simply, moving everything to a new
gamestable, by which each game is a row, indexable by setting identifier.This also removes code that migrated the legacy settings store to v2, but don't fret, I've replaced it with migration code that converts v2 game tables to the new v3 row entries.