aissat / easy_localization

Easy and Fast internationalizing your Flutter Apps
https://pub.dev/packages/easy_localization
MIT License
908 stars 325 forks source link

add extra asset loaders #654

Closed hamed-rezaee closed 7 months ago

hamed-rezaee commented 7 months ago

Add multi-asset loader to add more than one asset loader if it is needed, for example, if you want to add package localization to your project.

bw-flagship commented 7 months ago

Can you elaborate on a usecase? Why do you need multiple asset loaders for package localization?

hamed-rezaee commented 7 months ago

Can you elaborate on a usecase? Why do you need multiple asset loaders for package localization?

Hi @bw-flagship

This discussion: https://github.com/aissat/easy_localization/discussions/592

and also here is another usecase:

https://stackoverflow.com/questions/78068216/how-can-i-add-a-localizationsdelegate-form-another-package-that-uses-easylocali

I have a package that uses the EasyLocalization package for localization, and I want to add localizationsDelegate of this package to my main project.

I know that I can add something like this with flutter_intl package to pubspec.yaml:

flutter_intl:
  main_locale: en
  enabled: true
  class_name: MyPackageLocalization

And add the MyPackageLocalization.delegate to my localizationsDelegates inside the MaterialApp:

...
MaterialApp(
  localizationsDelegates: [
    ...context.localizationDelegates,
    MyPackageLocalization.delegate,
  ],
  supportedLocales: context.supportedLocales,
...

But, how can I do it with the EasyLocalization package?

bw-flagship commented 7 months ago

@hamed-rezaee thanks for explaining! I understand the purpose of the PR an since the discussion has at least a few upvotes, it seems to be a useful feature.

Some things prevent me from approving this pr in the current state:

hamed-rezaee commented 7 months ago

@hamed-rezaee thanks for explaining! I understand the purpose of the PR an since the discussion has at least a few upvotes, it seems to be a useful feature.

Some things prevent me from approving this pr in the current state:

  • The breaking change seems unnecessary. You can optionally accept a single asset loader in the constructor and pass it to the list. In v4.0 we could make the breaking change then
  • Instead of (await Future.wait(loaderFutures)).othermethods(), I think await Future.wait(loaderFutures).then((x) => othermethods()) would be more readable
  • Let's avoid the dependency to collection if possible. Each dependency we introduce might cause problems for consumers of the library
  • There seem to be unnecessary formatting changes which make the PR hard to review
  • We need unittests for this new functionality and for the new util

@bw-flagship I will apply the suggested changes and update the PR asap. 👍🏻

hamed-rezaee commented 7 months ago

Hey @bw-flagship this PR is ready for review.