fix(iOS): add missing forward blocks to RCTRootViewFactory #43526

okwasniewski · 2024-03-18T12:55:11Z

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

Test Plan:

Override: extraModulesForBridge, extraLazyModuleClassesForBridge, bridge didNotFindModule, sourceURLForBridge: methods in AppDelegate and check if they are called on old architecture

analysis-bot · 2024-03-18T13:28:28Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	18,127,323	+33,633
android	hermes	armeabi-v7a	n/a	--
android	hermes	x86	n/a	--
android	hermes	x86_64	n/a	--
android	jsc	arm64-v8a	21,493,881	+31,086
android	jsc	armeabi-v7a	n/a	--
android	jsc	x86	n/a	--
android	jsc	x86_64	n/a	--

Base commit: 1c52d38
Branch: main

facebook-github-bot · 2024-03-21T11:06:42Z

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

Kudo

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.

packages/react-native/Libraries/AppDelegate/RCTRootViewFactory.mm

packages/react-native/Libraries/AppDelegate/RCTAppDelegate.mm

Kudo · 2024-03-21T11:55:56Z

packages/react-native/Libraries/AppDelegate/RCTRootViewFactory.h

+ * If the module was not registered, return NO to prevent further searches.
+ */
+@property (nonatomic, nullable) RCTBridgeDidNotFindModuleBlock bridgeDidNotFindModule;
+


nit: there're other optional functions in RCTBridgeDelegate
shouldBridgeUseCustomJSC:, 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.

react-native/packages/react-native/React/Base/RCTBridgeDelegate.h

Lines 43 to 74 in 1c52d38

/**

* Configure whether the JSCExecutor created should use the system JSC API or

* alternative hooks provided. When returning YES from this method, you must have

* previously called facebook::react::setCustomJSCWrapper.

*

* @experimental

*/

- (BOOL)shouldBridgeUseCustomJSC:(RCTBridge *)bridge;

/**

* The bridge will call this method when a module been called from JS

* cannot be found among registered modules.

* It should return YES if the module with name 'moduleName' was registered

* in the implementation, and the system must attempt to look for it again among registered.

* If the module was not registered, return NO to prevent further searches.

*/

- (BOOL)bridge:(RCTBridge *)bridge didNotFindModule:(NSString *)moduleName;

/**

* The bridge will automatically attempt to load the JS source code from the

* location specified by the `sourceURLForBridge:` method, however, if you want

* to handle loading the JS yourself, you can do so by implementing this method.

*/

- (void)loadSourceForBridge:(RCTBridge *)bridge

onProgress:(RCTSourceLoadProgressBlock)onProgress

onComplete:(RCTSourceLoadBlock)loadCallback;

/**

* Similar to loadSourceForBridge:onProgress:onComplete: but without progress

* reporting.

*/

- (void)loadSourceForBridge:(RCTBridge *)bridge withBlock:(RCTSourceLoadBlock)loadCallback;

Looks like shouldBridgeUseCustomJSC: is not referenced at all in the codebase so Im not about it 😅 But I'll add the rest

Kudo

everything looks good now. thanks for following up to comment.
cc @cortinico that you may need to reimport the diff again.

packages/react-native/Libraries/AppDelegate/RCTRootViewFactory.mm

facebook-github-bot · 2024-03-21T22:34:05Z

@cortinico merged this pull request in 9d79f05.

cortinico · 2024-03-21T23:04:03Z

everything looks good now. thanks for following up to comment.
cc @cortinico that you may need to reimport the diff again.

oops I missed those comments. I'll follow-up with the delta tomorrow 👍

Summary: PR facebook#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 facebook#43526 Differential Revision: D55240435

cortinico · 2024-03-22T11:03:16Z

@okwasniewski @Kudo here the diff:

Follow-up with Review Feedback on RCTAppDelegate from #43526 #43607

Can I ask you to cross check that I haven't missed anything?

…acebook#43607) Summary: PR facebook#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 facebook#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 · 2024-03-22T12:16:17Z

@okwasniewski @Kudo
Can I ask you to add a pick request with #43526 and #43607 here:
https://github.com/reactwg/react-native-releases/issues/new/choose

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 🙏

…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: facebook#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)

…acebook#43607) Summary: Pull Request resolved: facebook#43607 PR facebook#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 facebook#43526 Reviewed By: dmytrorykun Differential Revision: D55240435 fbshipit-source-id: c296a1e14b7032b211551334ca7b5a6824e8d45c (cherry picked from commit 84c1c6e)

