Uh oh!
There was an error while loading. Please reload this page.
fix(iOS): add missing forward blocks to RCTRootViewFactory - #43526
fix(iOS): add missing forward blocks to RCTRootViewFactory#43526okwasniewski wants to merge 2 commits into
Conversation
Base commit: 1c52d38 |
facebook-github-bot
commented
Mar 21, 2024
@cortinico has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Kudo
left a comment
There was a problem hiding this comment.
helping review since Riccardo is off. i think we should especially address the respondsToSelector: change to prevent crash.
thanks @okwasniewski again for the great work.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
nit: there're other optional functions in RCTBridgeDelegateshouldBridgeUseCustomJSC:, loadSourceForBridge:onProgress:onComplete:, and loadSourceForBridge:withBlock:.
i don't think those are actually be used nowadays. just a note here that these functions are not going to support.
https://github.com/facebook/react-native/blob/1c52d385de662f034adf05df37073681cf609d78/packages/react-native/React/Base/RCTBridgeDelegate.h#L43-L74
There was a problem hiding this comment.
Looks like shouldBridgeUseCustomJSC: is not referenced at all in the codebase so Im not about it 😅 But I'll add the rest
d93de5c to
e42dd4bCompareKudo
left a comment
There was a problem hiding this comment.
everything looks good now. thanks for following up to comment.
cc @cortinico that you may need to reimport the diff again.
Uh oh!
There was an error while loading. Please reload this page.
facebook-github-bot
commented
Mar 21, 2024
@cortinico merged this pull request in 9d79f05. |
cortinico
commented
Mar 21, 2024
oops I missed those comments. I'll follow-up with the delta tomorrow 👍 |
cortinico
commented
Mar 22, 2024
@okwasniewski@Kudo here the diff: Can I ask you to cross check that I haven't missed anything? |
…act#43607) Summary: PR react#43526 was accidentally merged with several changes excluded. I'm following up on those here. Changelog: [Internal] [Changed] - Follow-up with Review Feedback on RCTAppDelegate from react#43526 Differential Revision: D55240435
Summary: Pull Request resolved: #43607 PR #43526 was accidentally merged with several changes excluded. I'm following up on those here. Changelog: [Internal] [Changed] - Follow-up with Review Feedback on RCTAppDelegate from #43526 Reviewed By: dmytrorykun Differential Revision: D55240435 fbshipit-source-id: c296a1e14b7032b211551334ca7b5a6824e8d45c
cortinico
commented
Mar 22, 2024
@okwasniewski@Kudo Also if you could create a PR with the two commits included against the 0.74-stable branch, it would be extremely helpful for us 🙏 |
Summary: This PR adds missing forwarding blocks to RCTRootViewFactory, currently when a user tries to override `sourceURLForBridge` in AppDelegate it isn't overridden. ## Changelog: [IOS] [FIXED] - add missing forward blocks to RCTRootViewFactory Pull Request resolved: react/react-native#43526 Test Plan: Override: `extraModulesForBridge`, `extraLazyModuleClassesForBridge`, `bridge didNotFindModule`, `sourceURLForBridge:` methods in AppDelegate and check if they are called on old architecture Reviewed By: philIip Differential Revision: D55186872 Pulled By: cortinico fbshipit-source-id: 5988c7bab1439ccc4885b7337336c1e120ba9ea6 (cherry picked from commit 9d79f05)
Summary: Pull Request resolved: react/react-native#43607 PR #43526 was accidentally merged with several changes excluded. I'm following up on those here. Changelog: [Internal] [Changed] - Follow-up with Review Feedback on RCTAppDelegate from #43526 Reviewed By: dmytrorykun Differential Revision: D55240435 fbshipit-source-id: c296a1e14b7032b211551334ca7b5a6824e8d45c (cherry picked from commit 84c1c6e)
Kudo
commented
Mar 25, 2024
done here: reactwg/react-native-releases#177 and the pr is also merged and based on 0.74-stable |
* fix(iOS): add missing forward blocks to RCTRootViewFactory (#43526) Summary: This PR adds missing forwarding blocks to RCTRootViewFactory, currently when a user tries to override `sourceURLForBridge` in AppDelegate it isn't overridden. ## Changelog: [IOS] [FIXED] - add missing forward blocks to RCTRootViewFactory Pull Request resolved: #43526 Test Plan: Override: `extraModulesForBridge`, `extraLazyModuleClassesForBridge`, `bridge didNotFindModule`, `sourceURLForBridge:` methods in AppDelegate and check if they are called on old architecture Reviewed By: philIip Differential Revision: D55186872 Pulled By: cortinico fbshipit-source-id: 5988c7bab1439ccc4885b7337336c1e120ba9ea6 (cherry picked from commit 9d79f05) * Follow-up with Review Feedback on RCTAppDelegate from #43526 (#43607) Summary: Pull Request resolved: #43607 PR #43526 was accidentally merged with several changes excluded. I'm following up on those here. Changelog: [Internal] [Changed] - Follow-up with Review Feedback on RCTAppDelegate from #43526 Reviewed By: dmytrorykun Differential Revision: D55240435 fbshipit-source-id: c296a1e14b7032b211551334ca7b5a6824e8d45c (cherry picked from commit 84c1c6e) --------- Co-authored-by: Oskar Kwaśniewski <oskarkwasniewski@icloud.com> Co-authored-by: Nicola Corti <ncor@meta.com>
Summary:
This PR adds missing forwarding blocks to RCTRootViewFactory, currently when a user tries to override
sourceURLForBridgein AppDelegate it isn't overridden.Changelog:
[IOS] [FIXED] - add missing forward blocks to RCTRootViewFactory
Test Plan:
Override:
extraModulesForBridge,extraLazyModuleClassesForBridge,bridge didNotFindModule,sourceURLForBridge:methods in AppDelegate and check if they are called on old architecture