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

[updates] fix error recovery on android #27815

Merged
merged 4 commits into from
Apr 1, 2024

Conversation

Kudo
Copy link
Contributor

@Kudo Kudo commented Mar 22, 2024

Why

add bridgeless support for expo-updates's error recovery on android
close ENG-11707

How

  • the DefaultJSExceptionHandler from DevSupportManager doesn't work on bridgeless mode. we should add a ReactNativeHostHandler.onReactInstanceException to pull the exception from ReactHost
  • add ReactApplication.restart() to support both bridgeless and bridge mode.
  • decouple from ReactNativeHost

Test Plan

Checklist

@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Mar 22, 2024
@expo-bot
Copy link
Collaborator

expo-bot commented Mar 22, 2024

The Pull Request introduced fingerprint changes against the base commit: 796d42a

Fingerprint diff
[
  {
    "type": "dir",
    "filePath": "../../packages/expo-modules-core/android",
    "reasons": [
      "expoAutolinkingAndroid"
    ],
    "hash": "de8d3310de13d9a5605e3879286bb29fd04443eb"
  },
  {
    "type": "dir",
    "filePath": "../../packages/expo-updates/android",
    "reasons": [
      "expoAutolinkingAndroid"
    ],
    "hash": "d1e2a67bbf7990d98ba4b62369007d725ac32645"
  },
  {
    "type": "dir",
    "filePath": "../../packages/expo/android",
    "reasons": [
      "expoAutolinkingAndroid"
    ],
    "hash": "dde693a1c8cad65bef7da0050bcd720f99343ed1"
  }
]

Generated by PR labeler 🤖

@Kudo Kudo force-pushed the @kudo/bridgeless/updates-error-recovery-android branch from 82ad034 to 3f20d49 Compare March 26, 2024 17:08
Copy link

linear bot commented Mar 26, 2024

@Kudo Kudo marked this pull request as ready for review March 26, 2024 17:39
@Kudo Kudo removed request for brentvatne and ide March 26, 2024 17:39
@Kudo Kudo force-pushed the @kudo/bridgeless/updates-error-recovery-android branch from 3f20d49 to 8eff6d1 Compare March 27, 2024 05:43
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Mar 27, 2024
@Kudo
Copy link
Contributor Author

Kudo commented Mar 27, 2024

the sdk check-packages failure would be fixed by #27891

Copy link
Member

@wschurman wschurman left a comment

Choose a reason for hiding this comment

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

Main change is to add asserts for our assumptions about code we expect to only run in bridgeless and code we expect to never run in bridgeless.

@@ -245,6 +245,10 @@ class StartupProcedure(
errorRecovery.startMonitoring(reactContext)
}

fun onReactInstanceException(exception: Exception) {
errorRecovery.handleException(exception)
Copy link
Member

Choose a reason for hiding this comment

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

since overriding the exception handler doesn't work on bridgeless, should we update ErrorRecovery to not register the error when in bridgeless mode?

edit: I see that the registration is done in onDidCreateReactInstanceManager/onDidCreateReactInstance so it shouldn't be running on bridgeless. let's add an assert to registerErrorHandler to assert non-bridgeless and also add an assert to onReactInstanceException (here) to assert bridgeless, just so that if upstream (expo-modules-core) is changed our assumptions hold.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tried to split that as two different implementation, hopes that looks good

  private fun registerErrorHandler(reactContext: ReactContext) {
    if (ReactFeatureFlags.enableBridgelessArchitecture) {
      registerErrorHandlerImplBridgeless(reactContext)
    } else {
      registerErrorHandlerImplBridge(reactContext)
    }
  }

Kudo and others added 3 commits April 1, 2024 10:38
Co-authored-by: Will Schurman <wschurman@expo.io>
Co-authored-by: Douglas Lowder <douglowder@mac.com>
@Kudo Kudo force-pushed the @kudo/bridgeless/updates-error-recovery-android branch from 8eff6d1 to 16526fd Compare April 1, 2024 03:09
@Kudo Kudo requested a review from wschurman April 1, 2024 04:58
@Kudo Kudo merged commit 705b518 into main Apr 1, 2024
16 checks passed
@Kudo Kudo deleted the @kudo/bridgeless/updates-error-recovery-android branch April 1, 2024 16:35
@brentvatne brentvatne added the published Changes from the PR have been published to npm label Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: fingerprint changed bot: passed checks ExpoBot has nothing to complain about published Changes from the PR have been published to npm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants