Skip to content

Treat opendir failures as empty directories #186

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jul 17, 2025

Conversation

kraenhansen
Copy link
Collaborator

@kraenhansen kraenhansen commented Jul 16, 2025

Merging this PR fixes #185 by introducing a few changes to the implementation used when looking for Node-API modules on the filesystem:

  • Treat opendir failures as empty directories.
  • Exclude react-native-node-api and react-native packages completely.

@kraenhansen kraenhansen self-assigned this Jul 16, 2025
@kraenhansen kraenhansen added Apple 🍎 Anything related to the Apple platform (iOS, macOS, Cocoapods, Xcode, XCFrameworks, etc.) Android 🤖 Anything related to the Android platform (Gradle, NDK, Android SDK) Linking 🔗 Discovering and copying prebuilds from packages into the host labels Jul 16, 2025
Copy link

changeset-bot bot commented Jul 16, 2025

🦋 Changeset detected

Latest commit: 12e9c20

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
react-native-node-api Patch
cmake-rn Patch
ferric-cli Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@kraenhansen kraenhansen changed the title Tread opendir failures as empty directories Treat opendir failures as empty directories Jul 16, 2025
@kraenhansen kraenhansen requested a review from matthargett July 16, 2025 20:29
Comment on lines +357 to +366
"react-native-node-api", // The host package itself
"react-native", // React Native core
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two known packages known not to have Node-API prebuilds which we want to link in.

@kraenhansen kraenhansen requested a review from Copilot July 16, 2025 20:59
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR updates the Node-API module discovery to treat directory read failures as empty directories and completely exclude the host and React Native packages.

  • Wraps fs.promises.opendir in a try/catch to skip over unreadable directories.
  • Removes the path-based exclusion for react-native-node-api and adds a package-name exclusion list.
  • Filters out react-native-node-api and react-native in findNodeApiModulePathsByDependency and adds a test for the unreadable-directory case.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
packages/host/src/node/path-utils.ts Wrap opendir in try/catch, remove path exclude, add package-name exclude.
packages/host/src/node/path-utils.test.ts Adjust existing test and add new test for unreadable directories.
.changeset/stale-coins-drop.md Add changeset entry summarizing the fix.
Comments suppressed due to low confidence (2)

packages/host/src/node/path-utils.ts:354

  • [nitpick] The comment refers to 'patterns' but the constant is a list of package names. Update the doc to clearly state these are package names being excluded.
 * Default patterns to use when excluding package names from the search for Node-API modules.

packages/host/src/node/path-utils.ts:385

  • There’s no test verifying that 'react-native-node-api' and 'react-native' are actually excluded. Add a test case to cover the excludePackages behavior in findNodeApiModulePathsByDependency.
      .filter(([name]) => !excludePackages.includes(name))

Comment on lines 345 to 346
} catch {
// Intentionally left empty: if the directory cannot be read, we skip it
Copy link
Preview

Copilot AI Jul 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Catching all errors here may mask unexpected issues (e.g., permission vs. path-not-found). Consider narrowing this to specific filesystem errors like ENOENT or EACCES to avoid hiding genuine failures.

Suggested change
} catch {
// Intentionally left empty: if the directory cannot be read, we skip it
} catch (error: any) {
if (error.code === "ENOENT" || error.code === "EACCES") {
// Skip the directory if it does not exist or access is denied
} else {
throw error; // Re-throw unexpected errors
}

Copilot uses AI. Check for mistakes.

@kraenhansen kraenhansen force-pushed the kh/filter-root-node_modules-fixup branch from f5af8fd to 12e9c20 Compare July 17, 2025 09:22
@kraenhansen kraenhansen merged commit acf1a7c into main Jul 17, 2025
6 checks passed
@kraenhansen kraenhansen deleted the kh/filter-root-node_modules-fixup branch July 17, 2025 09:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Android 🤖 Anything related to the Android platform (Gradle, NDK, Android SDK) Apple 🍎 Anything related to the Apple platform (iOS, macOS, Cocoapods, Xcode, XCFrameworks, etc.) Linking 🔗 Discovering and copying prebuilds from packages into the host
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make scanning for auto-linkable files robust to concurrent changes in the fs
1 participant