Use package name and relative paths when hashing #11
Conversation
|
There was a problem hiding this comment.
Pull Request Overview
This PR modifies how module paths are hashed by reverting the change to use absolute paths and instead basing the hash on the package name plus the relative path from the package root. It also updates helper functions, CLI commands, test cases, and the babel plugin to work with the new module context and asynchronous file system operations.
- Reverts absolute path hashing in favor of package name and relative path based hashing.
- Refactors file system operations to use asynchronous/promisified APIs and updates tests.
- Updates CLI commands and babel-plugin integration to reflect the new hashing logic.
Reviewed Changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/react-native-node-api-modules/src/node/path-utils.ts | Introduces ModuleContext, determineModuleContext, and refactors hashModulePath and getLibraryInstallName. |
| packages/react-native-node-api-modules/src/node/path-utils.test.ts | Adds tests covering module context determination and hash consistency across different filesystem contexts. |
| packages/react-native-node-api-modules/src/node/cli/program.ts | Updates CLI commands to use new hashing functions and refactors listener settings. |
| packages/react-native-node-api-modules/src/node/cli/helpers.ts | Refactors dependency resolution and xcframework discovery using async file system operations. |
| packages/react-native-node-api-modules/src/node/cli/helpers.test.ts | Updates tests for helper functions, ensuring proper behavior of xcframework lookup. |
| packages/react-native-node-api-modules/src/node/babel-plugin/plugin.ts | Updates plugin to reference the new getLibraryInstallName function. |
| packages/react-native-node-api-modules/src/node/babel-plugin/plugin.test.ts | Adapts tests to generate module hash using the updated function. |
| packages/node-addon-examples/scripts/copy-examples.mts | Adds a post-copy step to delete duplicate package.json files to prevent hash collisions. |
Files not reviewed (1)
- packages/react-native-node-api-modules/package.json: Language not supported
| if (json) { | ||
| console.log(JSON.stringify(dependencies, null, 2)); | ||
| } else { | ||
| const dependencyCount = Object.keys(dependencies); |
There was a problem hiding this comment.
Dependency count is stored as an array of keys rather than a numeric count. To improve clarity, change it to a number by using 'Object.keys(dependencies).length'.
| const dependencyCount = Object.keys(dependencies); | |
| const dependencyCount = Object.keys(dependencies).length; |
| export function normalizeModulePath(modulePath: string) { | ||
| // Transforming platform specific paths to a common path | ||
| if (path.extname(modulePath) !== ".node") { | ||
| return hashNodeApiModulePath(replaceWithNodeExtension(modulePath)); | ||
| return normalizeModulePath(replaceWithNodeExtension(modulePath)); |
There was a problem hiding this comment.
Nit: I think we introduce an unnecessary extra stack frame here.
There was a problem hiding this comment.
I've replaced this with a reassignment 👍
I recently changed path hashing to use the absolute path of the dynamic library on disk, because I was experiencing collisions with sub-packages in the node-addon-examples having the same name. But this will be an issue with over-the-air updates, as the JS bundle would get a tight coupling with the FS it was bundled on - I don't think that's the right tradeoff.
This PR reverts that change and instead traverse the file-system to look for the nearest package upwards from the dynamic library (see
determineModuleContext) and compute the hash based on that package name + the relative path (seegetHashModulePathInput) from the package root to the dynamic library.To work around the collisions in the node-addon-examples, I suggest simply deleting the package.json of those sub-packages for now.