Refactor C++ loader: extract cache and addon registry, resolve TODOs #104
Conversation
|
| sanitizeLibraryNameInplace(packageName); | ||
| sanitizeLibraryNameInplace(subpath); | ||
| const std::string libraryName = packageName + subpath; |
There was a problem hiding this comment.
The only issue I see with this now being codified in C++ is that the TypeScript code preparing prebuilds would have to have this logic duplicated to produce dynamic libraries with the right "install name" / "SONAME" - not a huge deal, but it is something which could get out of sync. Have you thought about ways to mitigate that risk?
There was a problem hiding this comment.
I believe that every platform has different requirements (and workarounds) on how to translate a "required path" to a shared library on disk/bundle. Some platforms might need a lib prefix, and different OSes have different extensions (if .node doesn't work, then we might try .dll or .so), and then there's Apple with Xcframeworks. I don't think that we should push that to the JS code. If all platforms will use the same JS code (just as any other typical Node-API addon), we would be able to have a single bundle driving it. If new platforms will be added later, then - in theory - there would be no need to rebundle JS just so Babel can output different "mangled paths". WDYT?
PS: I'd also would like to expose via our host a way to influence the search paths/patterns that are being scanned.
| #if USING_PATCHED_BABEL_PLUGIN | ||
| // Strip the extension (if present) | ||
| // NOTE: This is needed when working with updated Babel plugin | ||
| if (auto pos = name.find(".node"); std::string::npos != pos) { |
There was a problem hiding this comment.
This will strip ".node", not just from the end but from anywhere in the string (and to the end), right?
I mean, what would the result be on something like:
"my-lib/some-file-with.node-in-the-middle/libsomething.node"
There was a problem hiding this comment.
Good catch! It's still temporary code but I'll rewrite it to use a stripSuffix function
There was a problem hiding this comment.
If targeting c++20 you can use .ends_with()
| exports = addon.initFun_(env, exports); | ||
|
|
||
| // "Compute" the Fully Qualified Addon Path | ||
| const std::string fqap = addon.packageName_ + addon.subpath_.substr(1); |
There was a problem hiding this comment.
Nit: 🤔 I feel this reads too much like "fully qualified absolute path"
There was a problem hiding this comment.
☝️ probably not a big deal, but perhaps we could use "qualified_path" instead of "fqap" here?
|
You're including two empty files into |
| if (!pathPrefix.empty()) { | ||
| // URL protocol or prefix detected, dispatch via custom resolver | ||
| std::string pathPrefixCopy(pathPrefix); // HACK: Need explicit cast to `std::string` | ||
| if (auto handler = prefixResolvers_.find(pathPrefixCopy); prefixResolvers_.end() != handler) { |
There was a problem hiding this comment.
This feels driven by a requirement outside of my knowledge.
I love if we could include tests for this use-case 🙏
|
|
||
| namespace callstack::nodeapihost { | ||
|
|
||
| AddonRegistry::NodeAddon& AddonRegistry::loadAddon(std::string packageName, |
There was a problem hiding this comment.
Is there any reason why the parameters are passed by value here? 🤔
packages/react-native-node-api-modules/cpp/CxxNodeApiHostModule.cpp
Outdated
Show resolved
Hide resolved
mani3xis
force-pushed
the
mario/wip-refactor-cpp
branch
from
62eafd0 to
881cebb
Compare
…ypoints This big squashed change makes our Babel plugin to replace `require()` calls with `requireNodeAddons()` with 3 parameters, as initially described in #91 While squashing, I also removed my attempt to reimplement Node's resolution algorithm (https://nodejs.org/api/modules.html#all-together) and replaced it with a call to `require.resolve()`, which basically seems to do the same, just better with less code. This code has been tested with package that uses "main" entry point and has no JavaScript files. Finally, it commit will resolve the TODO regarding scanning the filesystem for addons when using `bindings`. Instead of globbing over the directories, I'm scanning the most popular directories where addons will be placed. Such list of directories was heavily influenced by `node-bindings` itself (https://github.com/TooTallNate/node-bindings/blob/v1.3.0/bindings.js#L21).
If the path points to a file with a `.node` extension, then it must be a Node addon
This small change relaxes the condition for taking the shortcut, as CommonJS modules (in contrast to ESM) do not require developers to explicitly include the file extensions. The Node.js module resolution algorithm (https://nodejs.org/api/modules.html#all-together) in step 4 of LOAD_AS_FILE(X) would try appending the `.node` extension. In theory, we should make sure that other extensions checked in previous steps are not present, but given that we are implementing it for `requireNodeAddon()`, it should be safe to skip those.
In this particular case we don't want to get packageName="sub-package" with path "../addon-2.node"
This is basically the generated CMakeList file (from binding.gyp), but with fixed NAPI_VERSION to 4 (as the conversion tool does not respect this setting)
…tion Early implementation just to get the app running. Without this change, the application crashes immediately after process was created. This is triggered by dynamic linker load addons at start up, and the "twisted addon" has a function decorated with `__attribute__ ((constructor))` which calls `napi_module_register()` before even Weak NodeAPI has change to inject the proper function pointers...
Currently the addon cannot be resolved using the bare specifier, therefore this workaround makes it use an intermediate file "index.js" which would import the native addon.
mani3xis
force-pushed
the
mario/wip-refactor-cpp
branch
from
881cebb to
7d82be1
Compare
mani3xis
changed the title
| @@ -0,0 +1,19 @@ | |||
| { | |||
| "name": "@callstackincubator/example-3", | |||
There was a problem hiding this comment.
I didn't intend for these ./examples to be committed into the repo 🤔 The idea was they they would be copied 1-to-1 from the repo owned by Node.js. I only recently changed the copy script to not delete the directory before copying.
Perhaps we could store this in a separate sub-directory? Of the node-addon-examples package?
There was a problem hiding this comment.
I can remove the example from here. Kept it for testing purposes.
There was a problem hiding this comment.
I suppose it would be good to keep around to avoid regressions? 🙂 Perhaps just move it into ./extras instead of ./examples?
I wanted to keep those a bit longer because they will be used in a different source file, but I can revive those functions from Git history, so farewell 👋
The plugin has been patched in this branch (rebased)
{build}/{configuration} directory and refactored pathSuffix naming strategy
#150
kraenhansen
force-pushed
the
main
branch
4 times, most recently
from
90dd405 to
8dac805
Compare
This branch attempts to rewrite the C++ addon host. The main goals are:
generalize Node-API addon loading process, making it more compatible with how Node.js loads modules
a. add support for addons that register themselves using the deprecated
napi_module_register()[62eafd0]b. add support for custom prefixes used in calls to
require(), without makingnode:a special case [3e5f57d]Introduce a (global)
AddonRegistryclass to handle loading and managing Node-API modules, replacing the old module loader implementation [a703626]a. global (and
jsi::Runtimeindependent) registry is required to properly handle calls tonapi_module_register()[62eafd0]Implements proper Node-API version detection from modules, following Node.js's own implementation patterns [0aaf8d7]
Moreover, this PR also generalizes a few concepts, providing the host mechanisms to fully control and override the resolution behavior per-package [ae6ba19] and per-prefix [3e5f57d] (which also handles URLs).
This branch introduces a breaking-change, where I drop support for
requireNodeAddons()taking only one argument. The required babel-plugin is currently extracted to a separate branch: mario/wip-babel-pluginCaution
It is important to note, that the deprecated method of loading modules (via
napi_module_register()) is implemented, but after introducing weak node api injection it does not work on iOS - it's not spec-compliant. The issue is that iOS' dynamic linker preloads all libraries and frameworks when the process is being created. This means that the such deprecated addons will be loaded before even React Native gets initialized (yet alone beforerequireNodeAddon()) is called. The old workaround of adding such frameworks to "Link Binary With Libraries" and marking it "Optional" does not work anymore!