Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Feature -> Develop | Added Local Storage option #43

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Feature -> Develop | Added Local Storage option #43

wants to merge 1 commit into from

Conversation

we-mohd-i001
Copy link
Collaborator

@we-mohd-i001 we-mohd-i001 commented May 23, 2024 •
edited
Loading

Task #9596 - Flutter > VaahFlutter > Research > Add Local Storage (Hive)

Task #9615 - Flutter > VaahFlutter > Research > Add Local Storage (FLutter Secure Storage)

Task #9751 - Flutter > VaahFlutter > Local Storage > Improvement

Time invested: 96:45 | Billable: 96:45 | Non-Billable: 00:00

Problem Statement

  1. The VaahFlutter doesn't have an option for local storage.

Describe The Fix/ Solution You Implemented

  1. I have added a local storage solution with two packages i.e., Hive and Flutter Secure Storage.
  2. An abstract class Storage has been added such that it can be implemented by both Hive and Flutter secure storage.

Proof of your testing (Demo link or video links or image links)

Dependencies
Updated:

Added:

  • hive: ^2.2.3
  • path_provider: ^2.1.3
  • flutter_secure_storage: ^9.1.1

Merge Request Checklist

  • I have performed a self-review of my code
  • My code does follow industry standards
  • My code doesn't produce warnings/ errors for dart analyzer
  • New and existing tests pass locally with my changes (No tests are there as of now)
  • The code modified as part of this PR has been covered with tests
  • My code is properly formatted
  • I've followed the proper naming conventions
  • I have rebased the feature on the latest develop
  • I have run flutter run after the rebase
  • I have added new dependencies/ updated old dependencies
  • I have read all the comments & notes in wireframe and verified that I did take care of that
  • verify that the UI must match with design & wireframe if available
  • I have updated the version/ build (x.x.x+xxx)
  • I've verified that the latest commit of develop exists in your feature branch after rebase.
  • My fix achieves 100% of what is required
  • Can enhance the solution in the future because better implementation could be made

@we-prajapati-c001 we-prajapati-c001 self-requested a review May 27, 2024 05:02
we-prajapati-c001
we-prajapati-c001 requested changes Jun 7, 2024
View reviewed changes
lib/vaahextendflutter/env.dart Outdated
Comment on lines 297 to 311

