Replace path hashing with escaped relative paths #32
Conversation
|
There was a problem hiding this comment.
Pull Request Overview
This PR replaces path hashing with an escaped relative path‐based naming strategy for Node-API modules, simplifying and making module names more descriptive. Key changes include updating the signature for requireNodeAddon to accept a libraryName, refactoring naming functions in path-utils and related tests, and adapting CLI commands and the Babel plugin to use the new naming configuration.
Reviewed Changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react-native-node-api-modules/src/react-native/NativeNodeApiHost.ts | Updated the TurboModule interface to accept libraryName instead of path. |
| packages/react-native-node-api-modules/src/node/path-utils.ts | Removed hash-based naming in favor of an escaped relative path approach. |
| packages/react-native-node-api-modules/src/node/path-utils.test.ts | Updated tests to reflect new relative path expectations. |
| packages/react-native-node-api-modules/src/node/cli/xcframeworks.ts | Modified CLI commands to pass the new stripPathSuffix option. |
| packages/react-native-node-api-modules/src/node/cli/program.ts | Switched to using the xcframeworks subcommand. |
| packages/react-native-node-api-modules/src/node/cli/helpers.ts | Replaced hash-based discriminators with libraryName. |
| packages/react-native-node-api-modules/src/node/babel-plugin/plugin.ts | Adjusted Babel plugin logic to work with the new stripPathSuffix option. |
| packages/react-native-node-api-modules/react-native-node-api-modules.podspec | Updated command construction and logging for xcframeworks copying. |
| packages/react-native-node-api-modules/cpp/CxxNodeApiHostModule.cpp | Changed parameter handling to use libraryName consistently. |
| apps/test-app/babel.config.js | Updated plugin configuration comments to remove outdated option usage. |
Files not reviewed (2)
- packages/react-native-node-api-modules/tsconfig.json: Language not supported
- packages/react-native-node-api-modules/tsconfig.node-tests.json: Language not supported
Comments suppressed due to low confidence (1)
apps/test-app/babel.config.js:3
- Update the Babel plugin configuration documentation to instruct users to use the 'stripPathSuffix' option (or omit options if using defaults) instead of the deprecated 'naming' option.
plugins: [['module:react-native-node-api-modules/babel-plugin', { naming: "package-name" }]],
| }; | ||
|
|
||
| function assertOptions(opts: unknown): asserts opts is PluginOptions { | ||
| assert(typeof opts === "object" && opts !== null, "Expected an object"); | ||
| if ("naming" in opts) { | ||
| assert(typeof opts.naming === "string", "Expected 'naming' to be a string"); | ||
| if ("stripPathSuffix" in opts) { |
There was a problem hiding this comment.
[nitpick] Consider updating the plugin options documentation (and any inline comments) to clearly describe the new 'stripPathSuffix' boolean parameter replacing the old 'naming' option.
Copilot uses AI. Check for mistakes.
| jsi::Value | ||
| CxxNodeApiHostModule::requireNodeAddon(jsi::Runtime &rt, | ||
| const jsi::String libraryName) { | ||
| const std::string libraryNameStr = libraryName.utf8(rt); |
There was a problem hiding this comment.
[nitpick] Update inline comments in the native code to reference 'libraryName' consistently, improving clarity for future maintainers.
Copilot uses AI. Check for mistakes.
kraenhansen
force-pushed
the
kh/path-hash-suffix
branch
from
56280d4 to
5dd3481
Compare
This PR builds upon (and in a large part simplifies and replaces) #12.
After looking with this for a few days, I'm realizing that hashing the path takes away important details when developing.
Instead I suggest a default mode where the library name is derived from the package name followed by two dashes
--and the relative path (escaped, such that all non-alpha-numeric characters are replaced by a dash-).Disabling via
stripPathSuffix: trueandNODE_API_MODULES_STRIP_PATH_SUFFIX=trueIf all dependencies with Node-API modules have only one module, we don't need the path inside the module to disambiguate.
For this use-case I've added a
stripPathSuffix: trueto the Babel plugin andNODE_API_MODULES_STRIP_PATH_SUFFIX=truewhich can be provided when running "pod install" - needless to say, these has to match.https://github.com/callstackincubator/react-native-node-api-modules/blob/56280d4248cb08dc6ac54069216593158170efa9/apps/test-app/babel.config.js#L3
https://github.com/callstackincubator/react-native-node-api-modules/blob/56280d4248cb08dc6ac54069216593158170efa9/packages/react-native-node-api-modules/react-native-node-api-modules.podspec#L9
Passing "library name" instead of "library path"
In preparation for Android (fixing #26) the babel plugin is now passing the library name and the native code is responsible for turning that into the candidate install path passed to
dlopen.