Enable RCT_USE_RN_DEP in test app and assert RCT_USE_PREBUILT_RNCORE != 1#232
Merged
kraenhansen merged 5 commits intomainfrom Sep 16, 2025
Merged
Enable RCT_USE_RN_DEP in test app and assert RCT_USE_PREBUILT_RNCORE != 1#232kraenhansen merged 5 commits intomainfrom
RCT_USE_RN_DEP in test app and assert RCT_USE_PREBUILT_RNCORE != 1#232kraenhansen merged 5 commits intomainfrom
Conversation
6b64353 to
86ff582
Compare
shirakaba
approved these changes
Sep 14, 2025
Collaborator
Author
|
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
Collaborator
Author
|
I updated the description to reflect my change in intent with this PR. |
kraenhansen
commented
Sep 14, 2025
Comment on lines
-49
to
-64
| 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 |
Collaborator
Author
There was a problem hiding this comment.
Drive-by deleting this as we don't support old versions of React Native anyway.
Contributor
There was a problem hiding this comment.
What's the output when users try on an unsupported version?
Collaborator
Author
There was a problem hiding this comment.
An error is raised:
This version of React Native is too old for React Native Node-API
Collaborator
Author
|
Please review in retrospect 🙏 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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).