class HiveConfig {
final String directoryName;
final Future<Directory> _appDocDirectory = getApplicationDocumentsDirectory();

HiveConfig({this.directoryName = 'dir'});

Future<void> init() async {
await _appDocDirectory;
Directory dir = await _appDocDirectory;
await Directory('${dir.path}/dir').create(recursive: true).then((Directory directory) async {
Hive.init(directory.path);
});
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

config are supposed to hold config values only & no methods, having init method here is worst practice. please check sentry config & implementation.

lib/vaahextendflutter/base/base_controller.dart Outdated
@@ -43,6 +43,9 @@ class BaseController extends GetxController {
await PushNotifications.init();
await InternalNotifications.init();
PushNotifications.askPermission();
if (config.hiveConfig != null) {
await config.hiveConfig!.init();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

configs aren't supposed to have functions like this

lib/vaahextendflutter/env.dart Outdated
@@ -208,6 +219,8 @@ enum PushNotificationsServiceType { local, remote, both, none }

enum InternalNotificationsServiceType { pusher, firebase, custom, none }

enum LocalStorageType { hive, flutterSecureStorage, none }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add support for both as well.

lib/vaahextendflutter/env.dart Outdated

class HiveConfig {
final String directoryName;
final Future<Directory> _appDocDirectory = getApplicationDocumentsDirectory();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this in config, it should be in service class or something

final Future<Directory> _appDocDirectory = getApplicationDocumentsDirectory();

lib/vaahextendflutter/env.dart Outdated
final String directoryName;
final Future<Directory> _appDocDirectory = getApplicationDocumentsDirectory();

HiveConfig({this.directoryName = 'dir'});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default value not looking good

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, is this ever used

lib/vaahextendflutter/services/storage/storage.dart Outdated
hive.init();
return hive;
case LocalStorageType.flutterSecureStorage:
return FlutterSecureStorageImpl();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

call init on this too, all classes which implements base class should have init (even if it is empty methos)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refer #43 (comment)

lib/vaahextendflutter/services/storage/storage.dart Outdated
///The argument [name] is used to open Hive box with that name.
///
///If you don't provide [name], 'default' will be used.
factory Storage.createLocal({String name = 'default'}) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove factory constructor & use init.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check

vaahflutter/lib/vaahextendflutter/services/notification/internal/notification.dart

Line 35 in ff155ef

static Future<void> init() async {

https://github.com/webreinvent/vaahflutter/blob/master/lib/vaahextendflutter/base/base_controller.dart#L44

lib/vaahextendflutter/services/storage/storage.dart Outdated
///
///The argument [name] is used to open Hive box with that name.
///
///If you don't provide [name], 'default' will be used.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

format comments properly

lib/vaahextendflutter/services/storage/storage.dart Outdated
/// example:
///```dart
/// Storage.createLocal('name')
/// ```
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

format comments properly

lib/vaahextendflutter/services/storage/storage.dart Outdated
///If the key is already present in the [Storage] it's vlaue will be overwritten.
///```dart
///await storage.create(key: 'key', value: 'value');
/// ```
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

format comments properly

@we-mohd-i001 we-mohd-i001 force-pushed the feature/add-local-storage branch 4 times, most recently from 979592f to f80f573 Compare June 13, 2024 13:32
@we-mohd-i001 we-mohd-i001 force-pushed the feature/add-local-storage branch 2 times, most recently from 5683b8a to dce850a Compare June 27, 2024 12:51
@we-mohd-i001 we-mohd-i001 force-pushed the feature/add-local-storage branch 6 times, most recently from b958cd0 to b05e28b Compare July 19, 2024 13:22
@we-mohd-i001 we-mohd-i001 force-pushed the feature/add-local-storage branch 3 times, most recently from 5813238 to 196c193 Compare July 23, 2024 13:21
we-khushal-y001
we-khushal-y001 requested changes Jul 24, 2024
View reviewed changes
lib/vaahextendflutter/base/base_controller.dart
Comment on lines +52 to +60
if (config.localStorageType == LocalStorageType.hive) {
final Directory appDocDirectory = await getApplicationDocumentsDirectory();
await Directory('${appDocDirectory.path}/vaahflutterdir')
.create(recursive: true)
.then((Directory directory) async {
Hive.init(directory.path);
});
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for better readability, this code should be look like this

 if (config.localStorageType == LocalStorageType.hive) {
        final Directory docsDirectory = await getApplicationDocumentsDirectory();
        final String storagePath = '${docsDirectory.path}/vaahflutterdir';
        final Directory storageDirectory = await Directory(storagePath).create(
          recursive: true,
        );

        Hive.init(storageDirectory.path);
      }

lib/vaahextendflutter/services/storage/local/extendable.dart Outdated
Comment on lines 29 to 30
class LocalStorageExtendable {
static const String defaultCollectionName = 'vaah-fluter-box';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to create extra class, you still can access your custom function and other crud functions by doing this

class CustomStorageService extends LocalStorageWithHive {
  Future<StorageResponse> customFunction(String collectionName, String key) async {
    return read(collectionName: collectionName, key: key);
  }
}

void main() {
  CustomStorageService custom = CustomStorageService();
  custom.deleteAll(collectionName: '');
  custom.customFunction('collection_name', 'key');
}

@we-prajapati-c001
Copy link
Collaborator

Handing this over to @we-kislay-k001 from @we-mohd-i001

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@themodernpk themodernpk themodernpk left review comments

@we-prajapati-c001 we-prajapati-c001 Awaiting requested review from we-prajapati-c001

@we-khushal-y001 we-khushal-y001 Awaiting requested review from we-khushal-y001

Assignees

@we-kislay-k001 we-kislay-k001

Labels
Need To Verify
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@we-mohd-i001 @we-prajapati-c001 @themodernpk @we-khushal-y001 @we-kislay-k001