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.

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

Decouple asset loading logic from Localization #56

Merged
merged 5 commits into from
Feb 27, 2020

Conversation

spiritinlife
Copy link
Collaborator

@spiritinlife spiritinlife commented Feb 27, 2020

Library shouldn't make assumptions about asset loading. For example the recent useDocumentStorage shouldn't be added as an option to EasyLocalizationDelegate since its very specific. Allowing the developer to bring their own asset loading logic is the only way to proceed safely.

We provide a default asset loader RootBundleAssetLoader and allow the developer to override this default. By doing this we also remove http dependency and the loadPath if/else logic.
In the example, I have provided asset loaders for Network, File, String, Testing and more can be added.

This also allows developers to do integration testing in their apps since this bug flutter/flutter#44182 right now is an issue for flutter_driver.

 - Helps developer in integration tests
 - Allows developer to use custom loading logic ( network http ( get/post/put etc ), file, rootbundle, string etcc
Copy link

@danilofuchs danilofuchs left a comment

Choose a reason for hiding this comment

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

I really like the approach you took.
It enables many more integration possibilities. Currently, the library assumes way too much responsibility when it comes to data fetching, with not much benefits.

Also fixes #38

lib/easy_localization_delegate.dart Outdated Show resolved Hide resolved
lib/easy_localization_delegate.dart Outdated Show resolved Hide resolved
@spiritinlife
Copy link
Collaborator Author

@danilofuchs We could also just expose load as function on the LocalizationDelegate instead of a class, wasn't sure whats best for this.

@spiritinlife
Copy link
Collaborator Author

@aissat @danilofuchs Loosing the dependency on rootBundle allows us to unit test localization.dart.
I have added some initial tests, take a look.
This could be a start for this #34

@aissat
Copy link
Owner

aissat commented Feb 27, 2020

hi guys this great work, I agree with all code but I have a suggestion to make this code more simple and easy

  • we must hide this code from main.dart
class StringAssetLoader extends AssetLoader {
  @override
  Future<String> load(String localePath) {
    return Future.value('''{"title" : "Hello"}''');
  }
}

class FileAssetLoader extends AssetLoader {
  @override
  Future<String> load(String localePath) {
    File file = File(localePath);
    return file.readAsString();
  }
}

class NetworkAssetLoader extends AssetLoader {
  @override
  Future<String> load(String localePath) async {
    String url =
        "https://raw.githubusercontent.com/aissat/easy_localization/master/example/$localePath";
    return http.get(url).then((response) => response.body.toString());
  }
}

// asset loader to be used when doing integration tests
// default AssetLoader suffers from this issue
// https://github.com/flutter/flutter/issues/44182
class TestsAssetLoader extends AssetLoader {
  @override
  Future<String> load(String localePath) async {
    final ByteData byteData = await rootBundle.load(localePath);
    return utf8.decode(byteData.buffer.asUint8List());
  }
}

i mean i'll make this in easy_localization PKG with keep others customizable
example :

import 'package:path_provider/path_provider.dart';
class FileProvidertLoade extends AssetLoader {
  @override
  Future<String> load(String localePath) {
       Directory appDocDir = await getApplicationDocumentsDirectory();
       String appDocPath = appDocDir.path+"localePath";
      .......
  }
}

@aissat aissat merged commit 19614a5 into aissat:master Feb 27, 2020
@spiritinlife
Copy link
Collaborator Author

@aissat agree on this.
Maybe these AssetLoaders should be given as examples in Readme.md

@aissat
Copy link
Owner

aissat commented Feb 27, 2020

@spiritinlife look last update

@aissat
Copy link
Owner

aissat commented Feb 27, 2020

@aissat @danilofuchs Loosing the dependency on rootBundle allows us to unit test localization.dart.
I have added some initial tests, take a look.
This could be a start for this #34

thanks, @spiritinlife that's great work 👍 💯

@spiritinlife
Copy link
Collaborator Author

spiritinlife commented Feb 27, 2020

@aissat Thank you for your feedback and your awesome work. I checked your latest commits and added some comments.
I don't fully agree but i am ok with it :)
The gist of my comments is that we shouldn't provide any AssetLoader apart from RootBundleAssetLoader and we should drop the dependency to http since its unrelated to the package.

Example AssetLoaders should be provided in Readme so that the user knows how to proceed.
Also loadPath is very specific to NetworkAssetLoader and does not serve any other purpose, thus imho it should be removed completely.
Localization should get the Locale and the base resources path and call load with the proper localePath.

@aissat
Copy link
Owner

aissat commented Feb 27, 2020

@spiritinlife I agree with u about NetworkAssetLoader and StringAssetLoader very logical

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants