-
Notifications
You must be signed in to change notification settings - Fork 12
support new arch, Expo 53+ #59
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -5,7 +5,6 @@ echo "Copying prisma migration files..." | |||||
| MIGRATIONS_TARGET=${CONFIGURATION_BUILD_DIR}/${UNLOCALIZED_RESOURCES_FOLDER_PATH} | ||||||
|
|
||||||
| rm -rf "$MIGRATIONS_TARGET/migrations" | ||||||
| mkdir "$MIGRATIONS_TARGET/migrations" | ||||||
| cp -r ${SRCROOT}/../migrations ${MIGRATIONS_TARGET} | ||||||
| cp -r ${SRCROOT}/../prisma/migrations "${MIGRATIONS_TARGET}/migrations" | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Quote the source path to prevent word splitting. If 🛠️ Proposed fix-cp -r ${SRCROOT}/../prisma/migrations "${MIGRATIONS_TARGET}/migrations"
+cp -r "${SRCROOT}/../prisma/migrations" "${MIGRATIONS_TARGET}/migrations"📝 Committable suggestion
Suggested change
🧰 Tools🪛 Shellcheck (0.11.0)[info] 8-8: Double quote to prevent globbing and word splitting. (SC2086) |
||||||
|
|
||||||
| echo "migration files copied ✅" | ||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,7 +2,12 @@ | |||||||||||||||||||||||||||||||||
| #ifndef query_engine_host_object_h | ||||||||||||||||||||||||||||||||||
| #define query_engine_host_object_h | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| #include "query_engine.h" | ||||||||||||||||||||||||||||||||||
| #include <TargetConditionals.h> | ||||||||||||||||||||||||||||||||||
| #if TARGET_OS_SIMULATOR | ||||||||||||||||||||||||||||||||||
| #include "../engines/ios/QueryEngine.xcframework/ios-arm64_x86_64-simulator/Headers/query_engine.h" | ||||||||||||||||||||||||||||||||||
| #else | ||||||||||||||||||||||||||||||||||
| #include "../engines/ios/QueryEngine.xcframework/ios-arm64/Headers/query_engine.h" | ||||||||||||||||||||||||||||||||||
| #endif | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+5
to
+10
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing platform guard will break non-Apple builds.
Wrap the Apple-specific conditional in a platform check: 🐛 Proposed fix to add platform guard+#if defined(__APPLE__)
`#include` <TargetConditionals.h>
`#if` TARGET_OS_SIMULATOR
`#include` "../engines/ios/QueryEngine.xcframework/ios-arm64_x86_64-simulator/Headers/query_engine.h"
`#else`
`#include` "../engines/ios/QueryEngine.xcframework/ios-arm64/Headers/query_engine.h"
`#endif`
+#else
+#include "query_engine.h"
+#endif📝 Committable suggestion
Suggested change
🧰 Tools🪛 Clang (14.0.6)[error] 5-5: 'TargetConditionals.h' file not found (clang-diagnostic-error) |
||||||||||||||||||||||||||||||||||
| #include <jsi/jsi.h> | ||||||||||||||||||||||||||||||||||
| #include <memory> | ||||||||||||||||||||||||||||||||||
| #include <string> | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,52 +1,119 @@ | ||
| #import "Prisma.h" | ||
| #import <jsi/jsi.h> | ||
| #import <React/RCTBridge.h> | ||
| #import <React/RCTBridge+Private.h> | ||
| #import <React/RCTSurfacePresenterBridgeAdapter.h> | ||
| #import <dispatch/dispatch.h> | ||
| #import <ReactCommon/RCTTurboModule.h> | ||
| #import <UIKit/UIKit.h> | ||
| #import <iostream> | ||
|
|
||
| #ifdef RCT_NEW_ARCH_ENABLED | ||
| #import "RNPrismaSpecJSI.h" | ||
| #endif | ||
|
|
||
| // Forward declare runtimeExecutor to silence selector warnings on older headers | ||
| @interface RCTBridge (RuntimeExecutorForwardDecl) | ||
| - (facebook::react::RuntimeExecutor)runtimeExecutor; | ||
| @end | ||
|
|
||
| static NSString *PrismaLibraryPath() { | ||
| NSArray *paths = NSSearchPathForDirectoriesInDomains(NSLibraryDirectory, NSUserDomainMask, true); | ||
| return [paths objectAtIndex:0]; | ||
| } | ||
|
|
||
| static NSString *PrismaMigrationsPath() { | ||
| auto bundleURL = NSBundle.mainBundle.bundleURL; | ||
| auto migrations_path_absolute = [NSString stringWithFormat:@"%@%@", bundleURL.absoluteString, @"migrations"]; | ||
| return [migrations_path_absolute stringByReplacingOccurrencesOfString:@"file://" withString:@""]; | ||
| } | ||
|
|
||
| static inline void InstallInRuntime(facebook::jsi::Runtime &runtime, | ||
| std::shared_ptr<facebook::react::CallInvoker> callInvoker) { | ||
| NSString *libraryPath = PrismaLibraryPath(); | ||
| NSString *migrationsPath = PrismaMigrationsPath(); | ||
| prisma::install_cxx(runtime, callInvoker, [libraryPath UTF8String], [migrationsPath UTF8String]); | ||
| } | ||
|
|
||
| static RCTBridge *ResolveBridge(id<RCTBridgeModule> module) { | ||
| RCTBridge *bridge = nil; | ||
| if ([module respondsToSelector:@selector(bridge)]) { | ||
| bridge = ((id<RCTBridgeModule>)module).bridge; | ||
| } | ||
| if (bridge == nil) { | ||
| bridge = [RCTBridge currentBridge]; | ||
| } | ||
| return bridge; | ||
| } | ||
|
|
||
| @implementation Prisma | ||
|
|
||
| @synthesize bridge=_bridge; | ||
|
|
||
| RCT_EXPORT_MODULE() | ||
|
|
||
| // Old-arch sync install for classic bridge | ||
| #if !RCT_NEW_ARCH_ENABLED | ||
| RCT_EXPORT_BLOCKING_SYNCHRONOUS_METHOD(install) | ||
| { | ||
| RCTCxxBridge *cxxBridge = (RCTCxxBridge *)_bridge; | ||
| if (cxxBridge == nil) { | ||
| return @false; | ||
| #if DEBUG | ||
| std::cout << "▲ NSHomeDirectory:\n" << [NSHomeDirectory() UTF8String] << std::endl; | ||
| std::cout << "▲ Library Path:\n" << [PrismaLibraryPath() UTF8String] << std::endl; | ||
| std::cout << "▲ Migrations Path:\n" << [PrismaMigrationsPath() UTF8String] << std::endl; | ||
| #endif | ||
|
|
||
| BOOL ok = NO; | ||
| auto okPtr = &ok; | ||
| RCTBridge *bridge = ResolveBridge(self); | ||
|
|
||
| // Try new-arch path first if runtimeExecutor is available | ||
| if (bridge && [bridge respondsToSelector:@selector(runtimeExecutor)]) { | ||
| facebook::react::RuntimeExecutor executor = RCTRuntimeExecutorFromBridge(bridge); | ||
| dispatch_semaphore_t sema = dispatch_semaphore_create(0); | ||
| executor([&](facebook::jsi::Runtime &runtime) { | ||
| InstallInRuntime(runtime, bridge.jsCallInvoker); | ||
| *okPtr = YES; | ||
| dispatch_semaphore_signal(sema); | ||
| }); | ||
| dispatch_semaphore_wait(sema, DISPATCH_TIME_FOREVER); | ||
| return @(ok); | ||
| } | ||
|
Comment on lines
+70
to
80
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potential deadlock if If 🐛 Proposed fix using try-catch if (bridge && [bridge respondsToSelector:`@selector`(runtimeExecutor)]) {
facebook::react::RuntimeExecutor executor = RCTRuntimeExecutorFromBridge(bridge);
dispatch_semaphore_t sema = dispatch_semaphore_create(0);
+ __block NSException *caughtException = nil;
executor([&](facebook::jsi::Runtime &runtime) {
- InstallInRuntime(runtime, bridge.jsCallInvoker);
- *okPtr = YES;
+ `@try` {
+ InstallInRuntime(runtime, bridge.jsCallInvoker);
+ *okPtr = YES;
+ } `@catch` (NSException *e) {
+ caughtException = e;
+ }
dispatch_semaphore_signal(sema);
});
dispatch_semaphore_wait(sema, DISPATCH_TIME_FOREVER);
+ if (caughtException) {
+ `@throw` caughtException;
+ }
return @(ok);
}Note: C++ exceptions from JSI may need different handling (e.g., |
||
|
|
||
| auto jsiRuntime = (facebook::jsi::Runtime *)cxxBridge.runtime; | ||
| if (jsiRuntime == nil) { | ||
|
|
||
| // Fallback to old-arch bridge access | ||
| RCTBridge *legacyBridge = _bridge ?: bridge; | ||
| RCTCxxBridge *cxxBridge = (RCTCxxBridge *)legacyBridge; | ||
| if (cxxBridge == nil || cxxBridge.runtime == nil) { | ||
| NSLog(@"[Prisma] no runtime available to install cxx"); | ||
| return @false; | ||
| } | ||
|
|
||
| auto jsiRuntime = (facebook::jsi::Runtime *)cxxBridge.runtime; | ||
| auto &runtime = *jsiRuntime; | ||
| auto callInvoker = _bridge.jsCallInvoker; | ||
|
|
||
| // get migrations folder | ||
| auto bundleURL = NSBundle.mainBundle.bundleURL; | ||
| auto migrations_path_absolute = [NSString stringWithFormat:@"%@%@", bundleURL.absoluteString, @"migrations"]; | ||
| auto migrations_path = [migrations_path_absolute stringByReplacingOccurrencesOfString:@"file://" withString:@""]; | ||
|
|
||
| NSArray *paths = NSSearchPathForDirectoriesInDomains(NSLibraryDirectory, NSUserDomainMask, true); | ||
| NSString *libraryPath = [paths objectAtIndex:0]; | ||
|
|
||
| #if DEBUG | ||
| std::cout << "▲ NSHomeDirectory:\n" << [NSHomeDirectory() UTF8String] << std::endl; | ||
| std::cout << "▲ Library Path:\n" << [libraryPath UTF8String] << std::endl; | ||
| std::cout << "▲ Migrations Path:\n" << [migrations_path UTF8String] << std::endl; | ||
| #endif | ||
| auto callInvoker = _bridge ? _bridge.jsCallInvoker : legacyBridge.jsCallInvoker; | ||
|
|
||
| prisma::install_cxx(runtime, callInvoker, [libraryPath UTF8String], [migrations_path UTF8String]); | ||
| return nil; | ||
| InstallInRuntime(runtime, callInvoker); | ||
| return @true; | ||
| } | ||
| #else | ||
| // Required by NativePrismaSpec protocol in new-arch; TurboModule path uses C++ spec below. | ||
| - (void)install {} | ||
| #endif | ||
|
|
||
| #ifdef RCT_NEW_ARCH_ENABLED | ||
| - (std::shared_ptr<facebook::react::TurboModule>)getTurboModule: | ||
| (const facebook::react::ObjCTurboModule::InitParams &)params | ||
| { | ||
| return std::make_shared<facebook::react::NativePrismaSpecJSI>(params); | ||
| class PrismaCxxTurboModule final : public facebook::react::NativePrismaCxxSpec<PrismaCxxTurboModule> { | ||
| public: | ||
| explicit PrismaCxxTurboModule(std::shared_ptr<facebook::react::CallInvoker> jsInvoker) | ||
| : NativePrismaCxxSpec(std::move(jsInvoker)) {} | ||
|
|
||
| void install(facebook::jsi::Runtime &rt) { | ||
| InstallInRuntime(rt, this->jsInvoker_); | ||
| } | ||
| }; | ||
|
|
||
| return std::make_shared<PrismaCxxTurboModule>(params.jsInvoker); | ||
| } | ||
| #endif | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Null-safety improvements are good, but the fallback path may still NPE.
The null checks for
jsContextPointerandgetJSCallInvokerHolder()are correct. However, on line 132,context.getCatalystInstance().getJSCallInvokerHolder()may also return null even whengetCatalystInstance()is non-null, leading to an NPE during the cast.🛠️ Proposed fix to add null check
if (context.getJSCallInvokerHolder() != null) { jsCallInvokerHolder = (CallInvokerHolderImpl) context.getJSCallInvokerHolder(); } else if (context.getCatalystInstance() != null) { - jsCallInvokerHolder = (CallInvokerHolderImpl) context.getCatalystInstance().getJSCallInvokerHolder(); + var holder = context.getCatalystInstance().getJSCallInvokerHolder(); + if (holder == null) { + throw new RuntimeException("JSCallInvokerHolder from CatalystInstance is null."); + } + jsCallInvokerHolder = (CallInvokerHolderImpl) holder; } else { throw new RuntimeException("JSCallInvokerHolder is not available yet."); }📝 Committable suggestion