Conversation
8c42996 to
84df9c3
Compare
Built with: ```sh ./build_all_react_native.sh ```
84df9c3 to
e5b8cd2
Compare
shirakaba
left a comment
There was a problem hiding this comment.
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.
ℹ️ Now we do want to build a .framework instead of a .dylib.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yep, we'd always produce a framework instead of a bare dylib. As for distribution, the file that @nativescript/macos-node-api distributes has changed as follows:
- "dist/macos/NativeScript.node",
+ "build/RelWithDebInfo/NativeScript.apple.node",Usage from Node.js is the same as before, though:
import "@nativescript/macos-node-api";
console.log(NSObject.new());
console.log(NSDate.date());
console.log(typeof NSString.stringWithUTF8String("Hello, world!"));This is made possible by the new changes to packages/macos/index.js. We now call process.dlopen() on a path that digs into the framework down to the dylib:
const path =
"./build/RelWithDebInfo/NativeScript.apple.node/macos-arm64_x86_64/NativeScript.framework/Versions/0.1.0/NativeScript";| # 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.
ℹ️ 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.
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.
ℹ️ This now comes from build_utils.sh.
| 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.
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.
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.
Makes sense here. We're loading it from a specific path in the JS entry point. macOS distribution should be fine then.
e5b8cd2 to
cd67c22
Compare
About
This PR adds React Native support, following the
react-native-node-apiprebuilds 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/iospackages/macosHow 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 diffcommit to restore the metadata, types, and binary changes.