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.

Sign up for GitHub

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

Jump to bottom

[0.74] fix(iOS): add missing forward blocks to RCTRootViewFactory #177

Closed
Kudo opened this issue Mar 25, 2024 · 7 comments
Closed

[0.74] fix(iOS): add missing forward blocks to RCTRootViewFactory #177

Kudo opened this issue Mar 25, 2024 · 7 comments
Assignees
@Kudo
Labels
Type Pick Request Pick requests to include commits inside a React Native release

Comments

@Kudo
Copy link

Kudo commented Mar 25, 2024

Target Branch(es)

0.74

Link to commit or PR to be picked

facebook/react-native#43638

Description

fix RCTRootViewFactory regression. the pr is already based on 0.74-stable and cherry-pick two fixes that recommended here
@Kudo Kudo added the Type Pick Request Pick requests to include commits inside a React Native release label Mar 25, 2024
@Kudo Kudo mentioned this issue Mar 25, 2024
Closed
@huntie
Copy link
Member

huntie commented Mar 25, 2024

Merged ✅

@huntie huntie closed this as completed Mar 25, 2024
@github-project-automation github-project-automation bot moved this from Inbox to Done / Picked in React Native 0.74 Releases Mar 25, 2024
@huntie
Copy link
Member

huntie commented Mar 25, 2024

Reverted ❗This change appeared to break iOS tests (RNTestProject, i.e. E2E project creation with RN CLI). Let's look at getting this fixed and picked again for RC6.

@huntie huntie reopened this Mar 25, 2024
@cortinico
Copy link
Collaborator

Also @Kudo @okwasniewski
This change was preventing the iOS app from starting at all on Old Architecture. We should test this once more before attempting to do another pick of this

@okwasniewski
Copy link

Ugh, sorry - I'll try to figure out what's wrong with this tomorrow

@Kudo
Copy link
Author

Kudo commented Mar 26, 2024

apologize for not checking correctly.
the problem was coming from the implementation
https://github.com/facebook/react-native/blob/00725fadff28bb3c7fed65f208e647f0dab69e75/packages/react-native/React/CxxBridge/RCTCxxBridge.mm#L519-L540

we should dynamically override loadSourceForBridge:onProgress:onComplete: and loadSourceForBridge:withBlock: in RCTRootViewFactory only when AppDelegate override it. one way to achieve this might be tricky that we may need to override respondsToSelector:.

otherwise, we could just skip the loadSourceForBridge:onProgress:onComplete: and loadSourceForBridge:withBlock: support.

@okwasniewski okwasniewski mentioned this issue Mar 26, 2024
Closed
@okwasniewski
Copy link

Hey @Kudo, I've opened a PR to remove those blocks for now - it's better to at least have support for the basic ones like (sourceURLForBridge) and figure out how to make loadSourceForBridge: work in some later patch to 0.74

@cortinico
Copy link
Collaborator

and figure out how to make loadSourceForBridge: work in some later patch to 0.74

Makes sense 👍 Also @cipolleschi can help here once he's back

facebook-github-bot pushed a commit to facebook/react-native that referenced this issue Apr 2, 2024
@okwasniewski @facebook-github-bot
fix: remove loadSourceForBridge in RCTRootViewFactory (#43656)
0c02b26 
Summary:
This PR removes forward declaration of `loadSourceForBridge` methods from the RCTRootViewFactory as it was causing an issue on old architecture, where the RedBox wouldn't popup when metro wasn't running.

As stated by Kudo [here](reactwg/react-native-releases#177):

> the problem was coming from the implementation
> https://github.com/facebook/react-native/blob/00725fadff28bb3c7fed65f208e647f0dab69e75/packages/react-native/React/CxxBridge/RCTCxxBridge.mm#L519-L540
>
> we should dynamically override loadSourceForBridge:onProgress:onComplete: and loadSourceForBridge:withBlock: in RCTRootViewFactory only when AppDelegate override it. one way to achieve this might be tricky that we may need to override respondsToSelector:.
>
> otherwise, we could just skip the loadSourceForBridge:onProgress:onComplete: and loadSourceForBridge:withBlock: support.

There is no straight forward solution to implement this without some _hacks_ so I'm removing this forward block for now.

## Changelog:

[IOS] [FIXED] - remove loadSourceForBridge in RCTRootViewFactory

Pull Request resolved: #43656

Test Plan: CI Green

Reviewed By: rshest

Differential Revision: D55485094

Pulled By: cortinico

fbshipit-source-id: 1e391e0795c3d99686f2805165f64a7715b013f6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Kudo Kudo

Labels
Type Pick Request Pick requests to include commits inside a React Native release
Projects
No open projects
React Native 0.74 Releases
Status: Done / Picked
Milestone
No milestone
Development

No branches or pull requests
4 participants
@Kudo @huntie @cortinico @okwasniewski