Kudo · 2024-03-25T06:32:31Z

Can I ask you to add a pick request with #43526 and #43607 here: https://github.com/reactwg/react-native-releases/issues/new/choose

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 🙏

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 Kwaśniewski <oskarkwasniewski@icloud.com> Co-authored-by: Nicola Corti <ncor@meta.com>

cortinico approved these changes Mar 21, 2024

View reviewed changes

Kudo reviewed Mar 21, 2024

View reviewed changes

fix(iOS): add missing forward blocks to RCTRootViewFactory

e42dd4b

okwasniewski force-pushed the feat/root-view-factory-missing-forwarding-methods branch from d93de5c to e42dd4b Compare March 21, 2024 15:11

feat: add loadSouceForBridge

c48ec2a

Kudo reviewed Mar 21, 2024

View reviewed changes

philIip reviewed Mar 21, 2024

View reviewed changes

packages/react-native/Libraries/AppDelegate/RCTRootViewFactory.mm Show resolved Hide resolved

facebook-github-bot closed this in 9d79f05 Mar 21, 2024

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

cortinico mentioned this pull request Mar 22, 2024

Follow-up with Review Feedback on RCTAppDelegate from #43526 #43607

Closed

This was referenced Mar 25, 2024

fix(iOS): add missing forward blocks to RCTRootViewFactory #43638

Merged

[0.74] fix(iOS): add missing forward blocks to RCTRootViewFactory reactwg/react-native-releases#177

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(iOS): add missing forward blocks to RCTRootViewFactory #43526

fix(iOS): add missing forward blocks to RCTRootViewFactory #43526

okwasniewski commented Mar 18, 2024

analysis-bot commented Mar 18, 2024 •

edited

Loading

facebook-github-bot commented Mar 21, 2024

Kudo left a comment

Kudo Mar 21, 2024

okwasniewski Mar 21, 2024

Kudo left a comment

facebook-github-bot commented Mar 21, 2024

cortinico commented Mar 21, 2024

cortinico commented Mar 22, 2024

cortinico commented Mar 22, 2024

Kudo commented Mar 25, 2024

	/**
	* Configure whether the JSCExecutor created should use the system JSC API or
	* alternative hooks provided. When returning YES from this method, you must have
	* previously called facebook::react::setCustomJSCWrapper.
	*
	* @experimental
	*/
	- (BOOL)shouldBridgeUseCustomJSC:(RCTBridge *)bridge;

	/**
	* The bridge will call this method when a module been called from JS
	* cannot be found among registered modules.
	* It should return YES if the module with name 'moduleName' was registered
	* in the implementation, and the system must attempt to look for it again among registered.
	* If the module was not registered, return NO to prevent further searches.
	*/
	- (BOOL)bridge:(RCTBridge )bridge didNotFindModule:(NSString )moduleName;

	/**
	* The bridge will automatically attempt to load the JS source code from the
	* location specified by the `sourceURLForBridge:` method, however, if you want
	* to handle loading the JS yourself, you can do so by implementing this method.
	*/
	- (void)loadSourceForBridge:(RCTBridge *)bridge
	onProgress:(RCTSourceLoadProgressBlock)onProgress
	onComplete:(RCTSourceLoadBlock)loadCallback;

	/**
	* Similar to loadSourceForBridge:onProgress:onComplete: but without progress
	* reporting.
	*/
	- (void)loadSourceForBridge:(RCTBridge *)bridge withBlock:(RCTSourceLoadBlock)loadCallback;

fix(iOS): add missing forward blocks to RCTRootViewFactory #43526

fix(iOS): add missing forward blocks to RCTRootViewFactory #43526

Conversation

okwasniewski commented Mar 18, 2024

Summary:

Changelog:

Test Plan:

analysis-bot commented Mar 18, 2024 • edited Loading

facebook-github-bot commented Mar 21, 2024

Kudo left a comment

Choose a reason for hiding this comment

Kudo Mar 21, 2024

Choose a reason for hiding this comment

okwasniewski Mar 21, 2024

Choose a reason for hiding this comment

Kudo left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Mar 21, 2024

cortinico commented Mar 21, 2024

cortinico commented Mar 22, 2024

cortinico commented Mar 22, 2024

Kudo commented Mar 25, 2024

analysis-bot commented Mar 18, 2024 •

edited

Loading