[Mobile-10] TTL for graphql client's cache #49
Conversation
ios/Podfile
Outdated
| @@ -1,5 +1,5 @@ | |||
| # Uncomment this line to define a global platform for your project | |||
| platform :ios, '12.0' | |||
| platform :ios, '14.0' | |||
There was a problem hiding this comment.
You have to consider if 14.0 is good idea
There was a problem hiding this comment.
@marekkedzia I don't like it, but it's required by Google Maps SDK for iOS. It's been the case for a while, but somehow before the latest flutter update it didn't cause any problems, but now it doesn't build without this specified.
There was a problem hiding this comment.
But it shouldn't be thaaaat bad. As far I've checked it only means dropping support for iPhone 6 and below
| } | ||
|
|
||
| Future<bool> saveTimestamp<T>(QueryResult<T> response) async { | ||
| final stamp = response.timestamp.toUtc().toIso8601String(); |
There was a problem hiding this comment.
create some util function that You will use every time You parse date in project. using .toIso....() might lead to some inaccuracy in future
lib/api_base/ttl/timestamp.dart
Outdated
| @@ -0,0 +1,28 @@ | |||
| import 'ttl_config.dart'; | |||
|
|
|||
| class Timestamp extends DateTime { | |||
lib/api_base/ttl/timestamp.dart
Outdated
| import 'ttl_config.dart'; | ||
|
|
||
| class Timestamp extends DateTime { | ||
| Timestamp.nil(this.key) : super(0); |
There was a problem hiding this comment.
this class is very good aproach. use it to parse date in project too
lib/api_base/ttl/ttl_config.dart
Outdated
| abstract class TtlStrategy { | ||
| static const day = Duration(days: 1); | ||
| static const week = Duration(days: 7); | ||
| static const month = Duration(days: 28); |
There was a problem hiding this comment.
i think this might be missleading. its better to use thirtyDays var so its clear how many days it last. week and day is clear ofc but month is not
lib/api_base/ttl/ttl_config.dart
Outdated
| sciCirclesRepository: month, | ||
| tagsRepository: month, | ||
| mapBuildingsRepository: month, | ||
| departmentsRepository: threeMonths, |
There was a problem hiding this comment.
You have to ask yourself a question: "is GET departmentsRepository so heavy operation that I have to cache it for threeMonths?"
| Future<FetchPolicy> build(TtlKey key) async { | ||
| /// returns FetchPolicy that loads from cache or fetch from network | ||
| final timestamp = await (await repository).getTimestamp(); | ||
| if (await timestamp.isCacheUpToDate) { |
There was a problem hiding this comment.
check if this approach doesnt kill performence in some cases
| buildingsData.valueOrNull?.where(_filterMethod), | ||
| ); // or elsewhere a whole list, filtered by text field | ||
| return buildingsData?.where( | ||
| _filterMethod); // or elsewhere a whole list, filtered by text field |
There was a problem hiding this comment.
im not a fan of comments in code but i see that u use it very common. its ok its not a mistake but in some companies it might be taken as bad practice.
probably in "Pragmatic Programmer" book ive read quote "If You can comment it, You can fix it"
| import '../../../utils/watch_locale.dart'; | ||
| import '../../../utils/where_non_null_iterable.dart'; | ||
| import 'getTags.graphql.dart'; | ||
|
|
||
| part 'tags_repository.g.dart'; | ||
|
|
||
|
|
||
| typedef Tag = Query$GetTags$tags; // just alias for shorter type name |
There was a problem hiding this comment.
this is very good practice, i use similar apporach very common
There was a problem hiding this comment.
Typescript has some special package called ts-opaque, You can try find similar for flutter (or write some simple code yourself)
|
just think about my comments but imo its ready to merge |
|
good work |
|
also now i have noticed that you have very clear git history. it looks very nice in repository |
I added TTL for the cache for our GraphQL client in our
api_basemodule and adjusted its usage across the whole project to slightly different syntax.New usage added to
api_base's README as well.