[iOS] Fabric: Post RCTInstanceDidLoadBundle notification after bundle loaded #48082
Conversation
facebook-github-bot
added
the
CLA Signed
facebook-github-bot
added
the
Shared with Meta
| @@ -476,7 +476,11 @@ - (void)_loadScriptFromSource:(RCTSource *)source | |||
| auto script = std::make_unique<NSDataBigString>(source.data); | |||
| const auto *url = deriveSourceURL(source.url).UTF8String; | |||
| _reactInstance->loadScript(std::move(script), url); | |||
| [[NSNotificationCenter defaultCenter] postNotificationName:@"RCTInstanceDidLoadBundle" object:nil]; | |||
| [self callFunctionOnBufferedRuntimeExecutor:[](facebook::jsi::Runtime &_) { | |||
There was a problem hiding this comment.
This jumps to a different thread and then we jumps back to the original one. I'm not super excited about this: what do you think about changing the ReactInstance::loadScript method to take an extra, optional parameter (nullptr as default value) which is a callback and invoke the callback when loadScript finishes?
It should prevent us a thread jump and ensure that we are actually posting the notification when the script has been loaded.
We can check here if the callback is set and invoke it.
There was a problem hiding this comment.
@cipolleschi Done.
| @@ -476,7 +476,11 @@ - (void)_loadScriptFromSource:(RCTSource *)source | |||
| auto script = std::make_unique<NSDataBigString>(source.data); | |||
| const auto *url = deriveSourceURL(source.url).UTF8String; | |||
| _reactInstance->loadScript(std::move(script), url); | |||
| [[NSNotificationCenter defaultCenter] postNotificationName:@"RCTInstanceDidLoadBundle" object:nil]; | |||
| [self callFunctionOnBufferedRuntimeExecutor:[](facebook::jsi::Runtime &_) { | |||
| dispatch_async(dispatch_get_main_queue(), ^{ | |||
There was a problem hiding this comment.
why are we jumping back on the UI thread? I don't think that it is necessary: we should be able to post notification from a background thread.
What happens if we dispatch the notification from a different thread?
There was a problem hiding this comment.
@cipolleschi I just keep the same logic as old arch, please see
. Seems it may have issue when post on Javascript thread directly, not sure fabric wether has potential issue #35556 .
|
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary:
Fixes #47949
Changelog:
[IOS] [FIXED] - Fabric: Post RCTInstanceDidLoadBundle notification after bundle loaded
Test Plan:
Post RCTInstanceDidLoadBundle notification after bundle loaded