Create bundle registry in Executor and pass bundle to register bundle (follow-up to #27844) #27855
Conversation
facebook-github-bot
added
the
CLA Signed
simka
changed the title
ejanzer
left a comment
ejanzer
left a comment
There was a problem hiding this comment.
I think this looks good to me, except for the commented out parts - and it's simpler, which is nice. I'll have to import it to test it internally to make sure it doesn't break stuff, but it looks good!
React/CxxBridge/RCTCxxBridge.mm
Outdated
| { | ||
| if (_reactInstance) { | ||
| _reactInstance->registerBundle(static_cast<uint32_t>(segmentId), path.UTF8String); | ||
| /* _reactInstance->registerBundle(static_cast<uint32_t>(segmentId), path.UTF8String); */ |
There was a problem hiding this comment.
Did you mean to leave this commented out?
ReactCommon/cxxreact/Instance.cpp
Outdated
| uint32_t bundleId, | ||
| const std::string &bundlePath) { | ||
| nativeToJsBridge_->registerBundle(bundleId, bundlePath); | ||
| /* nativeToJsBridge_->registerBundle(bundleId, bundlePath); */ |
simka
force-pushed
the
feat/ram-bundles-for-multibundle
branch
from
4b115cb to
2985c1b
Compare
simka
force-pushed
the
feat/ram-bundles-for-multibundle
branch
from
2985c1b to
7f0d6db
Compare
simka
force-pushed
the
feat/ram-bundles-for-multibundle
branch
from
a21b7d9 to
d34c83a
Compare
| __weak RCTCxxBridge *weakSelf = self; | ||
| NSURL *pathURL = [NSURL URLWithString:path]; | ||
| dispatch_group_t group = dispatch_group_create(); | ||
|
|
||
| dispatch_group_enter(group); | ||
| [RCTJavaScriptLoader loadBundleAtURL:pathURL onProgress:^(RCTLoadingProgress *progressData) {} onComplete:^(NSError *error, RCTSource *source) { | ||
| if (error) { | ||
| [weakSelf handleError:error]; | ||
| return; | ||
| } | ||
|
|
||
| NSData *sourceCode = source.data; | ||
| __strong RCTCxxBridge *strongSelf = weakSelf; | ||
| if (strongSelf->_reactInstance) { | ||
| [strongSelf executeApplicationScript:sourceCode url:pathURL async:NO]; | ||
| } | ||
| dispatch_group_leave(group); | ||
|
|
||
| }]; | ||
|
|
||
| dispatch_group_wait(group, DISPATCH_TIME_FOREVER); |
There was a problem hiding this comment.
@ejanzer We had to modify the code in registerSegmentWithId and replace it with this one. 2nd argument of registerBundle function on _reactInstance is no longer a string but the JSModulesUnbundle, so in registerSegmentWithId we now use RCTJavaScriptLoader to load ther bundle either from packager server or filesystem and then executeApplicationScript which now can be called multiple times. All of that is wrapped in a dispatch_group to make it synchronous to keep compatibility with the old behaviour. The same code was applied in the 3rd PR as well: #27875
|
This diff adds OTA-like functionality to React Native core which we cannot currently support. From looking at the code it will also break a bunch of stuff at FB so it'll likely be very hard to integrate. It's confusing to add a "bundleId" to so many callsites, I don't think it is necessary to increase complexity like this. I think we should take a different approach. I landed a diff that exposes a new loader on the bridge, which will allow you to build your own TurboModule to load bundles outside of React Native. It also comes with an example TurboModule that should only be used for development purposes (ie. we will not be accepting changes to this module to make it work in prod). This should give you an idea of what it would take to build your own OTA client including downloading, verification, storage and evaluation of code outside of RN core. See ad879e5 |
|
This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
github-actions
bot
added
the
Stale
|
This PR was closed because it has been stalled for 7 days with no activity. |
5 participants
Summary
This is the second of three PRs related to enabling multi-bundle support in React Native. First one can be found here. More details, motivation and reasoning behind multi-bundle support can be found in RFC here.
As the first one, it's based on @dratwas work from here, but applied to current
masterto avoid rebasing 3-months old branch and issues that come with that.Changelog
[Internal] [Changed] - Create bundle registry in Executor and pass bundle to register bundle to enable multi-bundle support in the future
Test Plan
Initialized new RN app with CLI, set RN to build from source and verified the still app builds and runs OK using code from this branch.