fix(iOS) [0.74]: RCTRedBox not appearing in Bridgeless when metro is not running #43147

okwasniewski · 2024-02-22T12:50:40Z

Summary:

When testing out 0.74.0-rc0 I found that when the metro is not running we are not displaying RedBox which bumps users to start the packager and reload the app. It also fixes the case where users try to reload by clicking the "Reload" button on RedBox.

Before

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-02-22.at.13.50.06.mp4

After

Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-02-22.at.13.45.42.mp4

Changelog:

[IOS] [FIXED] - RCTRedBox not appearing in Bridgeless when metro is not running

Test Plan:

Build the app without metro running check if RedBox is shown

analysis-bot · 2024-02-22T13:21:24Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	18,008,365	-4,139
android	hermes	armeabi-v7a	n/a	--
android	hermes	x86	n/a	--
android	hermes	x86_64	n/a	--
android	jsc	arm64-v8a	21,366,887	-4,152
android	jsc	armeabi-v7a	n/a	--
android	jsc	x86	n/a	--
android	jsc	x86_64	n/a	--

Base commit: f7bbaff
Branch: main

dmytrorykun

It looks like error handling is not implemented in RCTInstance. To solve this properly please implement a separate method called handleError that would be basically this:
https://github.com/facebook/react-native/blob/main/packages/react-native/React/CxxBridge/RCTCxxBridge.mm#L1068
Note that the _loading ivar does not exist in RCTInstance. You should assume it true.

okwasniewski · 2024-02-29T11:14:12Z

@dmytrorykun I've extracted the code to a separate handleError method but I'm not sure if we should copy everything that's in RCTCxxBridge.mm handle error (there were hacks required to make old arch work). I've simplified the method, and everything seems to work as it should.

I have one issue with the JS raw stack as it's not attached when error is thrown:

react-native/packages/react-native/React/Base/RCTJavaScriptLoader.mm

Line 106 in 6204ea3

