Skip to content

Commit

Permalink
[ios][expotools] Fixes for versioned TurboModules (#9862)
Browse files Browse the repository at this point in the history
# Why

After versioning, TurboModules integration started to make more harm than good.

# How

- moved creating of the `JSCExecutorFactory`, which should be versioned, to versioned realm (otherwise we try to pass an unversioned code into a versioned bridge) (huge thanks to C++ senpai @wkozyra95 for assistance)
  - to achieve this I unfortunately had to modify `react-native` to
    1. expect `void *` from `jsExecutorFactoryForBridge:`, otherwise we can't pass versioned factory through unversioned and back into versioned
    2. do not detect whether delegate is an instance of `RCTCxxBridgeDelegate` by protocol, but by the actual method used. This is because delegate for all bridges is always `EXReactAppManager` which is not versioned.
  - another change is that `EXVersionManager` now **holds strongly** one instance of the `JSCExecutorFactory` instead of just creating it on every call. I think this shouldn't break anything, as on each bridge reload we create a new instance of `EXVersionManager` and there shouldn't be more calls to `jsExecutorFactoryForBridge:` apart from when setting the bridge up, but I think it's a note-worthy mention here.
- then I bumped into the `XX is not a registered callable module` error which led me to update `expotools`'s pattern for adding `EX_REMOVE_VERSION` to `MessageQueue`.

# Test Plan

I have verified that these fixes work on a separate branch.
  • Loading branch information
sjchmiela committed Aug 25, 2020
1 parent 3e69bc1 commit b42dcb4
Show file tree
Hide file tree
Showing 5 changed files with 36 additions and 20 deletions.
18 changes: 2 additions & 16 deletions ios/Exponent/Kernel/ReactAppManager/EXReactAppManager.mm
Expand Up @@ -21,7 +21,6 @@
#import <React/RCTCxxBridgeDelegate.h>
#import <React/JSCExecutorFactory.h>
#import <React/RCTRootView.h>
#import <ReactCommon/RCTTurboModuleManager.h>

@interface EXVersionManager (Legacy)
// TODO: remove after non-unimodules SDK versions are dropped
Expand Down Expand Up @@ -66,7 +65,6 @@ @interface EXReactAppManager () <RCTBridgeDelegate, RCTCxxBridgeDelegate>
@property (nonatomic, copy) RCTSourceLoadBlock loadCallback;
@property (nonatomic, strong) NSDictionary *initialProps;
@property (nonatomic, strong) NSTimer *viewTestTimer;
@property (nonatomic, strong) RCTTurboModuleManager *turboModuleManager;

@end

Expand Down Expand Up @@ -720,21 +718,9 @@ - (NSString *)scopedCachesDirectory
return [[exponentCachesDirectory stringByAppendingPathComponent:escapedExperienceId] stringByStandardizingPath];
}

- (std::unique_ptr<facebook::react::JSExecutorFactory>)jsExecutorFactoryForBridge:(RCTBridge *)bridge
- (void *)jsExecutorFactoryForBridge:(id)bridge
{
__weak __typeof(self) weakSelf = self;
return std::make_unique<facebook::react::JSCExecutorFactory>([weakSelf, bridge](facebook::jsi::Runtime &runtime) {
if (!bridge) {
return;
}
__typeof(self) strongSelf = weakSelf;
if (strongSelf) {
strongSelf->_turboModuleManager = [[RCTTurboModuleManager alloc] initWithBridge:bridge
delegate:strongSelf.versionManager
jsInvoker:bridge.jsCallInvoker];
[strongSelf->_turboModuleManager installJSBindingWithRuntime:&runtime];
}
});
return [_versionManager versionedJsExecutorFactoryForBridge:bridge];
}

@end
2 changes: 2 additions & 0 deletions ios/Exponent/Versioned/Core/EXVersionManager.h
Expand Up @@ -29,4 +29,6 @@
*/
- (NSArray *)extraModulesForBridge:(id)bridge;

- (void *)versionedJsExecutorFactoryForBridge:(id)bridge;

@end
30 changes: 29 additions & 1 deletion ios/Exponent/Versioned/Core/EXVersionManager.mm
Expand Up @@ -35,6 +35,7 @@

#import <objc/message.h>

#import <UMCore/UMDefines.h>
#import <UMFileSystemInterface/UMFileSystemInterface.h>
#import <UMCore/UMModuleRegistry.h>
#import <UMCore/UMModuleRegistryDelegate.h>
Expand Down Expand Up @@ -67,11 +68,12 @@ - (void)reload;

@end

@interface EXVersionManager ()
@interface EXVersionManager () <RCTTurboModuleManagerDelegate>

// is this the first time this ABI has been touched at runtime?
@property (nonatomic, assign) BOOL isFirstLoad;
@property (nonatomic, strong) NSDictionary *params;
@property (nonatomic, strong) RCTTurboModuleManager *turboModuleManager;

@end

Expand Down Expand Up @@ -424,6 +426,17 @@ - (Class)getModuleClassFromName:(const char *)name
return [moduleClass new];
}

- (std::shared_ptr<facebook::react::TurboModule>)getTurboModule:(const std::string &)name
instance:(id<RCTTurboModule>)instance
jsInvoker:(std::shared_ptr<facebook::react::CallInvoker>)jsInvoker
nativeInvoker:(std::shared_ptr<facebook::react::CallInvoker>)nativeInvoker
perfLogger:(id<RCTTurboModulePerformanceLogger>)perfLogger
{
// TODO: ADD
return nullptr;
}


- (NSString *)_experienceId
{
return _params[@"manifest"][@"id"];
Expand All @@ -434,4 +447,19 @@ - (BOOL)_isOpeningHomeInProductionMode
return _params[@"browserModuleClass"] && !_params[@"manifest"][@"developer"];
}

- (void *)versionedJsExecutorFactoryForBridge:(RCTBridge *)bridge
{
UM_WEAKIFY(self);
return new facebook::react::JSCExecutorFactory([UMWeak_self, bridge](facebook::jsi::Runtime &runtime) {
if (!bridge) {
return;
}
UM_ENSURE_STRONGIFY(self);
self->_turboModuleManager = [[RCTTurboModuleManager alloc] initWithBridge:bridge
delegate:self
jsInvoker:bridge.jsCallInvoker];
[self->_turboModuleManager installJSBindingWithRuntime:&runtime];
});
}

@end
2 changes: 1 addition & 1 deletion react-native-lab/react-native
4 changes: 2 additions & 2 deletions tools/expotools/src/versioning/ios/transforms/injectMacros.ts
Expand Up @@ -31,8 +31,8 @@ export function injectMacros(versionName: string): TransformPipeline {
{
// injects macro into `enqueueJSCall:method:args:completion:` method of RCTCxxBridge
paths: 'RCTCxxBridge.mm',
replace: /callJSFunction\(\[module UTF8String\],/,
with: `callJSFunction([${versionName}EX_REMOVE_VERSION(module) UTF8String],`,
replace: /callJSFunction(\s+\(\s+)\[module UTF8String\],/,
with: `callJSFunction$1[${versionName}EX_REMOVE_VERSION(module) UTF8String],`,
},
{
// now that this code is versioned, remove meaningless EX_UNVERSIONED declaration
Expand Down

0 comments on commit b42dcb4

Please sign in to comment.