Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Fix issue with sync manifest downloading #174

Merged
merged 8 commits into from
Mar 21, 2022
Merged

Fix issue with sync manifest downloading #174

merged 8 commits into from
Mar 21, 2022

Conversation

serhii-londar
Copy link
Collaborator

Closes #169

@codecov
Copy link

codecov bot commented Feb 20, 2022

Codecov Report

Merging #174 (3335a6a) into master (e4fa76e) will increase coverage by 0.71%.
The diff coverage is 77.50%.

@@            Coverage Diff             @@
##           master     #174      +/-   ##
==========================================
+ Coverage   55.94%   56.64%   +0.71%     
==========================================
  Files         118      119       +1     
  Lines        4019     4151     +132     
==========================================
+ Hits         2248     2351     +103     
- Misses       1771     1800      +29     
Impacted Files Coverage Δ
...rs/Crowdin/Extensions/CrowdinSDK+ReactNative.swift 0.00% <0.00%> (ø)
...n/MappingDownloader/CrowdinMappingDownloader.swift 0.00% <0.00%> (ø)
...wdin/MappingDownloader/CrowdinMappingManager.swift 0.00% <0.00%> (ø)
...ders/Crowdin/ManifestManager/ManifestManager.swift 79.27% <78.21%> (-20.73%) ⬇️
...ers/Crowdin/CrowdinRemoteLocalizationStorage.swift 72.23% <79.17%> (+5.56%) ⬆️
...K/Localization/Provider/LocalizationProvider.swift 71.85% <82.15%> (+6.68%) ⬆️
Sources/CrowdinSDK/CrowdinSDK/CrowdinSDK.swift 60.68% <100.00%> (-5.63%) ⬇️
...wdinSDK/CrowdinSDK/Localization/Localization.swift 78.58% <100.00%> (+3.02%) ⬆️
...rowdin/Extensions/CrowdinSDK+CrowdinProvider.swift 41.67% <100.00%> (-8.33%) ⬇️
...tionDownloader/CrowdinLocalizationDownloader.swift 62.50% <100.00%> (+0.80%) ⬆️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e4fa76e...3335a6a. Read the comment docs.

@crowdin-bot
Copy link
Collaborator

crowdin-bot commented Mar 12, 2022

Warnings
⚠️ Big PR, try to keep changes smaller if you can

Generated by 🚫 Danger Swift against 3335a6a

@andrii-bodnar andrii-bodnar merged commit e2d197f into master Mar 21, 2022
@andrii-bodnar andrii-bodnar deleted the #169 branch March 21, 2022 08:34
@chrispomeroyhale
Copy link

Wow, this was a lot of work @serhii-londar!

I quickly tried this out and still a little unclear when the completion handler gets called? I recall not seeing a completion handler called if, say, the manifest request failed? The resilience of the SDK is important to us. We're needing to access properties like inSDKLocalizations which require full initialization before they are populated.

What is the expectation of the completion handler? Is that the signal for us clients that the SDK has successfully initialized?

@serhii-londar
Copy link
Collaborator Author

HI @chrispomeroyhale, thanks for your feedback!
The completion handler called after SDK get or tried to get all the required files. It means that currently if you initialize SDK without an internet connection completion handler still will be called, all errors you receive via the error handler.

About inSDKLocalizations, now we've added caching for the manifest file so that when you have something cached you are able to get it immediately after starting SDK. After completion called it means that you can access inSDKLocalizations and they are up to date. Same for localizations.

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.

Starting the SDK blocks the app from loading until the manifest downloads
4 participants