[error-recovery] Extract into unimodule

[lukmccall]

Why

Resolve #5256.

Test Plan

I've tested it with simple demo https://snack.expo.io/@lukaszkosmaty/error-recovery-test.
I've also tested scoped behaviour.

[sjchmiela]

Nice work! 💪

[bbarthec]

LGTM 💯

[ide]

The new code looks good. There are still some CI failures https://circleci.com/gh/expo/expo/92601 and merge conflicts that need to be fixed.

[sjchmiela]

Great job! Left some comments on things we still have to figure out. 💪

[sjchmiela]

There's a whole bunch of code in Kernel depending on ErrorRecoveryManager, like ReactNativeActivity#shouldShowErrorScreen (called by BaseExperienceActivity#consumeErrorQueue (called byBaseExperienceActivity.addError…))

and Updates depending on app.json#updates.checkAutomatically === 'ON_ERROR_RECOVERY'

expo/android/expoview/src/main/java/host/exp/exponent/AppLoader.java

Line 119 in cfa00db

if (experienceMetadata != null && experienceMetadata.optBoolean(ExponentSharedPreferences.EXPERIENCE_METADATA_LOADING_ERROR)) {

Maybe we should annotate that code too with a clear strategy on how to deprecate it? Currently I don't see any support kept for ON_ERROR_RECOVERY setting for Updates. How do you handle it in expo-ota PR, @mczernek?

[lukmccall]

I've tried to read and understood ErrorRecoveryManager code.
Here is what this class does:

• Android
  • owns recovery props
  • has shouldReloadOnError methods, which indicate if experience should reload after the crash (for me it's a little bit strange because this method depends on the time between app loading and crashing).
• iOS
  • associates arbitrary developer info with experience id (we don't use this)
  • knows if auto-reload after an error is possible
  • tracks state of experience (is recovering, has errors, last loaded)
  • knows if error screen should be shown

Imho, we should leave most of this class. However, maybe renaming will be good options.
This class has more common with error handling than with error recovering.

[sjchmiela]

So to sum up, ErrorRecoveryManager's responsibilities are:

• recovery props memory — moved to native module
• knows whether to show error screen based on timing of crashes/successful loads — this can stay even longer than when SDK34 is phased out, we may want to leave this feature. Moreover, it is independent from the recovery props memory responsibility
• informs other pieces of the infrastructure (Updates) that the experience is erroring — this, fortunately, is also independent from the recovery props memory responsibility.

ErrorRecoveryManager doesn't need to be removed, it can stay as a part of ExpoKit. We just removed the memory responbility. 🙂

[sjchmiela]

I've looked at ErrorRecoveryManagers again and here's what I found

[sjchmiela]

So to sum up, ErrorRecoveryManager's responsibilities are:

• recovery props memory — moved to native module
• knows whether to show error screen based on timing of crashes/successful loads — this can stay even longer than when SDK34 is phased out, we may want to leave this feature. Moreover, it is independent from the recovery props memory responsibility
• informs other pieces of the infrastructure (Updates) that the experience is erroring — this, fortunately, is also independent from the recovery props memory responsibility.

ErrorRecoveryManager doesn't need to be removed, it can stay as a part of ExpoKit. We just removed the memory responbility. 🙂

[sjchmiela]

A couple of comments, apart from them — it's ready when it comes to my review!

[sjchmiela]

Great work! 👏 The only thing left is for @EvanBacon to respond to https://github.com/expo/expo/pull/5357/files#r327406072. 🙂 Otherwise it should be good to merge!