feat: add findPackageJSON to node:module#29206
Open
SAY-5 wants to merge 4 commits intooven-sh:mainfrom
Open
Conversation
Bun's node:module polyfill was missing findPackageJSON, causing a SyntaxError on import. Handles bare specifiers (resolves via Bun's resolver), relative/absolute paths, and file:// URLs. Walks up the directory tree to find the nearest package.json. Fixes oven-sh#23898
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds exported findPackageJSON(specifier: string | URL, base?: string | URL) to Node module builtins. Validates and normalizes inputs (including file:// → path), branches for bare vs path specifiers, resolves targets, and walks parent directories to locate and return the nearest package.json or undefined. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/builtins/NodeModuleObject.ts`:
- Around line 28-33: Remove the redundant typeof checks around specifier and
base in NodeModuleObject.ts: since specifier has already been normalized to a
string (or produced from URL.href) you can drop `typeof specifier === "string"`
and just call `specifier.startsWith("file://")`; for base (which may be string
or undefined) replace `typeof base === "string" && base.startsWith("file://")`
with a null-safe check such as `base && base.startsWith("file://")` or
`base?.startsWith("file://")` and then run fileURLToPath accordingly so you
preserve behavior while eliminating unnecessary type guards on specifier and
base.
- Around line 75-80: The code incorrectly assumes `base` is a file by always
calling `path.dirname(base)` before resolving `startDir`; update the
relative-specifier branch (the block using `specifier`, `base`, `baseDir`,
`startDir`, `path.dirname`, and `path.resolve`) to first detect whether `base`
is a directory (e.g., using `fs.statSync(base).isDirectory()` in a try/catch or
`fs.existsSync` + `statSync`) and if so use `base` as `baseDir`, otherwise fall
back to `path.dirname(base)`; keep the existing `path.resolve(baseDir,
specifier)` logic and handle errors from stat checks gracefully by falling back
to `dirname`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d3afeb87-297e-4240-944d-7d68e5814475
📒 Files selected for processing (1)
src/js/builtins/NodeModuleObject.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/builtins/NodeModuleObject.ts`:
- Around line 47-68: The fallback that resolves a bare specifier using
$resolveSync and then returns the first package.json walking upward can return a
nested manifest; change findPackageJSON's fallback so after resolving the module
entry (using $resolveSync(specifier, base, ...)), walk up to find the package
root boundary (stop when parent directory is a node_modules directory or when
you reach filesystem root) and then return the package.json in that package root
directory; do not return an inner package.json encountered before reaching the
node_modules boundary (handle scoped packages by treating
"node_modules/@scope/<pkg>" as the boundary as well).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d6c7ea8a-c750-4d3d-b6d2-bde8c808dcb1
📒 Files selected for processing (1)
src/js/builtins/NodeModuleObject.ts
…JSON The fallback path was returning the first package.json found walking upward, which could be a nested manifest inside a subdirectory. For bare specifiers Node.js returns the package root manifest, which is the directory directly inside node_modules/ (or node_modules/@scope/pkg for scoped packages). Now walks up until hitting the node_modules boundary and returns that directory's package.json.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/builtins/NodeModuleObject.ts`:
- Around line 42-50: The code currently resolves specifier + "/package.json"
which can return nested subpath manifests for inputs like "pkg/subpath" or
"@scope/pkg/subpath"; update the isBare branch to first strip subpaths from
specifier by extracting the package-name-only (if specifier starts with "@" take
the first two path segments, otherwise take the first segment), then call
$resolveSync with that packageName + "/package.json" (still checking base
undefined as currently done) before probing fs.existsSync; use the existing
symbols isBare, base, specifier, and $resolveSync to locate and change this
logic so you always resolve the package-root manifest.
- Around line 18-25: After the URL-to-string conversion of the local variable
base in NodeModuleObject.ts, validate that base represents an absolute module
location: if base was a URL ensure its protocol is "file" (accept "file" or
"file:" before conversion), and if it's a string ensure path.isAbsolute(base) is
true; otherwise throw $ERR_INVALID_ARG_VALUE for "base". Do this before calling
$resolveSync() or path.resolve() so relative paths or non-file URLs are rejected
early; retain the existing $ERR_INVALID_ARG_TYPE check for non-string/URL inputs
and reference the same local variable name base when adding the new validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0241efdd-f1ef-4ec3-8ffb-f90f3a89216f
📒 Files selected for processing (1)
src/js/builtins/NodeModuleObject.ts
Two issues from review: - base must be a file: URL or absolute path, not a relative string or non-file URL. Now rejects early with ERR_INVALID_ARG_VALUE. - Bare specifiers like 'pkg/subpath' or '@scope/pkg/sub' were resolving subpath manifests. Now strips to package name only before looking up package.json.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
`import { findPackageJSON } from 'node:module'` throws a SyntaxError because the export doesn't exist in Bun's polyfill. Node.js added this in v22.
Implements the full API: handles bare specifiers (resolves via Bun's resolver first), relative and absolute paths, and file:// URLs. Walks up the directory tree from the resolved location to find the nearest package.json.
Fixes #23898