Skip to content

feat: expose downloaded manifest statically + check if locale is supported according to manifest #71

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

tim-alenus
Copy link

This PR makes the downloaded manifest (which happens during init) available under the static getter Crowdin.manifest.
This way once can check if a certain language is supported before calling await Crowdin.loadTranslations. Calling await Crowdin.loadTranslations results in an exception which is caught within the package. The function also does not return anything. With the manifest publicly exposed the app code can first check if it makes sense to call loadTranslations with a certain locale. One want to avoid to download the manifest again because every requests counts for the request count. Because this package already downloads the manifest I opted to make it publicly available.

@andrii-bodnar
Copy link
Member

@tim-alenus thank you for the contribution!

Would it make more sense to check for language support inside the Crowdin.loadTranslations method?

@tim-alenus
Copy link
Author

@andrii-bodnar that could be a useful addition as well. For this the manifest still needs to be saved somewhere though. There might be other use cases to expose the manifest, so having this publicly available still sounds like a good idea to me.

I will add a check in loadTranslations as well. In my opinion an exception should be thrown when a language is not supported. At the moment if you pass an unsupported language as parameter to loadTranslations the exception that occurs is caught and only causes a print in by the CrowdinLogger, but there is no way for the code that uses the package to known the language was not supported.

@tim-alenus tim-alenus changed the title feat: expose downloaded manifest statically feat: expose downloaded manifest statically + check if locale is supported according to manifest May 15, 2025
@andrii-bodnar andrii-bodnar requested a review from FlutterOd May 19, 2025 07:27
@andrii-bodnar
Copy link
Member

Thank you for the contributions, @tim-alenus! I'm requesting a review from @FlutterOd.

Comment on lines +151 to +153
if (manifest != null) {
checkManifestForLocale(locale);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We mainly used logs because in case the translation from Crowdin is not available, the translation will just be fetched from the fallback file seamlessly.

Also, if we throw an exception with the loadTranslation method, it could cause problems for existing users who don't expect exceptions here and don't handle them.

So for the loadTranslations method, let's just check if the locale is supported and prevent requests if it isn't.

And you'll still have the checkManifestForLocale method if you need to handle the exception.

Suggested change
if (manifest != null) {
checkManifestForLocale(locale);
}
if (manifest != null) {
try {
checkManifestForLocale(locale);
} catch (e) {
CrowdinLogger.printLog(e.toString());
_arb = null;
return;
}
}

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

Successfully merging this pull request may close these issues.

3 participants