refactor(mobile): split store into repo and service #16199
Conversation
alextran1502
requested review from
zackpollard,
jrasm91,
danieldietzler and
mertalev
| @@ -0,0 +1,5 @@ | |||
| abstract interface class IDatabaseRepository { | |||
| Future<T> nestTxn<T>(Future<T> Function() callback); | |||
There was a problem hiding this comment.
Is it typical to abbreviate function names in dart land? Usually I am against that, as it makes it harder to read for no benefit.
There was a problem hiding this comment.
Is it typical to abbreviate function names in dart land? Usually I am against that, as it makes it harder to read for no benefit.
Yes. But we can follow a similar convention to the server codebase in our mobile codebase as well. Will update all the methods to not use abbreviations
There was a problem hiding this comment.
The StoreKey enum is supposed to be defined in this file. I actually handled it in the next PR to minimise the number of places that needs updating
| final int? intValue; | ||
| final String? strValue; |
There was a problem hiding this comment.
Again, probably lack of dart knowledge, but (1) having the data type in the field name kind of sucks and (2) as I understand it either of them will always be undefined? If that's the case, wouldn't a generic make more sense? (or only storing the string and parsing it, idk)
| const (int) => entity.intValue, | ||
| const (String) => entity.strValue, | ||
| const (bool) => entity.intValue == 1, | ||
| const (DateTime) => entity.intValue == null | ||
| ? null | ||
| : DateTime.fromMillisecondsSinceEpoch(entity.intValue!), | ||
| const (User) => await UserRepository(_db).get(entity.strValue!), |
There was a problem hiding this comment.
Yeah this kind of stuff is IMO just ugly, so I'd probably try to find a better data model for the entity as mentioned earlier.
There was a problem hiding this comment.
True, but we are storing KV pairs inside a relational database which will always be ugly. We can instead store the entire setting as a single JSON string and parse it similar to the server SystemConfig
There was a problem hiding this comment.
I think I would prefer that, am interested for opinions of the others though.
There was a problem hiding this comment.
I think it is okay to keep the implementation, as I think we will have a different way of storing KV in sqlite anyway
There was a problem hiding this comment.
That is fair. We can revisit it then :)
mobile/lib/utils/migration.dart
Outdated
| if (version < targetVersion) { | ||
| _migrateTo(db, targetVersion); | ||
| final int version = StoreOld.getOldValue(StoreKey.version, 1)!; | ||
| if (version < 9) { |
There was a problem hiding this comment.
version < _ktargetVersion? Isn't it the same as the if below?
There was a problem hiding this comment.
Oh the below if block is used to clear the tables and re-sync them whenever there is a change in the targetVersion. The new if block is to only perform the migration once when the user comes from a version lesser than 9
shenlong-tanwen
force-pushed
the
refactor/store-domain
branch
from
f63d67f to
0539037
Compare
|
@shenlong-tanwen |
All the pending comments should be resolved now. |
Changes made
The change is split into multiple PRs. This handles the following:
The following changes are to be made in the successive PRs: