-
Notifications
You must be signed in to change notification settings - Fork 7
Enable RCT_USE_RN_DEP in test app and assert RCT_USE_PREBUILT_RNCORE != 1
#232
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
Conversation
6b64353 to
86ff582
Compare
|
Converted this into draft: I don't think we'll actually want to merge and add this matrix. We'll likely just pick one configuration (most likely |
RCT_USE_RN_DEP in test app and assert RCT_USE_PREBUILT_RNCORE != 0
RCT_USE_RN_DEP in test app and assert RCT_USE_PREBUILT_RNCORE != 0RCT_USE_RN_DEP in test app and assert RCT_USE_PREBUILT_RNCORE != 1
|
I updated the description to reflect my change in intent with this PR. |
| s.dependency "React-Core" | ||
| # Don't install the dependencies when we run `pod install` in the old architecture. | ||
| if ENV['RCT_NEW_ARCH_ENABLED'] == '1' then | ||
| # TODO: Re-visit these dependencies and flags and remove them if not needed. | ||
| s.compiler_flags = folly_compiler_flags + " -DRCT_NEW_ARCH_ENABLED=1" | ||
| s.pod_target_xcconfig = { | ||
| "HEADER_SEARCH_PATHS" => "\"$(PODS_ROOT)/boost\"", | ||
| "OTHER_CPLUSPLUSFLAGS" => "-DFOLLY_NO_CONFIG -DFOLLY_MOBILE=1 -DFOLLY_USE_LIBCPP=1", | ||
| "CLANG_CXX_LANGUAGE_STANDARD" => "c++17" | ||
| } | ||
| s.dependency "React-Codegen" | ||
| s.dependency "RCT-Folly" | ||
| s.dependency "RCTRequired" | ||
| s.dependency "RCTTypeSafety" | ||
| s.dependency "ReactCommon/turbomodule/core" | ||
| end |
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.
Drive-by deleting this as we don't support old versions of React Native anyway.
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.
What's the output when users try on an unsupported version?
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.
An error is raised:
This version of React Native is too old for React Native Node-API
|
Please review in retrospect 🙏 |
As an alternative to #207 want the test app to enable
RCT_USE_RN_DEP(since we can and this is faster) and assert thatRCT_USE_PREBUILT_RNCOREis never enabled (since we cannot support this as the prebuild of React Native Core use a JSI header which we have to patch - resulting in runtime crashes when enabled with our patched Hermes).