*error = [NSError

should we ignore it or there is a way to retrieve it?

RSNara

I left a few questions!

RSNara · 2024-03-04T18:59:43Z

packages/react-native/ReactCommon/react/runtime/platform/ios/ReactCommon/RCTInstance.mm

@@ -339,6 +341,10 @@ - (void)_start
    if (RCTGetUseNativeViewConfigsInBridgelessMode()) {
      installLegacyUIManagerConstantsProviderBinding(runtime);
    }
+
+    RCTExecuteOnMainQueue(^{
+      RCTRegisterReloadCommandListener(self);


I'm not sure if this reload change is necessary!

But, if it is, I have a request: Can we create a custom implementation of RCTReloadListener that retains RCTInstance weakly?

Bridgeless mode adheres to this principle:

Nothing outside of the RCTHost should own a reference to RCTInstance.

In practice, this line is fine: RCTRegisterReloadCommandListener holds on to RCTReloadListeners weakly.

But, I think bridgeless mode should not depend on this guarantee:

Weak retention of listeners is an implementation detail of RCTRegisterReloadCommandListener. If someone changes it in the future, this bridgeless principle could be violated.

Bridgeless classes should be responsible for enforcing bridgeless principles.

This was needed to make Reload button work in Redbox. I understand your concerns, I've tweaked the code to listen to notification through the notification center so now we don't need to worry about retaining this instance. WDYT about this approach?

@okwasniewski, the current solution definitely addresses the instance retention issue.

But, I don't think it's a good idea to rely on a global event emission to communicate between RCTRedBox and RCTInstance. Many apps/environments actually run multiple react instances at the same time. Global event emission makes that configuration on iOS more difficult. (We don't have a need for this right now, but it's always nice to extend react native in ways that don't preclude, or make this more difficult). And also, more generally: if we can avoid reliance on global things, we probably should.

Was there any problem with creating a custom RCTReloadListener? Perhaps you noticed an issue, or have a concern, that hadn't crossed my mind.

RSNara · 2024-03-04T19:00:02Z

packages/react-native/ReactCommon/react/runtime/platform/ios/ReactCommon/RCTInstance.mm

+- (void)didReceiveReloadCommand { 
+  [self _loadJSBundle:[self->_bridgeModuleDecorator.bundleManager bundleURL]];
+}
+


@okwasniewski, why did you end up registering the RCTInstance as a reload command listener here?

When clicking Reload on the redbox nothing was happening

dmytrorykun · 2024-03-06T17:57:36Z

Hey @okwasniewski , we are generally happy with the changes in this PR. But we would like to polish some details. Could you please resubmit this PR for the main branch. This way we'll be able to import it to our internal repo, and iterate on it faster. The goal is to land these changes before the 0.75-RC3.

facebook-github-bot · 2024-03-07T11:49:42Z

@dmytrorykun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

okwasniewski · 2024-03-08T12:29:00Z

Hey @dmytrorykun, @RSNara I'm going to be on PTO next week so I'm afraid I won't manage to implement custom implementation of RCTReloadListener are you guys okay with iterating on it internally?

facebook-github-bot · 2024-03-08T12:58:57Z

@dmytrorykun merged this pull request in 4adef35.

…not running (#43147) Summary: When testing out `0.74.0-rc0` I found that when the metro is not running we are not displaying RedBox which bumps users to start the packager and reload the app. It also fixes the case where users try to reload by clicking the "Reload" button on RedBox. ## Before https://github.com/facebook/react-native/assets/52801365/086c557f-ea1f-4a97-b4c7-df8a945cc7a0 ## After https://github.com/facebook/react-native/assets/52801365/9f8421b3-5e83-466f-8cdb-38f97981275d ## Changelog: [IOS] [FIXED] - RCTRedBox not appearing in Bridgeless when metro is not running Pull Request resolved: #43147 Test Plan: Build the app without metro running check if RedBox is shown Reviewed By: javache Differential Revision: D54632056 Pulled By: dmytrorykun fbshipit-source-id: fb6742898d3bd82545bfffd9175208e1a5984cb6

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Callstack Partner: Callstack Partner labels Feb 22, 2024

github-actions bot added the Pick Request label Feb 22, 2024

okwasniewski force-pushed the fix/redbox-not-appearing branch from d32f1a9 to 3026f13 Compare February 22, 2024 12:51

lunaleaps requested a review from philIip February 22, 2024 18:47

cortinico requested a review from dmytrorykun February 23, 2024 10:45

dmytrorykun requested changes Feb 28, 2024

View reviewed changes

okwasniewski force-pushed the fix/redbox-not-appearing branch from 3026f13 to 84c72b8 Compare February 29, 2024 11:16

RSNara requested changes Mar 4, 2024

View reviewed changes

okwasniewski force-pushed the fix/redbox-not-appearing branch from 03cf0dc to b41ab3b Compare March 5, 2024 14:26

okwasniewski requested a review from RSNara March 6, 2024 13:45

okwasniewski force-pushed the fix/redbox-not-appearing branch from 11f9f0d to 1be613b Compare March 7, 2024 11:01

okwasniewski changed the base branch from 0.74-stable to main March 7, 2024 11:01

okwasniewski added 2 commits March 7, 2024 12:20

fix: RCTRedBox not appearing in Bridgeless when metro is not running

8216168

fix: add listener through notification center

08eefac

okwasniewski force-pushed the fix/redbox-not-appearing branch from 1be613b to 08eefac Compare March 7, 2024 11:20

facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Mar 7, 2024

facebook-github-bot closed this in 4adef35 Mar 8, 2024

facebook-github-bot added the Merged This PR has been merged. label Mar 8, 2024

dmytrorykun mentioned this pull request Mar 8, 2024

fix(iOS) [0.74]: RCTRedBox not appearing in Bridgeless when metro is not running reactwg/react-native-releases#137

Closed

okwasniewski mentioned this pull request Mar 18, 2024

[0.74] New app hangs on blank screen if dev server is not started #43411

Closed

This was referenced Apr 5, 2024

[Yoga][0.74] Fix archive for MacOS Catalyst #43900

Merged

[RN][iOS] Cherry Pick #43757 and #43994 #44007

Merged

[iOS][Local] Fix breaking changes in 0.74-rc.8 #44025

Closed

This was referenced Jun 28, 2024

[CI] Move iOS Executors to M1 #45213

Closed

[Hermes] Bump Hermes #45220

Merged

cipolleschi mentioned this pull request Jul 22, 2024

[RN][0.74]Fix XROS build #45580

Merged

cipolleschi mentioned this pull request Jul 31, 2024

[RN][Hermes] Fore Hermes versions for slices through env var #45840

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(iOS) [0.74]: RCTRedBox not appearing in Bridgeless when metro is not running #43147

fix(iOS) [0.74]: RCTRedBox not appearing in Bridgeless when metro is not running #43147

okwasniewski commented Feb 22, 2024

analysis-bot commented Feb 22, 2024 •

edited

Loading

dmytrorykun left a comment

okwasniewski commented Feb 29, 2024

RSNara left a comment

RSNara Mar 4, 2024

okwasniewski Mar 5, 2024

RSNara Mar 6, 2024 •

edited

Loading

RSNara Mar 4, 2024

okwasniewski Mar 5, 2024

dmytrorykun commented Mar 6, 2024 •

edited

Loading

facebook-github-bot commented Mar 7, 2024

okwasniewski commented Mar 8, 2024 •

edited

Loading

facebook-github-bot commented Mar 8, 2024

fix(iOS) [0.74]: RCTRedBox not appearing in Bridgeless when metro is not running #43147

fix(iOS) [0.74]: RCTRedBox not appearing in Bridgeless when metro is not running #43147

Conversation

okwasniewski commented Feb 22, 2024

Summary:

Before

After

Changelog:

Test Plan:

analysis-bot commented Feb 22, 2024 • edited Loading

dmytrorykun left a comment

Choose a reason for hiding this comment

okwasniewski commented Feb 29, 2024

RSNara left a comment

Choose a reason for hiding this comment

RSNara Mar 4, 2024

Choose a reason for hiding this comment

okwasniewski Mar 5, 2024

Choose a reason for hiding this comment

RSNara Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

RSNara Mar 4, 2024

Choose a reason for hiding this comment

okwasniewski Mar 5, 2024

Choose a reason for hiding this comment

dmytrorykun commented Mar 6, 2024 • edited Loading

facebook-github-bot commented Mar 7, 2024

okwasniewski commented Mar 8, 2024 • edited Loading

facebook-github-bot commented Mar 8, 2024

analysis-bot commented Feb 22, 2024 •

edited

Loading

RSNara Mar 6, 2024 •

edited

Loading

dmytrorykun commented Mar 6, 2024 •

edited

Loading

okwasniewski commented Mar 8, 2024 •

edited

Loading