Fix visualizing duplicate library names #282
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the duplicate library name visualization in the CLI to make it clearer which libraries are actually duplicated when react-native-node-api link fails.
Key changes:
- Refactored library map visualization logic into reusable functions
- Improved error messaging to show only duplicated libraries instead of all libraries
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/host/src/node/path-utils.ts | Extracted getLibraryMap and visualizeLibraryMap functions, removed duplicate detection logic |
| packages/host/src/node/duplicates.ts | Deleted file containing generic findDuplicates utility |
| packages/host/src/node/cli/program.ts | Updated to use new getLibraryMap and visualizeLibraryMap functions |
| packages/host/src/node/cli/link-modules.ts | Replaced hasDuplicateLibraryNames with inline duplicate detection, improved error message to show only duplicates |
packages/host/src/node/path-utils.ts
Outdated
|
|
||
| export function getLibraryMap( | ||
| modulePaths: string[], | ||
| // TODO: Default to iterating and printing for all supported naming strategies |
There was a problem hiding this comment.
The TODO comment references 'printing' but this function no longer handles printing/logging - that responsibility has been moved to visualizeLibraryMap. Update the TODO to reflect that this is about supporting multiple naming strategies for building the map, or move it to visualizeLibraryMap if the intent is about visualizing multiple strategies.
| // TODO: Default to iterating and printing for all supported naming strategies | |
| // TODO: Default to iterating for all supported naming strategies |
| failText: () => | ||
| `Failed to link ${platformDisplayName} Node-API modules into ${prettyPath( | ||
| platformOutputPath, | ||
| )}: ${error.message}`, | ||
| )}`, |
There was a problem hiding this comment.
The error parameter was removed from the failText callback, but the error's message is no longer included in the output. This loses valuable debugging information. Consider retaining the error parameter and including error.message in the error text, or document why the error details are intentionally omitted.
1 participant
In the case of duplicate library names when failing
react-native-node-api linkall libraries would be printed with no obvious visualization on what libraries were actually duplicates.Merging this PR will: