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

Remove fatalError in throwing function to bubble error up and let caller handle the situation #78

Merged
merged 1 commit into from
Feb 9, 2021

Conversation

jakobmygind
Copy link

No description provided.

@mariusc
Copy link
Contributor

mariusc commented Nov 10, 2020

Hey Jakob! Can you point this PR to the feature/spm-support branch, please?

The issue you're fixing there is something we've seen often, and is usually a sign of something else that went wrong. It was a fatalError on purpose, because if it gets in that situation, then there's probably something else that has gone wrong. We have talked a few times about making it into a try. Let me just try and get the others involved in the review too 😄

@jakobmygind jakobmygind changed the base branch from master to feature/spm-support November 10, 2020 11:56
@jakobmygind
Copy link
Author

jakobmygind commented Nov 10, 2020

@mariusc My bad, somehow forgot to point to the spm-branch 🙈 The reason for this PR is that the function in question is a throwing one, but the only throwing thing in the scope results in a fatalError, which doesn't make a lot of sense. Also there seems to be some issues having the SDK in a different module than the main one, especially running the app for the very first time, and if this method fatalErrors then there's no way the missing translations can be handled gracefully manually in these special cases. If it were to fatalError here, then it should be guaranteed that if the internal function throws there's absolutely no way the app will work. With the throwing function actually throwing I am now able to safeguard like this:

internal var lo: Localizations {
    guard let manager = NStack.sharedInstance.localizationManager else {
        return Localizations()
    }
    do {
        return try manager.localization()
    } catch {
        print("saved translations not found due to \(error), falling back to bundled json")
        return loadTranslationsFromJSON("Localizations_da-DK", in: Bundle(for: TranslationsBundle.self))
    }

}

@BucekJiri BucekJiri self-requested a review November 10, 2020 14:29
@BucekJiri
Copy link
Collaborator

Just today I tried to update JimeZdrave project to NStack 5 and it keeps crashing on this fatalError on the app launch. Same thing for Carthage and CocoaPods. Have not figured out the reason yet. NStack 4 was working fine.

One thing to note is that the project has the localizations in a separate module as @jakobmygind pointed might cause trouble.

@jakobmygind
Copy link
Author

@mariusc Any progress on decisions regarding this? As I see it it is a fix to piece of code that in itself is a bug (fatalError-ing in a catch block inside a throwing function), and also it allows fixing of a very present crash when running the app for the first time under certain circumstances. Also it is a PR towards a feature branch, so it should pose little risk.

@mariusc
Copy link
Contributor

mariusc commented Feb 9, 2021

Yup, let's get this in the spm-support branch for now and see how that goes 😄

@mariusc mariusc merged commit b9fa942 into nstack-io:feature/spm-support Feb 9, 2021
@jakobmygind
Copy link
Author

@mariusc Cheers, buddy 👍🏻

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.

None yet

3 participants