feat: add request caching service (#280)#315
Conversation
|
as part of this pr, could you please include test cases to make sure that the request caching service works as intended! |
|
@ethan-tbd I’ve added test cases for the service. Will this be considered part of the same task, or will I receive extra points for it? |
|
|
||
| /// Fetches data from the cache and API. | ||
| Stream<T> fetchData(String url) async* { | ||
| final box = await Hive.openBox(networkCacheKey); |
There was a problem hiding this comment.
instead of opening and closing Box whenever fetchData() is called, we can just have RequestCacheService take in a Box (take a look at PfisNotifier, VcsNotifier for some examples)
| /// Fetches data from the cache and API. | ||
| Stream<T> fetchData(String url) async* { | ||
| final box = await Hive.openBox(networkCacheKey); | ||
| final cacheKey = _generateCacheKey(url); |
There was a problem hiding this comment.
no need to generate a hashed cache key here, we can just use url as the key and have multiple requests to the same url live in the same Box
| // Else, we have already emitted cached data, so we can silently fail or log the error | ||
| } | ||
| } finally { | ||
| await box.close(); |
There was a problem hiding this comment.
no need for this if RequestCacheService is updated to take in a Box
| if (now.difference(cachedTimestamp) < cacheDuration) { | ||
| // Cache is still valid, but we'll fetch new data to check for updates | ||
| // return; // Uncomment this line to skip fetching new data | ||
| } |
There was a problem hiding this comment.
this can just be a parameter in fetchData() that defaults to fetching new data
| import 'package:didpay/shared/services/request_cache_service.dart'; | ||
| import 'package:flutter_test/flutter_test.dart'; | ||
| import 'package:hive/hive.dart'; | ||
| import 'package:hive_test/hive_test.dart'; |
There was a problem hiding this comment.
let's stay away from adding new packages just for testing! once RequestCacheService is updated to take in a Box, you can update this test file to make use of MockBox in mocks.dart (take a look at the test files for PfisNotifier and VcsNotifier and try to mimic the structure of how we have been testing those notifiers)
I created a subissue #318, make sure to be thorough with the test cases and try to follow the existing pattern for how we test Hive boxes! |
|
Hi @aashish-g03 - could you please resolve the above comments so that @ethan-tbd may review/close your PR? Thank you! |
|
Hi @aashish-g03 - If you'd like for your contribution here to count towards Hacktoberfest, please review the comments above so that @ethan-tbd may close and add the |
feat: add tests for request caching service (TBD54566975#280) fix: fix box import and tests for request caching service (TBD54566975#280)
|
@taniashiba @ethan-tbd I've updated the service file according to your suggestions and included some suggestions at the end of service test file to enhance testing. Apologies for the delay—I experienced a sudden increase in office workload that limited my response time. |
3 participants
This PR adds request caching service with different customizable options.
Closes #280.