Conversation
…pported locales data and setting the device and saved locale.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (58)
WalkthroughRefactors asset-loading to named-parameter locale loading, injects ChangesLocalization v4 migration
Sequence Diagram(s)sequenceDiagram
participant Main
participant ELApp as EasyLocalization Widget
participant CTL as EasyLocalizationController
participant Storage as IEasyLocalizationStorage
participant Loader as AssetLoader
Main->>ELApp: ensureInitialized(assetLoader, storage)
ELApp->>CTL: initEasyLocation(assetLoader, storage)
CTL->>Storage: init()
CTL->>Storage: getValue('locale')
Storage-->>CTL: saved locale or null
CTL->>Loader: load({locale})
Loader-->>CTL: translations Map
CTL->>Storage: setValue('locale', ...)
CTL-->>ELApp: supportedLocales ready
ELApp->>Main: initialized
Note over ELApp: setLocale() uses injected loader & storage
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
example/lib/lang_view.dart (1)
Line range hint
39-43: Avoid hardcoded indices for locale selection.Using hardcoded indices (
context.supportedLocales[1]) is fragile and could break if the locale order changes.Consider using locale codes instead:
- locale: context.supportedLocales[1] //BuildContext extension method + locale: context.supportedLocales.firstWhere( + (l) => l.languageCode == 'ar', + orElse: () => context.supportedLocales.first, + )
🧹 Nitpick comments (16)
lib/src/easy_localization_controller.dart (4)
3-3: Remove or clarify commented-out import
The newly added imports for path utilities, asset loader, and storage interface align with the refactor. However, the commented-outSharedPreferencesimport may cause confusion if left unused. Consider removing it to keep the codebase clean.Also applies to: 8-8, 11-13
18-19: Revisit mutability of_storage
If the storage implementation isn’t meant to change at runtime, consider making_storagealate finalto prevent accidental reassignment. This clarifies intent and may reduce the chance of concurrency issues.
154-158: Remove lingeringSharedPreferencesreferences
The commented-out code suggests a previous approach to locale persistence. Since storage is now handled via_storage, removing these lines can reduce clutter and confusion.
189-240: Clarify asset-based loading for cross-platform usage
- Reading locales from a directory via
dart:iois incompatible with platforms like web. Ensure that this path-based approach is either gated by platform checks or supplemented with a fallback.- Throwing
io.PathExistsExceptionmight be misleading; a custom exception or a more descriptive message can improve debugging.lib/src/easy_localization_storage_interface.dart (1)
1-19: Add typed getters to avoid runtime type errors
Using a generic methodgetValue<T>()is flexible but may introduce runtime type mismatches. Consider providing typed methods (e.g.,getString()) or robust runtime checks. Also ensureclose()is reliably invoked to release resources (if needed).test/root_bundle_asset_loader_test.dart (1)
5-6: Remove TODO comment.The comment "Replace with your package and file name" should be removed as it's no longer needed.
test/easy_localization_ctl_test.dart (1)
7-15: Consider using a mock logger instead of print override.While the print override works, using a mock logger would be more maintainable and allow for better verification of log messages.
Consider this approach:
class MockLogger { final List<String> logs = []; void debug(String message) => logs.add(message); } final mockLogger = MockLogger(); // Inject mockLogger into EasyLocalizationControllerexample/lib/lang_view.dart (1)
101-105: Add semantic labels for accessibility.The language selection tiles should have semantic labels for better accessibility.
Add semantics to the ListTile:
child: ListTile( dense: true, + semanticLabel: 'Select $title language', title: Text( title, ),example/lib/main.dart (2)
26-35: Consider removing commented-out configuration options.The commented options for
fallbackLocaleandstartLocaleshould either be implemented or removed to maintain clean code.
41-41: Consider removing debug print statement.The debug print statement for supported locales should be removed before production deployment.
lib/src/easy_localization_app.dart (1)
104-109: Remove commented-out code.The commented-out properties should be removed as they're now handled by the controller.
test/easy_localization_context_test.dart (1)
43-44: Consider adding error handling test cases for asset loader initialization.The initialization of EasyLocalization with RootBundleAssetLoader should include test cases for error scenarios, such as invalid paths or missing assets.
await EasyLocalization.ensureInitialized( - assetLoader: const RootBundleAssetLoader('i18n')); + assetLoader: const RootBundleAssetLoader('i18n')).catchError((error) { + expect(error, isA<AssetLoaderException>()); + });test/easy_localization_test.dart (1)
75-85: Remove commented code to improve readability.The commented parameters are no longer needed with the new initialization pattern.
var r1 = EasyLocalizationController( forceLocale: const Locale('en'), - // path: 'path/en.json', - // supportedLocales: const [Locale('en')], useOnlyLangCode: true, useFallbackTranslations: false, saveLocale: false, onLoadError: (FlutterError e) { log(e.toString()); }, - // assetLoader: const JsonAssetLoader()test/easy_localization_widget_test.dart (1)
71-78: Consider enabling logger levels for debugging.The commented logger levels could be useful for debugging. Consider keeping them enabled in test environment.
TestWidgetsFlutterBinding.ensureInitialized(); SharedPreferences.setMockInitialValues({}); - // EasyLocalization.logger.enableLevels = <LevelMessages>[ - // LevelMessages.error, - // LevelMessages.warning, - // ]; + EasyLocalization.logger.enableLevels = <LevelMessages>[ + LevelMessages.error, + LevelMessages.warning, + ];i18n/ar/ar.json (1)
1-11: Add metadata and more comprehensive translations.Consider enhancing the JSON file with:
- Metadata about the language and region
- More comprehensive test cases
- Documentation for pluralization rules
{ + "_metadata": { + "language": "Arabic", + "country": "General", + "pluralization_rules": "Arabic has six plural forms" + }, "test": "اختبار", "day": { "zero": "{} يوم", "one": "{} يوم", "two": "{} أيام", "few": "{} أيام", "many": "{} يوم", "other": "{} يوم" } }example/pubspec.yaml (1)
Line range hint
31-34: Address the commented loader package dependency issue.There's a FIXME comment regarding the
easy_localization_loaderpackage dependencies issue that needs to be resolved.Would you like me to help investigate and resolve the dependency issue with the
easy_localization_loaderpackage? I can help create an issue to track this.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
example/lib/generated/codegen_loader.g.dartis excluded by!**/generated/**
📒 Files selected for processing (21)
bin/generate.dart(1 hunks)example/lib/lang_view.dart(3 hunks)example/lib/main.dart(3 hunks)example/linux/flutter/CMakeLists.txt(1 hunks)example/pubspec.yaml(2 hunks)example/test/widget_test.dart(1 hunks)i18n/ar/ar.json(1 hunks)i18n/ar_en/ar.json(1 hunks)i18n/ar_en/en.json(1 hunks)lib/easy_localization.dart(1 hunks)lib/src/asset_loader.dart(1 hunks)lib/src/easy_localization_app.dart(7 hunks)lib/src/easy_localization_controller.dart(4 hunks)lib/src/easy_localization_storage_interface.dart(1 hunks)pubspec.yaml(1 hunks)test/easy_localization_context_test.dart(1 hunks)test/easy_localization_ctl_test.dart(1 hunks)test/easy_localization_test.dart(7 hunks)test/easy_localization_widget_test.dart(9 hunks)test/root_bundle_asset_loader_test.dart(1 hunks)test/utils/test_asset_loaders.dart(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- example/test/widget_test.dart
- i18n/ar_en/en.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (25)
lib/src/easy_localization_controller.dart (5)
1-2: Ensure platform compatibility when usingdart:io
While this import is necessary for reading directories on platforms that support I/O, be aware that web builds don’t fully supportdart:io. If web compatibility is intended, consider a conditional import or alternative approach.
26-29: Initializelatefields thoroughly
Ensure_assetLoaderand_localeare always assigned during initialization. If the controller initialization is skipped in certain conditions (e.g., testing), referencing these fields prematurely can throw runtime errors.
102-117: Validate fallback translation flow
When loading fallback translations, the code merges base and fallback data. This approach appears correct, but keep in mind that_listtranslationDatais purged after its first use, forcing subsequent calls to rely solely on_assetLoader. Verify that this matches your intended behavior for multi-locale or repeated fallback scenarios.
162-185: Confirm one-time vs. repeatedinitEasyLocationcalls
This static method sets_storageand_assetLoaderglobally. IfinitEasyLocationcan be invoked multiple times, ensure that repeated calls won’t overwrite existing values or inadvertently reset partially loaded data.
241-241: Locale deletion confirmed
Removing the saved locale updates both the in-memory state and_storage. If an application flow requires resetting to a fallback or re-initializing after deletion, consider prompting re-initialization or reloading to avoid unexpected locale states.Also applies to: 244-244
lib/easy_localization.dart (1)
7-7: Export interface for custom storage
Exportingeasy_localization_storage_interface.dartallows external code to implement custom storage strategies, extending the library’s flexibility without tying users to a single persistence layer.test/easy_localization_ctl_test.dart (1)
50-51: Add test cases for core functionality.The TODO comment indicates missing test cases. Based on the PR objectives, tests should be added for:
- Asset loading initialization
- Storage interface integration
- Cached asset loader functionality
Let me help verify what needs to be tested:
✅ Verification successful
Add test cases for core functionality
The review comment is accurate. The codebase shows several untested core components:
- Asset Loaders:
- CodegenLoader
- ImmutableAssetLoader
- ImmutableJsonAssetLoader
- JsonAssetLoader
- Storage interface implementation and integration
- EasyLocalizationController functionality (currently has commented-out test cases)
The existing test coverage only covers RootBundleAssetLoader and basic Localization.load() scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Find core functionality that needs test coverage echo "Searching for core functionality..." rg -t dart "class.*Storage|class.*AssetLoader|class.*Controller" --no-filename # Find existing test coverage echo -e "\nSearching for existing tests..." rg -t dart "^.*test.*Storage|^.*test.*AssetLoader|^.*test.*Controller" --no-filenameLength of output: 840
Script:
#!/bin/bash # Find all test files echo "All test files:" fd -e dart "_test.dart$" echo -e "\nController implementation:" ast-grep --pattern 'class EasyLocalizationController { $$$ }' echo -e "\nExisting test cases (broader search):" rg -t dart "test.*\b(Storage|AssetLoader|Controller|load|save|init)" -A 2Length of output: 2722
test/utils/test_asset_loaders.dart (3)
5-14: LGTM! Well-structured test utility class.The
ImmutableAssetLoaderprovides a clean, minimal implementation suitable for testing scenarios.
17-25: LGTM! Constructor and method signature updates align with new interface.The changes properly implement the new AssetLoader interface while maintaining the test utility's immutable nature.
Line range hint
29-103: LGTM! Comprehensive test data with proper null safety.The changes properly implement the new AssetLoader interface while maintaining a robust test data structure. The null-safe access to
locale?.languageCodeis a good practice.example/lib/main.dart (2)
Line range hint
11-24: LGTM! Proper initialization setup with asset loader configuration.The initialization is correctly configured with the new
ensureInitializedmethod, providing a clear path for asset loading.
47-47: LGTM! Material 3 adoption.Good practice to adopt Material 3 for modern UI components and features.
lib/src/easy_localization_app.dart (3)
65-67: Verify impact of changing saveLocale default to false.This change might affect existing applications that rely on locale persistence. Consider documenting this breaking change in migration guide.
82-87: LGTM! Well-structured initialization method.The
ensureInitializedmethod properly handles both assetLoader and storage initialization.
171-172: LGTM! Proper centralization of locale management.Good practice to centralize supported locales in the controller and validate against it.
Also applies to: 194-194
bin/generate.dart (1)
247-247: LGTM! Method signature properly updated for null safety.The
loadmethod correctly implements the new AssetLoader interface with proper null safety.test/easy_localization_context_test.dart (2)
66-236: 🛠️ Refactor suggestionConsider keeping essential test cases from commented code.
While removing SharedPreferences dependency is good, some of the commented test cases cover important functionality like fallback locale and device locale handling. Consider preserving these tests with the new storage interface.
60-61: Verify test coverage for supported locales.The test only checks the length of supported locales but not their specific values. Consider adding more detailed assertions.
expect(_context.supportedLocales.length, 4); +expect(_context.supportedLocales, containsAll([ + const Locale("ar", "DZ"), + const Locale("en"), + const Locale("ar"), + const Locale("en", "US") +]));✅ Verification successful
The review suggestion is accurate and should be implemented
The suggested assertion precisely matches the actual supported locales found in the i18n directory:
- ar-DZ
- ar
- en
- en-US
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # First, let's look at the test file content cat test/easy_localization_context_test.dartLength of output: 8339
Script:
#!/bin/bash # Look for locale definitions in the source code rg -l "supportedLocales" --type dart # Also check for any i18n configuration files fd "i18n"Length of output: 414
Script:
#!/bin/bash # Check i18n directory content ls -la i18n/ # Look at the initialization code in the main implementation file cat lib/src/easy_localization_app.dartLength of output: 7980
test/easy_localization_test.dart (1)
26-48: LGTM! Good addition of asset loader test cases.The new test group for asset loader properly verifies error cases for path and supported locales.
i18n/ar_en/ar.json (1)
1-11: Verify Arabic pluralization rules and placeholder consistency.The JSON structure looks good, but there are a few points to consider:
- The placeholder
{}is consistently used across all plural forms- The plural forms follow Arabic grammar rules with appropriate forms for zero, one, two, few, many, and other cases
Let's validate the JSON structure and encoding:
✅ Verification successful
Arabic pluralization rules and placeholders are correctly implemented ✅
The plural forms and placeholders are consistent across all Arabic translation files and follow the correct Arabic grammar rules.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify JSON validity and UTF-8 encoding file="i18n/ar_en/ar.json" if [ -f "$file" ]; then # Check JSON syntax jq '.' "$file" > /dev/null # Verify UTF-8 encoding file -i "$file" | grep -q "charset=utf-8" # Print the normalized content for visual inspection jq -r '.' "$file" fiLength of output: 430
Script:
#!/bin/bash # Check for other Arabic translation files and references echo "Looking for other Arabic translation files:" fd -e json . i18n | grep -i "ar" echo -e "\nChecking for files referencing Arabic translations:" rg -l "ar\.json" --type ts --type jsLength of output: 519
Script:
#!/bin/bash # Compare plural rules across Arabic translation files echo "=== Standard Arabic (ar.json) ===" jq -r '.day | select(.)' i18n/ar.json 2>/dev/null || echo "No 'day' key found" echo -e "\n=== Algerian Arabic (ar-DZ.json) ===" jq -r '.day | select(.)' i18n/ar-DZ.json 2>/dev/null || echo "No 'day' key found" echo -e "\n=== Arabic in ar/ directory ===" jq -r '.day | select(.)' i18n/ar/ar.json 2>/dev/null || echo "No 'day' key found" echo -e "\n=== Arabic in ar_en/ directory (current file) ===" jq -r '.day | select(.)' i18n/ar_en/ar.json 2>/dev/null || echo "No 'day' key found"Length of output: 1023
pubspec.yaml (3)
8-8: Version bump indicates breaking changes.The version change to
4.0.0-dev.0correctly follows semantic versioning for breaking changes, as indicated in the PR objectives regarding initialization changes.
51-55: Asset paths align with new localization structure.The flutter assets section correctly includes the new i18n directory structure, which supports the improved asset loading mechanism mentioned in the PR objectives.
33-33: Verify compatibility impact of SDK constraint.The SDK constraint has been updated to require Dart
>=3.1.0. This is a significant change that might affect existing users.✅ Verification successful
SDK constraint change aligns with existing Dart 3 support ✅
The package already supports Dart 3 since version 3.0.2 as documented in the changelog, making this constraint change a formalization of existing compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check pub points and SDK compatibility curl -s "https://pub.dev/api/packages/easy_localization" | \ jq -r '.versions[] | select(.version | contains("3.")) | .pubspec.environment.sdk' | \ sort -V | tail -n 1Length of output: 179
Script:
#!/bin/bash # Check for migration guides and changelog entries echo "=== Checking for CHANGELOG.md ===" if [ -f CHANGELOG.md ]; then head -n 20 CHANGELOG.md fi echo -e "\n=== Checking for migration guides ===" fd -t f -e md migration fd -t f -e md guide echo -e "\n=== Checking for Dart 3.0 related changes ===" rg -i "dart 3" -A 2 -B 2Length of output: 942
example/linux/flutter/CMakeLists.txt (1)
85-85: LGTM! Platform-agnostic build configuration.The change to use
${FLUTTER_TARGET_PLATFORM}makes the build configuration more flexible and platform-agnostic.example/pubspec.yaml (1)
52-54: Resource paths support multiple localization formats.The addition of paths for JSON, YML, and XML formats aligns with the improved asset loading mechanism. However, ensure that the corresponding loader implementations are available and tested.
Consider documenting the supported formats and their usage in the migration guide.
| /// Abstract class for loading assets. | ||
| abstract class AssetLoader { | ||
| const AssetLoader(); | ||
| Future<Map<String, dynamic>?> load(String path, Locale locale); | ||
| /// Path to the assets directory. | ||
| /// Example: | ||
| /// ```dart | ||
| /// path: 'assets/translations', | ||
| /// path: 'assets/translations/lang.csv', | ||
| /// ``` | ||
| final String? path; | ||
|
|
||
| /// List of supported locales. | ||
| /// {@macro flutter.widgets.widgetsApp.supportedLocales} | ||
| final List<Locale>? supportedLocales; | ||
|
|
||
| /// Constructor for [AssetLoader]. | ||
| /// | ||
| /// [path] is the path to the assets directory. | ||
| /// [supportedLocales] is a list of locales that the assets support. | ||
| const AssetLoader({this.path, this.supportedLocales}) | ||
| : assert(path != null || supportedLocales != null, | ||
| 'path or supportedLocales must not be null'); | ||
|
|
||
| /// Loads the assets for the given [locale]. | ||
| /// | ||
| /// Returns a map of loaded assets. | ||
| Future<Map<String, dynamic>> load({Locale? locale}); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making locale required in load() method.
While the class structure and documentation look good, making locale optional in the load() method seems inconsistent since implementations like RootBundleAssetLoader require it and throw an error if it's null.
Consider updating the method signature to:
- Future<Map<String, dynamic>> load({Locale? locale});
+ Future<Map<String, dynamic>> load({required Locale locale});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Abstract class for loading assets. | |
| abstract class AssetLoader { | |
| const AssetLoader(); | |
| Future<Map<String, dynamic>?> load(String path, Locale locale); | |
| /// Path to the assets directory. | |
| /// Example: | |
| /// ```dart | |
| /// path: 'assets/translations', | |
| /// path: 'assets/translations/lang.csv', | |
| /// ``` | |
| final String? path; | |
| /// List of supported locales. | |
| /// {@macro flutter.widgets.widgetsApp.supportedLocales} | |
| final List<Locale>? supportedLocales; | |
| /// Constructor for [AssetLoader]. | |
| /// | |
| /// [path] is the path to the assets directory. | |
| /// [supportedLocales] is a list of locales that the assets support. | |
| const AssetLoader({this.path, this.supportedLocales}) | |
| : assert(path != null || supportedLocales != null, | |
| 'path or supportedLocales must not be null'); | |
| /// Loads the assets for the given [locale]. | |
| /// | |
| /// Returns a map of loaded assets. | |
| Future<Map<String, dynamic>> load({Locale? locale}); | |
| /// Abstract class for loading assets. | |
| abstract class AssetLoader { | |
| /// Path to the assets directory. | |
| /// Example: |
| /// The `RootBundleAssetLoader` class is a subclass of `AssetLoader` that uses Flutter's asset loader | ||
| /// to load localized JSON files. | ||
| /// | ||
| class RootBundleAssetLoader extends AssetLoader { | ||
| const RootBundleAssetLoader(); | ||
| // A custom asset loader that loads assets from the root bundle | ||
|
|
||
| String getLocalePath(String basePath, Locale locale) { | ||
| return '$basePath/${locale.toStringWithSeparator(separator: "-")}.json'; | ||
| const RootBundleAssetLoader(String path) : super(path: path); | ||
|
|
||
| /// Returns the path for the specified locale | ||
| /// | ||
| /// The [locale] parameter represents the desired locale. | ||
| /// The returned path is based on the [path] of the asset loader | ||
| /// and the [locale] with a separator ("-") between language and country. | ||
| String getLocalePath(Locale locale) { | ||
| return '$path/${locale.toStringWithSeparator(separator: "-")}.json'; | ||
| } | ||
|
|
||
| /// | ||
| /// Loads the localized JSON file for the given `locale`. | ||
| /// | ||
| /// Throws an `ArgumentError` if the `locale` is `null`. | ||
| /// | ||
| @override | ||
| Future<Map<String, dynamic>?> load(String path, Locale locale) async { | ||
| var localePath = getLocalePath(path, locale); | ||
| Future<Map<String, dynamic>> load({Locale? locale}) async { | ||
| if (locale == null) throw ArgumentError.notNull('locale'); | ||
| var localePath = getLocalePath(locale); | ||
| EasyLocalization.logger.debug('Load asset from $path'); | ||
|
|
||
| // Load the asset as a string and decode it as JSON | ||
| return json.decode(await rootBundle.loadString(localePath)); | ||
| } |
There was a problem hiding this comment.
Add error handling for JSON parsing and file loading.
The implementation should handle potential errors when loading and parsing JSON files.
Consider adding try-catch blocks:
@override
Future<Map<String, dynamic>> load({Locale? locale}) async {
if (locale == null) throw ArgumentError.notNull('locale');
var localePath = getLocalePath(locale);
EasyLocalization.logger.debug('Load asset from $path');
- // Load the asset as a string and decode it as JSON
- return json.decode(await rootBundle.loadString(localePath));
+ try {
+ final jsonString = await rootBundle.loadString(localePath);
+ return json.decode(jsonString) as Map<String, dynamic>;
+ } on FlutterError catch (e) {
+ throw AssetLoadException('Failed to load $localePath: ${e.message}');
+ } on FormatException catch (e) {
+ throw AssetLoadException('Invalid JSON in $localePath: ${e.message}');
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| import 'package:easy_localization/src/asset_loader.dart'; | ||
|
|
||
| import 'package:flutter/widgets.dart'; | ||
| import 'package:flutter_test/flutter_test.dart'; | ||
| // Replace with your package and file name | ||
|
|
||
| void main() { | ||
| TestWidgetsFlutterBinding.ensureInitialized(); | ||
| group('RootBundleAssetLoader', () { | ||
| const path = 'i18n'; // Replace with the actual asset path | ||
| const locale = Locale('en'); | ||
| late RootBundleAssetLoader assetLoader; | ||
|
|
||
| setUp(() async { | ||
| assetLoader = const RootBundleAssetLoader(path); | ||
| await assetLoader.load(locale: locale); | ||
| }); | ||
|
|
||
| test('getLocalePath returns the correct path', () async { | ||
| const expectedPath = '$path/en.json'; | ||
| var result = assetLoader.getLocalePath(locale); | ||
|
|
||
| expect(result, expectedPath); | ||
| }); | ||
|
|
||
| test('load throws an error when the locale is null', () { | ||
| assetLoader = const RootBundleAssetLoader(path); | ||
| expect(() => assetLoader.load(locale: null), throwsArgumentError); | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage with more test cases.
The test file needs additional test cases to cover error scenarios and actual asset loading.
Consider adding these test cases:
test('load throws AssetLoadException when file not found', () {
assetLoader = const RootBundleAssetLoader('invalid/path');
expect(
() => assetLoader.load(locale: locale),
throwsA(isA<AssetLoadException>()),
);
});
test('load throws AssetLoadException for invalid JSON', () {
// Mock rootBundle.loadString to return invalid JSON
expect(
() => assetLoader.load(locale: locale),
throwsA(isA<AssetLoadException>()),
);
});
|
Seems like a valid improvement to me. Would be nice to see this getting approved or any recognition. |
There was a problem hiding this comment.
Pull Request Overview
This PR implements a major architectural overhaul to migrate EasyLocalization from v3.x to v4.x, focusing on improved initialization, asset loading, and storage abstraction. The changes introduce a new initialization pattern that separates asset loading from widget configuration and provides better performance through optimized loading strategies.
Key changes include:
- Introduction of
ensureInitializedmethod to handle asset loading and storage setup before widget creation - Removal of
supportedLocalesandpathparameters from the EasyLocalization widget constructor - New storage abstraction layer through
IEasyLocalizationStorageinterface - Updated asset loader architecture requiring path specification during initialization
Reviewed Changes
Copilot reviewed 20 out of 40 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
test/utils/test_asset_loaders.dart |
Updates test asset loaders to match new AssetLoader interface with supportedLocales parameter |
test/root_bundle_asset_loader_test.dart |
Adds comprehensive tests for RootBundleAssetLoader with new path-based initialization |
test/easy_localization_widget_test.dart |
Updates widget tests to use new initialization pattern and removes deprecated parameters |
test/easy_localization_test.dart |
Refactors localization tests to use new controller initialization approach |
test/easy_localization_ctl_test.dart |
Adds new controller-specific tests for locale selection logic |
test/easy_localization_context_test.dart |
Updates context extension tests for new initialization flow |
pubspec.yaml |
Bumps version to 4.0.0-dev.0, updates SDK requirement, and adds comprehensive topic tags |
lib/src/easy_localization_storage_interface.dart |
Introduces new storage abstraction interface |
lib/src/easy_localization_controller.dart |
Major refactor implementing new initialization and storage patterns |
lib/src/easy_localization_app.dart |
Updates main widget to remove deprecated parameters and use new initialization |
lib/src/asset_loader.dart |
Modernizes AssetLoader with path requirement and improved error handling |
lib/easy_localization.dart |
Exports new storage interface |
i18n/ files |
Adds sample localization files for testing |
example/ files |
Updates example app to demonstrate new initialization pattern |
| /// Retrieves the value associated with the given [key]. | ||
| /// | ||
| /// Returns the value of type [T]. | ||
| T getValue<T>(String key); |
There was a problem hiding this comment.
The getValue method should return Future<T?> instead of T to handle asynchronous storage operations and potential null values consistently with other methods in the interface.
| T getValue<T>(String key); | |
| Future<T?> getValue<T>(String key); |
| storage?.init(); | ||
|
|
||
| // Get the saved locale from the storage | ||
| final strLocale = storage?.getValue<String>('locale'); |
There was a problem hiding this comment.
The getValue method is called synchronously but should likely be awaited since storage operations are typically asynchronous. This could lead to incorrect behavior.
| final strLocale = storage?.getValue<String>('locale'); | |
| final strLocale = await storage?.getValue<String>('locale'); |
| } else { | ||
| throw io.PathExistsException; | ||
| } | ||
| } |
There was a problem hiding this comment.
Using io.Directory for asset loading is incorrect. Flutter assets are not file system directories but are embedded in the app bundle. This will cause runtime errors in production builds.
| } | |
| // Set the supported locales directly from loader.supportedLocales | |
| if (loader.supportedLocales == null) { | |
| throw Exception('supportedLocales must be provided when using asset loading in Flutter. Listing asset directories is not supported.'); | |
| } | |
| _supportedLocales = loader.supportedLocales!; |
| .toLocale(separator: '-')) | ||
| .toList(); | ||
| } else { | ||
| throw io.PathExistsException; |
There was a problem hiding this comment.
PathExistsException is not the appropriate exception type for a missing directory. Consider using PathNotFoundException or a custom exception.
| throw io.PathExistsException; | |
| throw io.PathNotFoundException(loader.path!); |
| /// [supportedLocales] is a list of locales that the assets support. | ||
| const AssetLoader({this.path, this.supportedLocales}) | ||
| : assert(path != null || supportedLocales != null, | ||
| 'path or supportedLocales must not be null'); |
There was a problem hiding this comment.
The assertion message could be more descriptive. Consider changing it to 'Either path or supportedLocales must be provided' for better developer experience.
| 'path or supportedLocales must not be null'); | |
| 'Either path or supportedLocales must be provided'); |
| child: MyApp(), | ||
| // fallbackLocale: Locale('en', 'US'), | ||
| // startLocale: Locale('ar', 'DZ'), | ||
| supportedLocales: [Locale('en', 'US'), Locale('ar', 'DZ')], |
There was a problem hiding this comment.
The supportedLocales parameter is being used in the EasyLocalization widget, but according to the migration guide, this parameter should be removed in v4.x.
| supportedLocales: [Locale('en', 'US'), Locale('ar', 'DZ')], |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 40 changed files in this pull request and generated 14 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| final Function(FlutterError e) onLoadError; | ||
| // ignore: prefer_typing_uninitialized_variables | ||
| final assetLoader; | ||
| final String path; | ||
| final bool useFallbackTranslations; | ||
| final bool saveLocale; | ||
| final bool useOnlyLangCode; |
| child: MyApp(), | ||
| // fallbackLocale: Locale('en', 'US'), | ||
| // startLocale: Locale('ar', 'DZ'), | ||
| supportedLocales: [Locale('en', 'US'), Locale('ar', 'DZ')], |
| // useOnlyLangCode: true, | ||
|
|
||
| await EasyLocalization.ensureInitialized( | ||
| assetLoader: RootBundleAssetLoader(path: 'resources/langs/json'), |
| static Future<void> ensureInitialized({ | ||
| AssetLoader assetLoader = const RootBundleAssetLoader('resources/langs'), | ||
| IEasyLocalizationStorage? storage, | ||
| }) async => | ||
| await EasyLocalizationController.initEasyLocation(assetLoader, |
| try { | ||
| if (_listtranslationData != null) { | ||
| var index = _supportedLocales.indexOf(locale); | ||
| translationData = _listtranslationData![index]; |
| String getLocalePath(Locale locale) { | ||
| return '$path/${locale.toStringWithSeparator(separator: "-")}.json'; | ||
| } |
| /// Save locale in device storage. | ||
| /// @Default value true | ||
| final bool saveLocale; |
| testWidgets('Counter increments smoke test', (WidgetTester tester) async { | ||
| // Build our app and trigger a frame. | ||
| await tester.pumpWidget(MyApp()); | ||
| // // Build our app and trigger a frame. | ||
| // await tester.pumpWidget(MyApp()); | ||
|
|
||
| // Verify that our counter starts at 0. | ||
| expect(find.text('0'), findsOneWidget); | ||
| expect(find.text('1'), findsNothing); | ||
| // // Verify that our counter starts at 0. |
| Future<Map<String, dynamic>> loadTranslationData(Locale locale) async { | ||
| late Map<String, dynamic>? data; | ||
| late final Map<String, dynamic> translationData; | ||
|
|
||
| if (useOnlyLangCode) { | ||
| data = await assetLoader.load(path, Locale(locale.languageCode)); | ||
| } else { | ||
| data = await assetLoader.load(path, locale); | ||
| try { | ||
| if (_listtranslationData != null) { |
| // group('SharedPreferences SavedLocale NULL', () { | ||
| // setUp(() { | ||
| // SharedPreferences.setMockInitialValues({ | ||
| // 'locale': '', | ||
| // }); |
Introduced CachedAssetLoader for base functionality and updated RootBundleAssetLoader to include internal translation caching. Added OptimizedAssetLoader as a wrapper for backward compatibility.
- Optimize translation lookup and caching - Add `TranslationLoadException` for better error handling - Update `TextTranslateExtension` to use `TextScaler` - Refactor `EasyLocalizationController` for cleaner initialization - Improve plural rule evaluation fallback
- Upgrade intl to version 0.19.0+ - Add benchmark_harness to dev_dependencies - Standardize SDK version quote style
Included a stopwatch to measure the locale switching time and displayed the result via a SnackBar to provide immediate performance feedback.
- Add constructor to CachedAssetLoader - Prevent overwriting existing keys when merging fallback translations - Log TranslationLoadException details - Expose raw data in Translations for testing purposes
Updated test suite to use named arguments for RootBundleAssetLoader and initialized controllers within setUpAll to ensure correct state.
Update IEasyLocalizationStorage to use Future for all operations to support async-first persistence layers like SharedPreferences. Added concrete SharedPreferences and InMemory implementations.
- Refactored `EasyLocalization.ensureInitialized` to accept `assetLoader` and optional `storage`. - Added `IEasyLocalizationStorage` interface with `SharedPreferencesStorage` and `InMemoryStorage` implementations. - Updated README examples and widget properties to reflect the new initialization flow.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
lib/src/easy_localization_app.dart (2)
188-194:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
assertfor locale validation only runs in debug mode.Line 191 uses
assertto validate that the locale is supported. In release builds, this check is stripped, allowing unsupported locales to be set silently. Consider using an explicit check that throws in all modes:Suggested fix
Future<void> setLocale(Locale locale) async { // Check old locale if (locale != _localeState.locale) { - assert(EasyLocalizationController.supportedLocales.contains(locale)); + if (!EasyLocalizationController.supportedLocales.contains(locale)) { + throw ArgumentError('Locale $locale is not in supportedLocales: ${EasyLocalizationController.supportedLocales}'); + } await _localeState.setLocale(locale); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/src/easy_localization_app.dart` around lines 188 - 194, The setLocale method uses assert(EasyLocalizationController.supportedLocales.contains(locale)) which is stripped in release builds, so replace the assert with an explicit runtime check in Future<void> setLocale(Locale locale) that verifies EasyLocalizationController.supportedLocales.contains(locale) and throws a clear exception (e.g., ArgumentError or StateError) or returns an error before calling _localeState.setLocale(locale); keep the existing early-return when locale == _localeState.locale and ensure the error message names the unsupported locale and references EasyLocalizationController.supportedLocales for debugging.
50-52:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocumentation says
saveLocaledefaults totrue, but actual default isfalse.The doc comment on line 51 states "
@Defaultvalue true", but the constructor on line 65 setsthis.saveLocale = false. Update the documentation to match the implementation.Fix documentation
/// Save locale in device storage. - /// `@Default` value true + /// `@Default` value false final bool saveLocale;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/src/easy_localization_app.dart` around lines 50 - 52, The doc comment for the field saveLocale is wrong (it says "`@Default` value true" while the EasyLocalizationApp constructor sets this.saveLocale = false); update the documentation on the saveLocale field to state the default is false (or mirror the implementation) so the comment matches the EasyLocalizationApp constructor and the saveLocale field description.test/easy_localization_test.dart (2)
41-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winError-path tests can pass even when no error occurs.
These tests do not fail if
loadTranslations()completes normally, and they assert thrown exceptions even thoughloadTranslations()reports loader failures throughonLoadErrorinstead of rethrowing.Suggested test pattern update
- test('is Assertion Error when path and supportedLocales null', () async { - try { - await r1.loadTranslations(); - } on AssertionError catch (e) { - expect(e, isAssertionError); - } - }); + test('calls onLoadError when loader config is invalid', () async { + FlutterError? captured; + final c = EasyLocalizationController( + forceLocale: const Locale('en'), + useOnlyLangCode: true, + useFallbackTranslations: false, + saveLocale: false, + onLoadError: (e) => captured = e, + ); + await c.loadTranslations(); + expect(captured, isNotNull); + });Also applies to: 64-70
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/easy_localization_test.dart` around lines 41 - 47, The tests expecting an AssertionError from await r1.loadTranslations() are invalid because loadTranslations reports errors via onLoadError rather than throwing; update the tests (both the "is Assertion Error when path and supportedLocales null" and the similar one at 64-70) to register an onLoadError handler and assert that it was invoked (e.g., set a boolean or use a mock/callback) when calling r1.loadTranslations(), and remove any try/catch that expects an AssertionError or replace it with an expectation that the future completes and the onLoadError callback was called; reference r1.loadTranslations and the onLoadError handler in your changes.
290-315:⚠️ Potential issue | 🟠 MajorMove
initEasyLocationintosetUpAllsoloadTranslations()doesn’t depend on hook order (group('tr') / group('plural'))
Intest/easy_localization_test.dart,setUpAllcallsr.loadTranslations()+Localization.load(...), butinitEasyLocation(...)(which setsEasyLocalizationController’s static late_assetLoader) runs only insetUp.setUpAllruns beforesetUp, so this is order-dependent and can fail when running the group in isolation.Suggested fix shape
- setUp(() async { - EasyLocalization.logger.enableLevels = LevelMessages.values.toList(); - await EasyLocalizationController.initEasyLocation( - const JsonAssetLoader([Locale('en'), Locale('fb')])); - }); + setUpAll(() async { + EasyLocalization.logger.enableLevels = LevelMessages.values.toList(); + await EasyLocalizationController.initEasyLocation( + const JsonAssetLoader([Locale('en'), Locale('fb')])); + await r.loadTranslations(); + Localization.load(const Locale('en'), + translations: r.translations, + fallbackTranslations: r.fallbackTranslations); + }); - setUpAll(() async { - await r.loadTranslations(); - Localization.load(const Locale('en'), - translations: r.translations, - fallbackTranslations: r.fallbackTranslations); - });Also apply the same restructuring to
group('plural')(around lines 481-505).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/easy_localization_test.dart` around lines 290 - 315, The test fails due to initEasyLocation(...) (which sets EasyLocalizationController's static late _assetLoader) being called in setUp while r.loadTranslations() and Localization.load(...) are invoked in setUpAll (which runs earlier); move the initEasyLocation(...) call into the same setUpAll that calls r.loadTranslations() and Localization.load(...) so the asset loader is initialized before loadTranslations() runs (apply the same change inside both group('tr') and group('plural')), keeping the EasyLocalization.logger setup as needed and leaving r, loadTranslations, and Localization.load calls intact.
🧹 Nitpick comments (4)
pubspec.yaml (1)
42-42: Intl constraint update is unlikely to breakIntl.defaultLocale/NumberFormat.currency
intl0.19.0 didn’t introduce breaking public API changes forIntl.defaultLocaleorNumberFormat.currency, so the updated constraint>=0.19.0 <0.21.0should be API-compatible across0.19.x–0.20.x. Dropping0.17.x/0.18.xcan still affect dependency resolution for downstreams (they may need to upgrade to satisfy your new pub constraint), so confirm whether that restriction is intentional/required.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pubspec.yaml` at line 42, The pubspec change tightened the intl constraint to ">=0.19.0 <0.21.0" which may force downstream consumers off 0.17/0.18; decide whether that restriction is intentional and either revert or broaden the range. If you intend to require >=0.19.0, add a short comment in pubspec.yaml or the PR description explaining that Intl.defaultLocale and NumberFormat.currency compatibility was verified for 0.19.x–0.20.x; otherwise change the dependency line for intl back to include 0.17/0.18 (e.g., a wider lower bound) so downstreams aren’t forced to upgrade. Ensure the dependency string referring to intl in pubspec.yaml and any CI/pub upgrade notes reflect the chosen scope.lib/src/asset_loader.dart (1)
121-121: 💤 Low valueMissing
constconstructor.
OptimizedAssetLoaderconstructor is notconstunlike its parentRootBundleAssetLoader. If intentional (due to non-const initialization), document why; otherwise addconstfor consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/src/asset_loader.dart` at line 121, The OptimizedAssetLoader constructor should be made const to match its parent RootBundleAssetLoader unless there's non-const initialization—update the constructor declaration OptimizedAssetLoader({required String path}) : super(path: path); to be const (i.e., add the const keyword) so instances can be compile-time constants; if there is a deliberate reason it cannot be const, add a brief comment in the class (near OptimizedAssetLoader) explaining why const was omitted.lib/src/easy_localization_controller.dart (2)
126-138: ⚖️ Poor tradeoffConcurrent
setLocalecalls are silently dropped.The
_loadingguard on lines 127-128 causes concurrent locale change requests to be silently ignored. If a user rapidly switches locales, intermediate requests are lost without feedback. Consider queuing the latest request or returning a status indicating the call was skipped.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/src/easy_localization_controller.dart` around lines 126 - 138, The setLocale(Locale l) method currently drops concurrent calls due to the _loading boolean guard; change it to either (A) queue the latest requested locale by adding a _pendingLocale field and, if _loading is true, assign _pendingLocale = l and return (or await completion then apply pending), or (B) make setLocale return a status/future indicating it was skipped by returning a bool/Result when _loading is true. Implement the chosen approach inside setLocale and ensure you still call loadTranslations(), notifyListeners(), EasyLocalization.logger(...) and _saveLocale(_locale) for the applied locale, and clear/update _pendingLocale and the _loading flag (or resolve awaiting callers) so no intermediate requests are silently lost.
80-89: 💤 Low value
loadTranslationDatawraps exceptions but loses stack trace context.Line 87 creates a new
TranslationLoadExceptionbut the original stack trace from line 83 is logged but not preserved in the exception. Consider including the stack trace in the exception for better debugging downstream.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/src/easy_localization_controller.dart` around lines 80 - 89, The catch block in loadTranslationData logs the original stackTrace but throws a new TranslationLoadException without preserving it; modify the error handling so the original stackTrace is carried into the thrown exception—update the TranslationLoadException type (e.g., add an optional StackTrace parameter or a factory like TranslationLoadException.withTrace) and change the throw in loadTranslationData to throw TranslationLoadException(locale, e, stackTrace) (or use the new factory), keeping EasyLocalization.logger.error(...) as-is and ensuring _assetLoader.load errors retain their original stack trace for downstream debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/src/asset_loader.dart`:
- Around line 59-70: CachedAssetLoader.load() currently returns an empty Map on
cache miss which causes missing translations; change it so that when
!_translationCache.containsKey(locale) it loads translations from the actual
loader and caches them instead of returning {}. Locate the
CachedAssetLoader.load method and use the real loading mechanism (for example
call the underlying loader method or super.load if this class wraps/extends a
real loader) to obtain the Map<String, dynamic>, store it in
_translationCache[locale], and then return the loaded map; keep the existing
null-check for locale and the early return for cached entries.
- Around line 118-140: OptimizedAssetLoader is double-caching translations;
remove the redundant internal cache and rely on RootBundleAssetLoader's built-in
caching: delete the static Map<Locale, Map<String, dynamic>> _cache and the
clearCache() method, keep the constructor and the load({Locale? locale})
override but simplify it to only validate locale, optionally log a cache-hit by
probing super.load's behavior is unnecessary, then call and return await
super.load(locale: locale); this preserves behavior while eliminating duplicate
storage and references to OptimizedAssetLoader._cache.
In `@lib/src/easy_localization_controller.dart`:
- Around line 140-145: The early-return condition in _saveLocale is inverted:
change the guard so the method returns when saving is disabled OR storage is
unavailable (i.e., when saveLocale is false OR _storage is null). Update the
condition in Future<void> _saveLocale(Locale? locale) accordingly (referencing
_saveLocale, saveLocale, and _storage) so that await
_storage?.setValue('locale', ...) and EasyLocalization.logger(...) only run when
saving is enabled and storage exists.
In `@lib/src/easy_localization_storage_interface.dart`:
- Around line 30-42: The storage methods getValue, setValue, and removeValue
currently use the nullable _prefs with the safe-call operator and thus silently
no-op if init() wasn't called; update these methods (getValue, setValue,
removeValue) to guard against a null _prefs by either throwing a clear
StateError (e.g., "SharedPreferences not initialized; call init()") when _prefs
== null or by exposing an isInitialized getter and returning a controlled error
for callers — implement one approach consistently and ensure all three methods
check _prefs and fail fast instead of silently returning null/do-nothing.
In `@pubspec.yaml`:
- Line 33: The pubspec environment.sdk currently requires ">=3.1.0 <4.0.0" but
the codebase doesn’t use Dart 3.1+ language features; update the SDK constraint
to the lowest compatible version (e.g., ">=2.18.0 <4.0.0" or whatever minimum
your dependencies/tools require) in the pubspec.yaml environment.sdk field;
before committing, verify all package dependencies and CI/tooling (pubspec.lock,
analyzer/formatter versions, and any GitHub Actions or build scripts) support
the lower SDK and run dart analyze and dart pub get to ensure no SDK-specific
errors remain.
In `@test/easy_localization_widget_test.dart`:
- Around line 320-322: The tests call EasyLocalization.ensureInitialized with a
custom RootBundleAssetLoader(path: 'i18n/ar') but never restore
EasyLocalizationController's global state (_assetLoader, _supportedLocales,
_locale), making later tests order-dependent; after each test that switches to
'i18n/ar' (the calls at/around EasyLocalization.ensureInitialized(assetLoader:
const RootBundleAssetLoader(path: 'i18n/ar'))), add teardown code to restore the
global EasyLocalization state by calling EasyLocalization.ensureInitialized
again with the default loader and expected defaults (e.g.,
EasyLocalization.ensureInitialized(assetLoader: const RootBundleAssetLoader(),
supportedLocales: <your-defaults>, locale: yourDefaultLocale)) or wrap the
change in a try/finally so the finally block re-initializes EasyLocalization to
the original/default loader/locale to contain the change.
---
Outside diff comments:
In `@lib/src/easy_localization_app.dart`:
- Around line 188-194: The setLocale method uses
assert(EasyLocalizationController.supportedLocales.contains(locale)) which is
stripped in release builds, so replace the assert with an explicit runtime check
in Future<void> setLocale(Locale locale) that verifies
EasyLocalizationController.supportedLocales.contains(locale) and throws a clear
exception (e.g., ArgumentError or StateError) or returns an error before calling
_localeState.setLocale(locale); keep the existing early-return when locale ==
_localeState.locale and ensure the error message names the unsupported locale
and references EasyLocalizationController.supportedLocales for debugging.
- Around line 50-52: The doc comment for the field saveLocale is wrong (it says
"`@Default` value true" while the EasyLocalizationApp constructor sets
this.saveLocale = false); update the documentation on the saveLocale field to
state the default is false (or mirror the implementation) so the comment matches
the EasyLocalizationApp constructor and the saveLocale field description.
In `@test/easy_localization_test.dart`:
- Around line 41-47: The tests expecting an AssertionError from await
r1.loadTranslations() are invalid because loadTranslations reports errors via
onLoadError rather than throwing; update the tests (both the "is Assertion Error
when path and supportedLocales null" and the similar one at 64-70) to register
an onLoadError handler and assert that it was invoked (e.g., set a boolean or
use a mock/callback) when calling r1.loadTranslations(), and remove any
try/catch that expects an AssertionError or replace it with an expectation that
the future completes and the onLoadError callback was called; reference
r1.loadTranslations and the onLoadError handler in your changes.
- Around line 290-315: The test fails due to initEasyLocation(...) (which sets
EasyLocalizationController's static late _assetLoader) being called in setUp
while r.loadTranslations() and Localization.load(...) are invoked in setUpAll
(which runs earlier); move the initEasyLocation(...) call into the same setUpAll
that calls r.loadTranslations() and Localization.load(...) so the asset loader
is initialized before loadTranslations() runs (apply the same change inside both
group('tr') and group('plural')), keeping the EasyLocalization.logger setup as
needed and leaving r, loadTranslations, and Localization.load calls intact.
---
Nitpick comments:
In `@lib/src/asset_loader.dart`:
- Line 121: The OptimizedAssetLoader constructor should be made const to match
its parent RootBundleAssetLoader unless there's non-const initialization—update
the constructor declaration OptimizedAssetLoader({required String path}) :
super(path: path); to be const (i.e., add the const keyword) so instances can be
compile-time constants; if there is a deliberate reason it cannot be const, add
a brief comment in the class (near OptimizedAssetLoader) explaining why const
was omitted.
In `@lib/src/easy_localization_controller.dart`:
- Around line 126-138: The setLocale(Locale l) method currently drops concurrent
calls due to the _loading boolean guard; change it to either (A) queue the
latest requested locale by adding a _pendingLocale field and, if _loading is
true, assign _pendingLocale = l and return (or await completion then apply
pending), or (B) make setLocale return a status/future indicating it was skipped
by returning a bool/Result when _loading is true. Implement the chosen approach
inside setLocale and ensure you still call loadTranslations(),
notifyListeners(), EasyLocalization.logger(...) and _saveLocale(_locale) for the
applied locale, and clear/update _pendingLocale and the _loading flag (or
resolve awaiting callers) so no intermediate requests are silently lost.
- Around line 80-89: The catch block in loadTranslationData logs the original
stackTrace but throws a new TranslationLoadException without preserving it;
modify the error handling so the original stackTrace is carried into the thrown
exception—update the TranslationLoadException type (e.g., add an optional
StackTrace parameter or a factory like TranslationLoadException.withTrace) and
change the throw in loadTranslationData to throw
TranslationLoadException(locale, e, stackTrace) (or use the new factory),
keeping EasyLocalization.logger.error(...) as-is and ensuring _assetLoader.load
errors retain their original stack trace for downstream debugging.
In `@pubspec.yaml`:
- Line 42: The pubspec change tightened the intl constraint to ">=0.19.0
<0.21.0" which may force downstream consumers off 0.17/0.18; decide whether that
restriction is intentional and either revert or broaden the range. If you intend
to require >=0.19.0, add a short comment in pubspec.yaml or the PR description
explaining that Intl.defaultLocale and NumberFormat.currency compatibility was
verified for 0.19.x–0.20.x; otherwise change the dependency line for intl back
to include 0.17/0.18 (e.g., a wider lower bound) so downstreams aren’t forced to
upgrade. Ensure the dependency string referring to intl in pubspec.yaml and any
CI/pub upgrade notes reflect the chosen scope.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 30693c36-bd73-48fb-b002-302cd60a6eb9
📒 Files selected for processing (18)
README.mdexample/lib/lang_view.dartlib/easy_localization.dartlib/src/asset_loader.dartlib/src/easy_localization_app.dartlib/src/easy_localization_controller.dartlib/src/easy_localization_storage_interface.dartlib/src/exceptions.dartlib/src/localization.dartlib/src/public_ext.dartlib/src/translations.dartlib/src/widgets.dartpubspec.yamltest/easy_localization_context_test.darttest/easy_localization_test.darttest/easy_localization_widget_test.darttest/root_bundle_asset_loader_test.darttest/utils/test_asset_loaders.dart
✅ Files skipped from review due to trivial changes (2)
- lib/src/widgets.dart
- README.md
🚧 Files skipped from review as they are similar to previous changes (5)
- lib/easy_localization.dart
- example/lib/lang_view.dart
- test/root_bundle_asset_loader_test.dart
- test/easy_localization_context_test.dart
- test/utils/test_asset_loaders.dart
Refactor test setup to include explicit supported locales and start locales in EasyLocalization initializers to ensure consistent test environments.
- Update CachedAssetLoader to inherit from RootBundleAssetLoader. - Simplify OptimizedAssetLoader by utilizing base class caching. - Add error handling for uninitialized SharedPreferences. - Fix locale saving condition in EasyLocalizationController.
Reset EasyLocalization state after fallback locale tests and correct the cache clearing reference in OptimizedAssetLoader tests.
aissat
changed the title
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
test/compatibility_test.dart (1)
15-23: ⚡ Quick winThese compatibility assertions should verify contract values, not only types.
The test names claim behavior (
useOnlyLangCode, defaultpath) but only assertisA<...>(). Please assert the actual forwarded/default values.Concrete assertions to add
test('withConfig respects useOnlyLangCode', () { final widget = EasyLocalizationCompat.withConfig( child: const Placeholder(), useOnlyLangCode: true, fallbackLocale: const Locale('fr', 'FR'), ); expect(widget, isA<EasyLocalization>()); + final easy = widget as EasyLocalization; + expect(easy.useOnlyLangCode, isTrue); + expect(easy.fallbackLocale, const Locale('fr', 'FR')); }); test('migrateAssetLoader uses default path when not specified', () { final loader = EasyLocalizationCompat.migrateAssetLoader( type: AssetLoaderType.rootBundle, ); expect(loader, isA<RootBundleAssetLoader>()); + final root = loader as RootBundleAssetLoader; + expect(root.path, 'assets/translations'); });Also applies to: 36-41
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/compatibility_test.dart` around lines 15 - 23, The test currently only checks the returned type from EasyLocalizationCompat.withConfig; update it to assert that the returned EasyLocalization instance has the expected forwarded configuration values: verify that useOnlyLangCode is true and fallbackLocale equals Locale('fr','FR') for the first case by accessing the EasyLocalization widget's properties (from the EasyLocalization instance returned by withConfig), and similarly update the other test (lines 36-41) to assert the default path value (e.g., 'assets/translations' or the library's default) and any other default flags forwarded by withConfig rather than only using isA<...>(). Ensure you locate the instance returned by EasyLocalizationCompat.withConfig and assert on its useOnlyLangCode, fallbackLocale, path, and related config properties.test/optimized_asset_loader_test.dart (1)
21-31: ⚡ Quick winCache behavior tests are currently non-verifying.
These cases only assert that loads throw, so they do not actually validate caching or per-locale cache separation. A regression in cache behavior would still pass these tests.
Suggested test direction
test('caches translations across loads', () async { final loader = OptimizedAssetLoader(path: testPath); final locale = const Locale('en', 'US'); - await expectLater( - () => loader.load(locale: locale), - throwsA(anything), - ); + // Arrange a mock asset response and count loadString hits for this locale. + // Assert first load hits bundle once, second load returns cached data with no extra hit. }); test('handles different locales independently', () async { final loader = OptimizedAssetLoader(path: testPath); final localeUS = const Locale('en', 'US'); final localeFR = const Locale('fr', 'FR'); - await expectLater(() => loader.load(locale: localeUS), throwsA(anything)); - await expectLater(() => loader.load(locale: localeFR), throwsA(anything)); + // Arrange distinct mock payloads for each locale path. + // Assert each locale is cached independently and returns its own data. });Also applies to: 33-47
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/optimized_asset_loader_test.dart` around lines 21 - 31, The tests named 'caches translations across loads' (and the similar test around lines 33-47) currently only assert that load() throws, so they don't verify caching or per-locale isolation; update them to actually assert cache behavior by either (a) creating real temporary translation files and calling OptimizedAssetLoader.load(locale: ...) twice and asserting the loader returns from cache on the second call (e.g., check a private/public cache accessor or size if available) or (b) inject or spy a loader internals method used for disk reads (mock the disk-read helper or wrap the file-read method) and assert it is invoked only once per locale while two loads are requested for the same Locale and invoked for two different Locales to assert separation; modify the tests to use the OptimizedAssetLoader constructor and the specific methods load and the internal cache accessor/spied method names to locate the code to change.lib/src/asset_loader.dart (1)
37-72: 💤 Low valueCachedAssetLoader and RootBundleAssetLoader both maintain static caches, doubling memory usage.
CachedAssetLoader._translationCache(line 40) andRootBundleAssetLoader._cache(line 82) are both staticMap<Locale, Map<String, dynamic>>. WhenCachedAssetLoader.load()callssuper.load()at line 69, data is stored inRootBundleAssetLoader._cache, then again in_translationCacheat line 70.If the public
translationCachegetter is needed for testing/external access, consider havingCachedAssetLoaderskip the parent's caching by directly callingrootBundle.loadString()instead ofsuper.load(), or makeRootBundleAssetLoadercheck if a subclass already handles caching.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/src/asset_loader.dart` around lines 37 - 72, CachedAssetLoader duplicates RootBundleAssetLoader's static cache by calling super.load(), so change CachedAssetLoader.load to bypass the parent's cache: instead of calling super.load(locale: locale), use rootBundle.loadString(...) to read the asset for the given Locale, decode the JSON into a Map<String, dynamic>, store it only into CachedAssetLoader._translationCache and return it; keep the translationCache getter and other helpers as-is so tests can access the cache but avoid calling super.load() (and thus RootBundleAssetLoader._cache) to prevent double-storing.lib/src/compatibility.dart (1)
10-37: ⚡ Quick win
withConfig()is missingstartLocale,saveLocale, anderrorWidgetparameters.The v4
EasyLocalizationwidget retains these parameters (per PR objectives), butwithConfig()doesn't expose them. Users migrating v3 code that used these features would need to bypass this helper.Suggested enhancement
static Widget withConfig({ required Widget child, List<Locale>? supportedLocales, String? path, AssetLoader? assetLoader, Locale? fallbackLocale, + Locale? startLocale, + bool saveLocale = true, bool useOnlyLangCode = false, bool useFallbackTranslations = false, + Widget Function(FlutterError? message)? errorWidget, }) { // ... existing warning logic ... return EasyLocalization( fallbackLocale: fallbackLocale, + startLocale: startLocale, + saveLocale: saveLocale, useOnlyLangCode: useOnlyLangCode, useFallbackTranslations: useFallbackTranslations, + errorWidget: errorWidget, child: child, ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/src/compatibility.dart` around lines 10 - 37, Update the withConfig signature to accept the missing parameters startLocale (Locale?), saveLocale (bool?), and errorWidget (WidgetBuilder?) and forward them into the EasyLocalization(...) constructor call (i.e., add startLocale: startLocale, saveLocale: saveLocale, errorWidget: errorWidget) so that withConfig exposes the same options as the EasyLocalization widget; keep the existing warnings for supportedLocales/path but ensure the new parameters are declared with the same types/names used by EasyLocalization to maintain parity for migrating v3 users.example/v3_vs_v4_comparison.md (1)
114-127: 💤 Low valueAdd language specifiers to code blocks.
The two code blocks showing directory structures are missing language specifiers. Consider adding
textor leave them as plain text blocks for clarity.📝 Suggested fix
-``` +```text assets/translations/ ├── en.json { "greeting": "Hello", ... } └── ar.json { "greeting": "مرحبا", ... }Or with country codes (when
useOnlyLangCode: false, default):-
+text
assets/translations/
├── en-US.json
└── ar-DZ.json🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@example/v3_vs_v4_comparison.md` around lines 114 - 127, Update the two fenced code blocks that show the assets/translations directory structure to include a language specifier (e.g., ```text) so they render as plain text; specifically, change the opening backticks for both directory-structure blocks (the ones starting with "assets/translations/") to use ```text instead of plain ``` to ensure proper formatting and clarity.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/playwright.yml:
- Around line 4-6: The workflow currently only triggers on branches: [ main,
master ] for both push and pull_request, so PRs targeting develop won’t run;
update the branch filters in the Playwright workflow by adding "develop" to both
branches arrays (the branches lists used for the push and pull_request triggers)
so the workflow will run for pushes and PRs against develop as well.
- Line 12: Update the Playwright workflow's checkout step to disable persisted
credentials by adding persist-credentials: false to the actions/checkout@v4
invocation; locate the existing uses: actions/checkout@v4 step in the workflow
and add the persist-credentials: false key under that step so the job does not
retain repo write credentials.
- Around line 12-13: The workflow uses floating tags for actions (uses:
actions/checkout@v4 and uses: actions/setup-node@v4) which must be pinned to
commit SHAs; update those two uses entries to reference the exact commit SHAs
from the respective GitHub repos (e.g., actions/checkout@<commit-sha> and
actions/setup-node@<commit-sha>) by looking up the desired release commit in
each action's repo and replacing the `@v4` tags with the corresponding SHAs so the
workflow no longer relies on retargetable tags.
In `@CHANGELOG.md`:
- Around line 3-15: Update the v4.0.0-dev changelog entry to explicitly list the
three removed features so they appear among the breaking changes: add entries
for useFallbackTranslationsForEmptyResources, ignorePluralRules, and
extraAssetLoaders (mention they were removed in v4), and ensure the phrasing
matches existing BREAKING entries and aligns with the
example/v3_vs_v4_comparison.md notes for consistency.
In `@example/v3_vs_v4_comparison.md`:
- Around line 139-141: Verify whether the three features
`useFallbackTranslationsForEmptyResources`, `ignorePluralRules`, and
`extraAssetLoaders` were actually removed in v4; if they were removed, add
explicit entries for each under the v4.0.0-dev BREAKING CHANGES section of
CHANGELOG.md describing the removal and any migration steps; if they are still
supported, remove these rows from the v3_vs_v4_comparison.md table to avoid
incorrect documentation. Ensure the CHANGELOG entries reference the exact
feature names above and briefly state the impact or recommended migration
action.
In `@pubspec.yaml`:
- Line 18: The SDK lower bound in pubspec.yaml (environment.sdk) is incompatible
with the declared intl range; update pubspec.yaml so either raise the Dart SDK
lower bound (environment.sdk) to a 3.x-compatible value (e.g., ">=3.0.0 <4.0.0")
or pin the intl dependency (the intl entry in dependencies) to a 2.x-compatible
range that supports Dart 2 (e.g., restrict to versions prior to 0.19), and
update the dependency constraint accordingly to ensure pub can resolve
dependencies for Dart 2.x users.
---
Nitpick comments:
In `@example/v3_vs_v4_comparison.md`:
- Around line 114-127: Update the two fenced code blocks that show the
assets/translations directory structure to include a language specifier (e.g.,
```text) so they render as plain text; specifically, change the opening
backticks for both directory-structure blocks (the ones starting with
"assets/translations/") to use ```text instead of plain ``` to ensure proper
formatting and clarity.
In `@lib/src/asset_loader.dart`:
- Around line 37-72: CachedAssetLoader duplicates RootBundleAssetLoader's static
cache by calling super.load(), so change CachedAssetLoader.load to bypass the
parent's cache: instead of calling super.load(locale: locale), use
rootBundle.loadString(...) to read the asset for the given Locale, decode the
JSON into a Map<String, dynamic>, store it only into
CachedAssetLoader._translationCache and return it; keep the translationCache
getter and other helpers as-is so tests can access the cache but avoid calling
super.load() (and thus RootBundleAssetLoader._cache) to prevent double-storing.
In `@lib/src/compatibility.dart`:
- Around line 10-37: Update the withConfig signature to accept the missing
parameters startLocale (Locale?), saveLocale (bool?), and errorWidget
(WidgetBuilder?) and forward them into the EasyLocalization(...) constructor
call (i.e., add startLocale: startLocale, saveLocale: saveLocale, errorWidget:
errorWidget) so that withConfig exposes the same options as the EasyLocalization
widget; keep the existing warnings for supportedLocales/path but ensure the new
parameters are declared with the same types/names used by EasyLocalization to
maintain parity for migrating v3 users.
In `@test/compatibility_test.dart`:
- Around line 15-23: The test currently only checks the returned type from
EasyLocalizationCompat.withConfig; update it to assert that the returned
EasyLocalization instance has the expected forwarded configuration values:
verify that useOnlyLangCode is true and fallbackLocale equals Locale('fr','FR')
for the first case by accessing the EasyLocalization widget's properties (from
the EasyLocalization instance returned by withConfig), and similarly update the
other test (lines 36-41) to assert the default path value (e.g.,
'assets/translations' or the library's default) and any other default flags
forwarded by withConfig rather than only using isA<...>(). Ensure you locate the
instance returned by EasyLocalizationCompat.withConfig and assert on its
useOnlyLangCode, fallbackLocale, path, and related config properties.
In `@test/optimized_asset_loader_test.dart`:
- Around line 21-31: The tests named 'caches translations across loads' (and the
similar test around lines 33-47) currently only assert that load() throws, so
they don't verify caching or per-locale isolation; update them to actually
assert cache behavior by either (a) creating real temporary translation files
and calling OptimizedAssetLoader.load(locale: ...) twice and asserting the
loader returns from cache on the second call (e.g., check a private/public cache
accessor or size if available) or (b) inject or spy a loader internals method
used for disk reads (mock the disk-read helper or wrap the file-read method) and
assert it is invoked only once per locale while two loads are requested for the
same Locale and invoked for two different Locales to assert separation; modify
the tests to use the OptimizedAssetLoader constructor and the specific methods
load and the internal cache accessor/spied method names to locate the code to
change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6eba10fc-1366-498e-a38b-5f8b320b0021
📒 Files selected for processing (14)
.github/workflows/playwright.yml.gitignoreCHANGELOG.mdeasy_localization.imlexample/.metadataexample/v3_vs_v4_comparison.mdlib/src/asset_loader.dartlib/src/compatibility.dartlib/src/easy_localization_controller.dartlib/src/easy_localization_storage_interface.dartpubspec.yamltest/compatibility_test.darttest/easy_localization_widget_test.darttest/optimized_asset_loader_test.dart
💤 Files with no reviewable changes (3)
- example/.metadata
- easy_localization.iml
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/src/easy_localization_storage_interface.dart
- test/easy_localization_widget_test.dart
| branches: [ main, master ] | ||
| pull_request: | ||
| branches: [ main, master ] |
There was a problem hiding this comment.
Workflow won’t run for develop-targeted PRs.
This workflow only listens to main/master, but this PR targets develop; tests won’t execute on that integration path unless develop is added.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/playwright.yml around lines 4 - 6, The workflow currently
only triggers on branches: [ main, master ] for both push and pull_request, so
PRs targeting develop won’t run; update the branch filters in the Playwright
workflow by adding "develop" to both branches arrays (the branches lists used
for the push and pull_request triggers) so the workflow will run for pushes and
PRs against develop as well.
| timeout-minutes: 60 | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
Disable persisted checkout credentials for this read-only test job.
Set persist-credentials: false on actions/checkout; this job does not need repo write credentials and reducing token exposure hardens the pipeline.
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 12-12: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 12-12: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/playwright.yml at line 12, Update the Playwright
workflow's checkout step to disable persisted credentials by adding
persist-credentials: false to the actions/checkout@v4 invocation; locate the
existing uses: actions/checkout@v4 step in the workflow and add the
persist-credentials: false key under that step so the job does not retain repo
write credentials.
Source: Linters/SAST tools
| - uses: actions/checkout@v4 | ||
| - uses: actions/setup-node@v4 |
There was a problem hiding this comment.
Pin GitHub Actions to commit SHAs instead of floating tags.
Using @v4 leaves the workflow open to upstream tag retargeting/supply-chain risk and violates the reported policy check.
Also applies to: 22-22
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 12-12: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 12-12: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 13-13: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/playwright.yml around lines 12 - 13, The workflow uses
floating tags for actions (uses: actions/checkout@v4 and uses:
actions/setup-node@v4) which must be pinned to commit SHAs; update those two
uses entries to reference the exact commit SHAs from the respective GitHub repos
(e.g., actions/checkout@<commit-sha> and actions/setup-node@<commit-sha>) by
looking up the desired release commit in each action's repo and replacing the
`@v4` tags with the corresponding SHAs so the workflow no longer relies on
retargetable tags.
Source: Linters/SAST tools
|
|
||
| environment: | ||
| sdk: '>=2.12.0 <4.0.0' | ||
| sdk: ">=2.17.0 <4.0.0" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify intl version metadata directly from pub.dev (read-only).
curl -s https://pub.dev/api/packages/intl | jq '.latest.version, .versions[] | select(.version=="0.19.0" or .version=="0.20.0") | {version, sdk: .pubspec.environment.sdk}'Repository: aissat/easy_localization
Length of output: 136
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Fetch intl package metadata from pub.dev (read-only) and inspect JSON shape first.
curl -s https://pub.dev/api/packages/intl | jq 'keys'
# Show structure of a single versions element (if present).
curl -s https://pub.dev/api/packages/intl | jq '.versions | type, (.[0] | keys)'
# Extract the pubspec.environment.sdk for specific versions.
curl -s https://pub.dev/api/packages/intl | jq '
[0.19.0, 0.20.0] as $wanted
| {matches: [.versions[]
| select(.version? and ($wanted|index(.version)) != null)
| {version: .version, sdk: (.pubspec.environment.sdk? // .pubspec.environment? // null)}]]
'Repository: aissat/easy_localization
Length of output: 845
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show available intl versions in the 0.19.x / 0.20.x range.
curl -s https://pub.dev/api/packages/intl | jq -r '
.versions
| map(.version)
| map(select(startswith("0.19.") or startswith("0.20.")))
| sort
| .[]
'
# Extract pubspec.environment.sdk constraints for intl 0.19.0 and 0.20.0 (quoted strings).
curl -s https://pub.dev/api/packages/intl | jq '
[ "0.19.0", "0.20.0" ] as $wanted
| .versions
| map(select(.version as $v | $wanted | index($v) != null))
| map({version: .version, sdk: (.pubspec.environment.sdk? // .pubspec.environment? // null)})
'
# Also extract any versions in repo constraint range >=0.19.0 <0.21.0 (best-effort).
curl -s https://pub.dev/api/packages/intl | jq '
.versions
| map(select(.version? != null))
| map({version: .version, sdk: (.pubspec.environment.sdk? // .pubspec.environment? // null)})
| sort_by(.version)
| map(select(.version >= "0.19.0" and .version < "0.21.0"))
'Repository: aissat/easy_localization
Length of output: 629
🏁 Script executed:
#!/bin/bash
set -euo pipefail
curl -s https://pub.dev/api/packages/intl | jq '
[ "0.20.1", "0.20.2" ] as $wanted
| .versions
| map(select(.version as $v | $wanted | index($v) != null))
| map({version: .version, sdk: (.pubspec.environment.sdk? // .pubspec.environment? // null)})
'Repository: aissat/easy_localization
Length of output: 179
Align Dart SDK lower bound with intl’s Dart 3+ requirement
pubspec.yaml sets environment.sdk: ">=2.17.0 <4.0.0", but intl: ">=0.19.0 <0.21.0" can resolve to intl 0.19.0–0.20.2, whose pubspec.environment.sdk is ^3.0.0 / ^3.3.0. This can break pub dependency resolution for consumers on Dart 2.17–2.19. Align these constraints by either raising the Dart minimum (e.g., >=3.0.0) or tightening intl to versions that support Dart 2.x.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pubspec.yaml` at line 18, The SDK lower bound in pubspec.yaml
(environment.sdk) is incompatible with the declared intl range; update
pubspec.yaml so either raise the Dart SDK lower bound (environment.sdk) to a
3.x-compatible value (e.g., ">=3.0.0 <4.0.0") or pin the intl dependency (the
intl entry in dependencies) to a 2.x-compatible range that supports Dart 2
(e.g., restrict to versions prior to 0.19), and update the dependency constraint
accordingly to ensure pub can resolve dependencies for Dart 2.x users.
- Added a library-level description to easy_localization.dart. - Restricted intl package exports to specific classes. - Added a macro reference to the plural extension method.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/easy_localization.dart`:
- Line 10: The export in lib/easy_localization.dart now only re-exports Intl,
DateFormat, and NumberFormat which is a breaking change for consumers who relied
on other package:intl/intl.dart symbols; update the project docs to call this
out and provide migration guidance (add a CHANGELOG.md entry and a README note)
stating that easy_localization no longer re-exports Bidi, DateSymbols,
NumberSymbols, message-lookup classes, etc., and advise consumers to import
package:intl/intl.dart directly or restore the broader re-export if you intend
to keep previous surface area; reference the export statement in
lib/easy_localization.dart and make the docs change alongside a clear "Breaking
change" header and suggested code examples for consumers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 23280047-9a5e-4eb3-b086-e67ddf5d643f
📒 Files selected for processing (6)
CHANGELOG.mdlib/easy_localization.dartlib/src/easy_localization_controller.dartlib/src/plural_rules.dartlib/src/public_ext.dartpubspec.yaml
✅ Files skipped from review due to trivial changes (1)
- lib/src/plural_rules.dart
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/src/public_ext.dart
- pubspec.yaml
- CHANGELOG.md
- lib/src/easy_localization_controller.dart
| export 'package:intl/intl.dart'; | ||
| export 'package:easy_localization/src/easy_localization_storage_interface.dart'; | ||
| export 'package:easy_localization/src/compatibility.dart'; | ||
| export 'package:intl/intl.dart' show Intl, DateFormat, NumberFormat; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for intl API usage beyond Intl, DateFormat, NumberFormat
# Check for Bidi class usage
rg -n '\bBidi\b' --type dart --glob '!**/test/**'
# Check for TextDirection from intl (note: Flutter also has TextDirection)
rg -n "import.*intl.*TextDirection" --type dart
# Check for DateSymbols, NumberSymbols usage
rg -n '\b(DateSymbols|NumberSymbols)\b' --type dart --glob '!**/test/**'
# Check for Intl message classes
rg -n '\b(IntlMessage|MessageLookup|CompositeMessage)\b' --type dart --glob '!**/test/**'Repository: aissat/easy_localization
Length of output: 50
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect the export line in question
echo "=== lib/easy_localization.dart (around line 10) ==="
nl -ba lib/easy_localization.dart | sed -n '1,60p'
# 2) Search for other intl symbols/types used anywhere in the repo (non-test)
echo
echo "=== Search: other intl types used ==="
rg -n --type dart --glob '!**/test/**' '\b(Bidi|DateSymbols|NumberSymbols|MessageLookup|CompositeMessage|IntlMessage|TextDirection)\b' . || true
# 3) Search for intl imports/usages patterns
echo
echo "=== Search: imports from intl ==="
rg -n --type dart --glob '!**/test/**' 'import\s+['"]package:intl/intl.dart['"]' . || true
rg -n --type dart --glob '!**/test/**' 'package:intl/intl.dart.*\b' . || true
# 4) Search for the export narrowing in docs/changelog
echo
echo "=== Search: changelog/migration docs mention intl export narrowing ==="
fd -a "CHANGELOG*|changelog*|MIGRATION*|migration*|UPGRADING*|upgrading*|README*|roadmap*" . -t f 2>/dev/null || true
rg -n --glob '!*test/**' 'intl\.|easy_localization.*intl|DateFormat|NumberFormat|Bidi|TextDirection|DateSymbols|NumberSymbols|MessageLookup|CompositeMessage' CHANGELOG* **/*CHANGELOG* **/*migration* **/*MIGRATION* **/*README* **/*UPGRADING* 2>/dev/null || trueRepository: aissat/easy_localization
Length of output: 181
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect the export line in question (no `nl` available)
echo "=== lib/easy_localization.dart (first ~60 lines) ==="
awk 'NR>=1 && NR<=60 {printf "%d:%s\n", NR, $0}' lib/easy_localization.dart | sed -n '1,60p'
# 2) Search for other intl symbols/types used anywhere in the repo (non-test)
echo
echo "=== Search: other intl types used (non-test) ==="
rg -n --type dart --glob '!**/test/**' '\b(Bidi|DateSymbols|NumberSymbols|MessageLookup|CompositeMessage|IntlMessage|TextDirection)\b' . || true
# 3) Search for intl imports/usages patterns
echo
echo "=== Search: imports from package:intl/intl.dart (non-test) ==="
rg -n --type dart --glob '!**/test/**' 'import\s+['"'"'"]package:intl/intl.dart['"'"'"]' . || true
rg -n --type dart --glob '!**/test/**' 'package:intl/intl.dart.*\b' . || true
# 4) Search for the export narrowing mention in changelog/migration docs
echo
echo "=== Search: changelog/migration docs mention intl export narrowing ==="
# List likely docs/changelogs first
fd . CHANGELOG* changelog* MIGRATION* migration* UPGRADING* upgrading* README* 2>/dev/null || true
rg -n --glob '!*test/**' \
'intl\.|easy_localization.*intl|DateFormat|NumberFormat|Bidi|TextDirection|DateSymbols|NumberSymbols|MessageLookup|CompositeMessage' \
CHANGELOG* **/*CHANGELOG* **/*migration* **/*MIGRATION* **/*README* **/*UPGRADING* 2>/dev/null || trueRepository: aissat/easy_localization
Length of output: 1491
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== lib/src/public.dart (first 200 lines) ==="
awk 'NR<=200 {printf "%d:%s\n", NR, $0}' lib/src/public.dart
echo
echo "=== lib/src/public_ext.dart (first 200 lines) ==="
awk 'NR<=200 {printf "%d:%s\n", NR, $0}' lib/src/public_ext.dart
echo
echo "=== lib/src/widgets.dart (around TextDirection usage) ==="
rg -n "TextDirection\.ltr" lib/src/widgets.dart
sed -n '1,120p' lib/src/widgets.dart
echo
echo "=== Explicit searches for other intl symbols (non-test) ==="
for sym in Bidi DateSymbols NumberSymbols MessageLookup CompositeMessage IntlMessage; do
echo "-- $sym --"
rg -n --type dart --glob '!**/test/**' "\b${sym}\b" . || true
done
echo
echo "=== Repo-wide search for intl re-exports / narrowed show export ==="
rg -n --type dart --glob '!**/test/**' "export\s+'package:intl/intl\.dart'\s+show" . || true
rg -n --type dart --glob '!**/test/**' "show\s+Intl,\s*DateFormat,\s*NumberFormat" . || true
rg -n --type dart --glob '!**/test/**' "package:intl/intl\.dart'\s+show" . || true
echo
echo "=== Find changelog/migration/upgrading docs filenames ==="
fd . -t f -i "changelog|migration|upgrading|upgrade" || true
echo
echo "=== Search docs for intl export narrowing / migration notes ==="
rg -n --glob '!*test/**' '(intl\.dart|DateFormat|NumberFormat|Bidi|TextDirection|DateSymbols|NumberSymbols|MessageLookup|CompositeMessage)' \
CHANGELOG* **/CHANGELOG* **/*migration* **/*MIGRATION* **/*upgrad* **/*README* **/*UPGRAD* 2>/dev/null || trueRepository: aissat/easy_localization
Length of output: 13597
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Repo top-level files ==="
ls -la
echo
echo "=== Candidate changelog/migration/upgrading files ==="
fd -t f -i '(changelog|migration|upgrading|upgrad(es?)|release|news)\b' . || true
echo
echo "=== Search docs for intl export narrowing / breaking change ==="
rg -n --glob '!*test/**' '(intl\.dart|package:intl/intl\.dart|DateFormat|NumberFormat|Bidi|DateSymbols|NumberSymbols|MessageLookup|CompositeMessage)\b' \
--glob '*README*' --glob '*CHANGELOG*' --glob '*MIGRATION*' --glob '*UPGRADING*' . || true
echo
echo "=== Search for 'breaking' + 'intl' wording ==="
rg -n --glob '!*test/**' '(breaking|migration|upgrade|changelog)\b.*\bintl\b' . || trueRepository: aissat/easy_localization
Length of output: 2463
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== CHANGELOG.md: search for intl/breaking/migration/export keywords ==="
rg -n --glob '*CHANGELOG*' '(intl|package:intl/intl\.dart|breaking|migration|upgrade|export)' CHANGELOG.md || true
echo
echo "=== README.md: search for intl/export keywords ==="
rg -n '(intl|package:intl/intl\.dart|export.*intl|migration|upgrade|breaking)' README.md example/README.md || trueRepository: aissat/easy_localization
Length of output: 959
Document the breaking impact of the narrowed intl re-export (lib/easy_localization.dart:10).
- Repo usage check: no references to other
package:intl/intl.dartsymbols such asBidi,DateSymbols,NumberSymbols, or message-lookup classes—only the narrowedIntl/DateFormat/NumberFormatsurface is used internally (FlutterTextDirectionis unrelated). - Documentation check:
CHANGELOG.md/README do not mention thateasy_localizationnow re-exports onlyIntl,DateFormat, andNumberFormat, so consumers relying on otherintlexports througheasy_localizationwon’t have a migration path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/easy_localization.dart` at line 10, The export in
lib/easy_localization.dart now only re-exports Intl, DateFormat, and
NumberFormat which is a breaking change for consumers who relied on other
package:intl/intl.dart symbols; update the project docs to call this out and
provide migration guidance (add a CHANGELOG.md entry and a README note) stating
that easy_localization no longer re-exports Bidi, DateSymbols, NumberSymbols,
message-lookup classes, etc., and advise consumers to import
package:intl/intl.dart directly or restore the broader re-export if you intend
to keep previous surface area; reference the export statement in
lib/easy_localization.dart and make the docs change alongside a clear "Breaking
change" header and suggested code examples for consumers.
Introduces a suite of functions for resolving nested translation keys, handling link references, and performing argument substitution. These functions operate on plain maps and strings, ensuring they are environment-agnostic and easy to test.
- Remove commented-out code and unused TODOs - Improve code formatting and style consistency
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/easy_logger/pubspec.yaml (1)
16-16: Consider removing redundantmatcherdev dependency
packages/easy_logger/pubspec.yamladdsmatcher: ^0.12.19(line 16), but there are no directpackage:matcherimports anywhere underpackages/easy_logger/—so it’s likely provided transitively (e.g., viaflutter_test). Drop it unless you’re intentionally pinning a specificmatcherversion for your tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/easy_logger/pubspec.yaml` at line 16, The pubspec dev_dependency entry for "matcher: ^0.12.19" is redundant in the easy_logger package; remove the "matcher" dev_dependencies entry from the package's pubspec.yaml (unless you intentionally need that specific version for tests), then run pub get and run the package's tests to ensure nothing breaks; if a test actually requires matcher, instead document the reason in a comment near the dev_dependencies entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/src/resolver.dart`:
- Around line 27-29: The _capitalize function can crash on empty input; update
_capitalize(String val) to first check val.isEmpty and return val (or '')
immediately, otherwise construct '${val[0].toUpperCase()}${val.substring(1)}';
optionally apply the same defensive empty-string check to _upper and _lower for
consistency (though toUpperCase()/toLowerCase() already handle empty strings) so
all three helpers are safe to call with empty input.
---
Nitpick comments:
In `@packages/easy_logger/pubspec.yaml`:
- Line 16: The pubspec dev_dependency entry for "matcher: ^0.12.19" is redundant
in the easy_logger package; remove the "matcher" dev_dependencies entry from the
package's pubspec.yaml (unless you intentionally need that specific version for
tests), then run pub get and run the package's tests to ensure nothing breaks;
if a test actually requires matcher, instead document the reason in a comment
near the dev_dependencies entry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 18a02147-ef7f-4c1e-9e63-468170605efa
📒 Files selected for processing (9)
lib/src/resolver.dartpackages/easy_logger/pubspec.yamltest/easy_localization_context_test.darttest/easy_localization_ctl_test.darttest/easy_localization_test.darttest/easy_localization_widget_test.darttest/optimized_asset_loader_test.darttest/resolver_test.darttest/root_bundle_asset_loader_test.dart
💤 Files with no reviewable changes (1)
- test/easy_localization_widget_test.dart
🚧 Files skipped from review as they are similar to previous changes (5)
- test/optimized_asset_loader_test.dart
- test/easy_localization_ctl_test.dart
- test/root_bundle_asset_loader_test.dart
- test/easy_localization_context_test.dart
- test/easy_localization_test.dart
v4.0.0-dev — API refactor, caching, fallback fixes, and benchmark suite
Summary
This PR modernizes the
easy_localizationAPI by movingpathandsupportedLocalesintoAssetLoader, adding built-in caching, introducing a clean storage interface withSharedPreferencesStorageandInMemoryStorage, fixing fallback translation bugs, and introducing a comprehensive benchmark suite (v3 vs v4).AssetLoaderreworkAssetLoaderis no longer a stateless pass-through. It now ownspathandsupportedLocales, passed via constructor:RootBundleAssetLoaderrequirespathEasyLocalizationwidget simplifiedssupportedLocales`` andpathare no longer widget properties — they live on the loader. The widget only receives `child`, `fallbackLocale`, `startLocale`, `saveLocale`, `useFallbackTranslations`, `useOnlyLangCode`, and `errorWidget`.ensureInitialized()is now required, takes loader + optional storageRemoved APIs
useFallbackTranslationsForEmptyResourcesignorePluralRulesextraAssetLoadersAssetLoaderAssetLoader.localeExists()load()throws on failure instead🗄️ Storage Interface
Provided a clean, testable storage abstraction with two implementations:
IEasyLocalizationStorageSharedPreferencesStorage(default)Backed by
shared_preferences. Pass toensureInitialized():InMemoryStorage(testing)Ephemeral map-based storage for unit tests — no mock setup needed:
✨ New Features
Built-in caching
Three caching layers available:
CachedAssetLoader_translationCache,cacheTranslations(),getCachedTranslations(),isCached(). Building block for custom loaders.OptimizedAssetLoaderRootBundleAssetLoaderwith a separate static cache layer + cache-hit logging.RootBundleAssetLoader_cacheinternally. Switching back to a previously loaded locale costs ~0.02–0.09 ms.Interactive switch timing
When running
flutter runand tapping a language:Switch time: 0.56 msLast switch: X.XX mson the main pageBENCH switch (interactive: en_US) 0.56 msBenchmark suite
flutter_benchmark/contains identical Flutter Linux apps for v3.0.8 vs v4.0.0-dev.Run with:
cd flutter_benchmark && bash bench.shSample results (Linux x64 release AOT):
setLocale()🐛 Bug Fixes
Fallback translations overwritten by base language
When
_localehas a country code (e.g.es_AR) andsupportedLocalescontains the base language (es), the base language data was merged after the fallback locale data, overwriting real fallback values withnull:useOnlyLangCodewith country-code locales_localewas static — all controller instances shared it, and later constructor calls overwrote it beforeloadTranslations()ran. Restructured tests to create + load each controller within the same microtask.CachedAssetLoadermissingconstconstructorAdded
const CachedAssetLoader({required super.path})for use in const contexts.ListTileink-splash warningsWrapped
Container+ListTileinMaterial(type: MaterialType.transparency)across all benchmarks and example.🧪 Test Improvements
test/easy_localization_test.dart(~175 → ~600 lines)Asset loader,wrong path, linked locale messages, nested arguments, fallback edge casesEasyLocalizationControllerinstances (no global singleton)putIfAbsentnull-skipMigration Guide
Step 1: Update
main()Step 2: Update custom asset loaders
Summary by CodeRabbit
New Features
Breaking Changes
Improvements
Bug Fixes