-
-
Notifications
You must be signed in to change notification settings - Fork 0
feat: React Native support #20
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?
Conversation
8c42996
to
84df9c3
Compare
Built with: ```sh ./build_all_react_native.sh ```
git restore --source=main --staged --worktree packages/macos/types packages/ios/types metadata-generator/metadata packages/macos/build packages/ios/build packages/ios-sim/types packages/ios/dist packages/macos/dist
84df9c3
to
e5b8cd2
Compare
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.
Self-reviewed to walk through the code changes.
elseif(TARGET_PLATFORM_MACOS) | ||
# If building a generic library for macOS, we'll build a dylib instead of a framework | ||
unset(BUILD_FRAMEWORK) | ||
else() |
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.
ℹ️ Now we do want to build a .framework
instead of a .dylib
.
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.
With no other condition unsetting BUILD_FRAMEWORK, we would always produce a framework instead of dylib, is that right? How does it affect distribution of the macOS .node binary build? As of now, we produce a .dylib for macOS, and simply rename it to NativeScript.node.
# if (GENERIC_NAPI) | ||
target_link_options( | ||
${NAME} | ||
PRIVATE | ||
"-Wl" | ||
"-undefined" | ||
"dynamic_lookup" | ||
) | ||
# endif() | ||
if(GENERIC_NAPI) | ||
target_link_options( | ||
${NAME} | ||
PRIVATE | ||
"-Wl" | ||
"-undefined" | ||
"dynamic_lookup" | ||
) | ||
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.
ℹ️ As we're prebuilding this binary on its own and bringing the Node-API implementation separately later, we need to restore GENERIC_NAPI
.
This may have consequences for the Node-API distribution of @nativescript/ios
. If so, please adjust it as needed so that it works for both targets.
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.
I think this is the right change to make as not all builds would require this flag anymore. Since making it backwards compatible, we don't make separate NS FFI and runtime frameworks - its all in one binary and linking should work fine unless building for platforms that dynamically load NativeScript later (GENERIC_NAPI: Node.js and co, React Native). Would be good to test a build against V8 once to make sure it compiles.
set -e | ||
source "$(dirname "$0")/build_utils.sh" | ||
|
||
function to_bool() { |
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.
ℹ️ This now comes from build_utils.sh
.
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.
LGTM 👍
elseif(TARGET_PLATFORM_MACOS) | ||
# If building a generic library for macOS, we'll build a dylib instead of a framework | ||
unset(BUILD_FRAMEWORK) | ||
else() |
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.
With no other condition unsetting BUILD_FRAMEWORK, we would always produce a framework instead of dylib, is that right? How does it affect distribution of the macOS .node binary build? As of now, we produce a .dylib for macOS, and simply rename it to NativeScript.node.
# if (GENERIC_NAPI) | ||
target_link_options( | ||
${NAME} | ||
PRIVATE | ||
"-Wl" | ||
"-undefined" | ||
"dynamic_lookup" | ||
) | ||
# endif() | ||
if(GENERIC_NAPI) | ||
target_link_options( | ||
${NAME} | ||
PRIVATE | ||
"-Wl" | ||
"-undefined" | ||
"dynamic_lookup" | ||
) | ||
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.
I think this is the right change to make as not all builds would require this flag anymore. Since making it backwards compatible, we don't make separate NS FFI and runtime frameworks - its all in one binary and linking should work fine unless building for platforms that dynamically load NativeScript later (GENERIC_NAPI: Node.js and co, React Native). Would be good to test a build against V8 once to make sure it compiles.
// === | ||
|
||
const path = | ||
"./build/RelWithDebInfo/NativeScript.apple.node/macos-arm64_x86_64/NativeScript.framework/Versions/0.1.0/NativeScript"; |
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.
Makes sense here. We're loading it from a specific path in the JS entry point. macOS distribution should be fine then.
About
This PR adds React Native support, following the
react-native-node-api
prebuilds standard, a format which is understood by both Node.js and React Native.Change in structure
Following the prebuilds standard means adopting a new file structure.
packages/ios
packages/macos
How to review
To make this PR easy to review, I have structured it as two commits. The first commit commits all changes, while the second commit reverts all the metadata, types, and binary changes so that you can just see which 19 source files were changed.
Once the PR is approved, I'd just drop that
reduce diff
commit to restore the metadata, types, and binary changes.