feat: Implements the Module.findPackageJSON function Node.js - #24098
amustaque97 wants to merge 5 commits into
Conversation
WalkthroughAdds Module.findPackageJSON: a host function exposed from C++ that normalizes file URLs/paths, calls a new Zig-exported implementation to locate the nearest package.json, and returns an absolute path string or null; includes tests and an exported resolver helper. Changes
Suggested reviewers
Pre-merge checks✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📥 CommitsReviewing files that changed from the base of the PR and between fb35fe5eb5112e464f52ceb7427a241eb13c2e42 and 6692eb6. 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (3)**/*.zig📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
Files:
src/**/*.zig📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Files:
src/bun.js/**/*.zig📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
Files:
🔇 Additional comments (1)
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}
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/bun.js/modules/NodeModuleModule.cpp(3 hunks)src/bun.js/node/path.zig(1 hunks)test/js/node/module/node-module-findPackageJSON.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (14)
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/bun.js/node/path.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/node/path.zig
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
When adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
src/**/*.zig: Use Zig private fields with the # prefix for encapsulation (e.g., struct { #foo: u32 })
Prefer Decl literals for initialization (e.g., const decl: Decl = .{ .binding = 0, .value = 0 };)
Place @import statements at the bottom of the file (formatter will handle ordering)
src/**/*.zig: In Zig code, manage memory carefully: use appropriate allocators and defer for cleanup
Cache JavaScriptCore class structures in ZigGlobalObject when adding new classes
Files:
src/bun.js/node/path.zig
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/node/module/node-module-findPackageJSON.test.ts
test/js/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place JavaScript and TypeScript tests under test/js/
Files:
test/js/node/module/node-module-findPackageJSON.test.ts
test/js/node/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place Node.js module compatibility tests under test/js/node/, separated by module (e.g., test/js/node/assert/)
Files:
test/js/node/module/node-module-findPackageJSON.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/js/node/module/node-module-findPackageJSON.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun:testfor files ending with*.test.{ts,js,jsx,tsx,mjs,cjs}
Prefer concurrent tests (test.concurrent/describe.concurrent) over sequential when feasible
Organize tests withdescribeblocks to group related tests
Use utilities likedescribe.each,toMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach) and track resources for cleanup
Files:
test/js/node/module/node-module-findPackageJSON.test.ts
test/**/*.{ts,tsx,js,jsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
For large/repetitive strings, use
Buffer.alloc(count, fill).toString()instead of"A".repeat(count)
Files:
test/js/node/module/node-module-findPackageJSON.test.ts
test/js/{bun,node}/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Organize unit tests by module under
/test/js/bun/and/test/js/node/
Files:
test/js/node/module/node-module-findPackageJSON.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must live under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or invent random port functions
Prefer snapshot assertions and use normalizeBunSnapshot for snapshot output in tests
Never write tests that assert absence of crashes (e.g., no "panic" or "uncaught exception") in output
Use tempDir from "harness" for temporary directories; do not use tmpdirSync or fs.mkdtempSync in tests
When spawning processes in tests, assert on stdout before asserting exitCode
Do not use setTimeout in tests; await conditions instead to avoid flakiness
Files:
test/js/node/module/node-module-findPackageJSON.test.ts
test/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid shell commands in tests (e.g., find, grep); use Bun's Glob and built-in tools
Files:
test/js/node/module/node-module-findPackageJSON.test.ts
**/*.{cpp,h}
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.{cpp,h}: When exposing a JS class with public Constructor and Prototype, define three C++ types: class Foo : public JSC::DestructibleObject (if it has C++ fields), class FooPrototype : public JSC::JSNonFinalObject, and class FooConstructor : public JSC::InternalFunction
If the class has C++ data members, inherit from JSC::DestructibleObject and provide proper destruction; if it has no C++ fields (only JS properties), avoid a class and use JSC::constructEmptyObject(vm, structure) with putDirectOffset
Prefer placing the subspaceFor implementation in the .cpp file rather than the header when possible
Files:
src/bun.js/modules/NodeModuleModule.cpp
**/*.cpp
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.cpp: Include "root.h" at the top of C++ binding files to satisfy lints
Define prototype properties using a const HashTableValue array and declare accessors/functions with JSC_DECLARE_* macros
Prototype classes should subclass JSC::JSNonFinalObject, provide create/createStructure, DECLARE_INFO, finishCreation that reifies static properties, and set mayBePrototype on the Structure
Custom getters should use JSC_DEFINE_CUSTOM_GETTER, jsDynamicCast to validate this, and throwThisTypeError on mismatch
Custom setters should use JSC_DEFINE_CUSTOM_SETTER, validate this via jsDynamicCast, and store via WriteBarrier/set semantics
Prototype functions should use JSC_DEFINE_HOST_FUNCTION, validate this with jsDynamicCast, and return encoded JSValue
Constructors should subclass JSC::InternalFunction, return internalFunctionSpace in subspaceFor, set the prototype property as non-configurable/non-writable, and provide create/createStructure
Provide a setup function that builds the Prototype, Constructor, and Structure, and assigns them to the LazyClassStructure initializer
Use the cached Structure via globalObject->m_.get(globalObject) when constructing instances
Expose constructors to Zig via an extern "C" function that returns the constructor from the LazyClassStructure
Provide an extern "C" Bun____toJS function that creates an instance using the cached Structure and returns an EncodedJSValue
Files:
src/bun.js/modules/NodeModuleModule.cpp
🧠 Learnings (5)
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/js/{bun,node}/** : Organize unit tests by module under `/test/js/bun/` and `/test/js/node/`
Applied to files:
test/js/node/module/node-module-findPackageJSON.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/js/node/**/*.{js,ts} : Place Node.js module compatibility tests under test/js/node/, separated by module (e.g., test/js/node/assert/)
Applied to files:
test/js/node/module/node-module-findPackageJSON.test.ts
📚 Learning: 2025-10-19T02:44:46.354Z
Learnt from: theshadow27
PR: oven-sh/bun#23798
File: packages/bun-otel/context-propagation.test.ts:1-1
Timestamp: 2025-10-19T02:44:46.354Z
Learning: In the Bun repository, standalone packages under packages/ (e.g., bun-vscode, bun-inspector-protocol, bun-plugin-yaml, bun-plugin-svelte, bun-debug-adapter-protocol, bun-otel) co-locate their tests with package source code using *.test.ts files. This follows standard npm/monorepo patterns. The test/ directory hierarchy (test/js/bun/, test/cli/, test/js/node/) is reserved for testing Bun's core runtime APIs and built-in functionality, not standalone packages.
Applied to files:
test/js/node/module/node-module-findPackageJSON.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/js/third_party/**/*.{js,ts} : Place third-party npm package tests under test/js/third_party/
Applied to files:
test/js/node/module/node-module-findPackageJSON.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/**/*.{js,ts} : Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Applied to files:
test/js/node/module/node-module-findPackageJSON.test.ts
🧬 Code graph analysis (1)
src/bun.js/modules/NodeModuleModule.cpp (2)
src/bun.js/bindings/ErrorCode.cpp (14)
throwError(1690-1694)throwError(1690-1690)INVALID_ARG_VALUE(846-864)INVALID_ARG_VALUE(846-846)INVALID_ARG_VALUE(887-904)INVALID_ARG_VALUE(887-887)INVALID_ARG_VALUE(907-943)INVALID_ARG_VALUE(907-907)INVALID_ARG_VALUE(945-972)INVALID_ARG_VALUE(945-945)INVALID_ARG_VALUE(974-999)INVALID_ARG_VALUE(974-974)INVALID_ARG_VALUE(1001-1022)INVALID_ARG_VALUE(1001-1001)src/bun.js/bindings/BunString.cpp (10)
toString(178-181)toString(178-178)toString(208-211)toString(208-208)toString(230-236)toString(230-230)toString(237-243)toString(237-237)toString(244-250)toString(244-244)
🔇 Additional comments (3)
test/js/node/module/node-module-findPackageJSON.test.ts (1)
44-51: Add a negative test for non-file URLs.Ensure an http: URL throws ERR_INVALID_ARG_VALUE, matching the C++ validation.
Suggested test:
+ test.concurrent("rejects non-file URLs", () => { + expect(() => findPackageJSON("http://example.com/x.js")).toThrow(); + });src/bun.js/modules/NodeModuleModule.cpp (2)
292-345: Argument normalization and return semantics look correct; consider minor parity tweaks.
- URL/file-path normalization mirrors createRequire and returns null on miss; good.
- Minor parity: consider accepting trailing-slash paths like directories consistently (optional; Zig handles dir via stat already).
- Memory refs: result WTF::String deref pattern matches existing usage.
If Node accepts URL objects directly, toWTFString should produce href; confirm with a quick check in REPL.
892-893: LUT wiring is correct.Entry exposes findPackageJSON with arity 1 under node:module.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/bun.js/node/path.zig(1 hunks)test/js/node/module/node-module-findPackageJSON.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (12)
test/**
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place all tests under the test/ directory
Files:
test/js/node/module/node-module-findPackageJSON.test.ts
test/js/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place JavaScript and TypeScript tests under test/js/
Files:
test/js/node/module/node-module-findPackageJSON.test.ts
test/js/node/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Place Node.js module compatibility tests under test/js/node/, separated by module (e.g., test/js/node/assert/)
Files:
test/js/node/module/node-module-findPackageJSON.test.ts
test/**/*.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
test/**/*.{js,ts}: Write tests in JavaScript or TypeScript using Bun’s Jest-style APIs (test, describe, expect) and run with bun test
Prefer data-driven tests (e.g., test.each) to reduce boilerplate
Use shared utilities from test/harness.ts where applicable
Files:
test/js/node/module/node-module-findPackageJSON.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun:testfor files ending with*.test.{ts,js,jsx,tsx,mjs,cjs}
Prefer concurrent tests (test.concurrent/describe.concurrent) over sequential when feasible
Organize tests withdescribeblocks to group related tests
Use utilities likedescribe.each,toMatchSnapshot, and lifecycle hooks (beforeAll,beforeEach,afterEach) and track resources for cleanup
Files:
test/js/node/module/node-module-findPackageJSON.test.ts
test/**/*.{ts,tsx,js,jsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
For large/repetitive strings, use
Buffer.alloc(count, fill).toString()instead of"A".repeat(count)
Files:
test/js/node/module/node-module-findPackageJSON.test.ts
test/js/{bun,node}/**
📄 CodeRabbit inference engine (test/CLAUDE.md)
Organize unit tests by module under
/test/js/bun/and/test/js/node/
Files:
test/js/node/module/node-module-findPackageJSON.test.ts
test/**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
test/**/*.test.{ts,tsx}: Test files must live under test/ and end with .test.ts or .test.tsx
In tests, always use port: 0; do not hardcode ports or invent random port functions
Prefer snapshot assertions and use normalizeBunSnapshot for snapshot output in tests
Never write tests that assert absence of crashes (e.g., no "panic" or "uncaught exception") in output
Use tempDir from "harness" for temporary directories; do not use tmpdirSync or fs.mkdtempSync in tests
When spawning processes in tests, assert on stdout before asserting exitCode
Do not use setTimeout in tests; await conditions instead to avoid flakiness
Files:
test/js/node/module/node-module-findPackageJSON.test.ts
test/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid shell commands in tests (e.g., find, grep); use Bun's Glob and built-in tools
Files:
test/js/node/module/node-module-findPackageJSON.test.ts
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/bun.js/node/path.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/node/path.zig
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
When adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
src/**/*.zig: Use Zig private fields with the # prefix for encapsulation (e.g., struct { #foo: u32 })
Prefer Decl literals for initialization (e.g., const decl: Decl = .{ .binding = 0, .value = 0 };)
Place @import statements at the bottom of the file (formatter will handle ordering)
src/**/*.zig: In Zig code, manage memory carefully: use appropriate allocators and defer for cleanup
Cache JavaScriptCore class structures in ZigGlobalObject when adding new classes
Files:
src/bun.js/node/path.zig
🧠 Learnings (9)
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/js/{bun,node}/** : Organize unit tests by module under `/test/js/bun/` and `/test/js/node/`
Applied to files:
test/js/node/module/node-module-findPackageJSON.test.ts
📚 Learning: 2025-08-30T00:12:56.803Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/writing-tests.mdc:0-0
Timestamp: 2025-08-30T00:12:56.803Z
Learning: Applies to test/js/node/**/*.{js,ts} : Place Node.js module compatibility tests under test/js/node/, separated by module (e.g., test/js/node/assert/)
Applied to files:
test/js/node/module/node-module-findPackageJSON.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Prefer concurrent tests (`test.concurrent`/`describe.concurrent`) over sequential when feasible
Applied to files:
test/js/node/module/node-module-findPackageJSON.test.ts
📚 Learning: 2025-10-26T05:04:50.682Z
Learnt from: CR
PR: oven-sh/bun#0
File: CLAUDE.md:0-0
Timestamp: 2025-10-26T05:04:50.682Z
Learning: Applies to test/**/*.{ts,tsx} : Avoid shell commands in tests (e.g., find, grep); use Bun's Glob and built-in tools
Applied to files:
test/js/node/module/node-module-findPackageJSON.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/dev/ecosystem.test.ts : ecosystem.test.ts should focus on concrete library integration bugs rather than whole-package coverage
Applied to files:
test/js/node/module/node-module-findPackageJSON.test.ts
📚 Learning: 2025-10-12T02:22:34.373Z
Learnt from: CR
PR: oven-sh/bun#0
File: test/CLAUDE.md:0-0
Timestamp: 2025-10-12T02:22:34.373Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use utilities like `describe.each`, `toMatchSnapshot`, and lifecycle hooks (`beforeAll`, `beforeEach`, `afterEach`) and track resources for cleanup
Applied to files:
test/js/node/module/node-module-findPackageJSON.test.ts
📚 Learning: 2025-08-30T00:09:39.100Z
Learnt from: CR
PR: oven-sh/bun#0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-08-30T00:09:39.100Z
Learning: Applies to test/bake/{dev/*.test.ts,dev-and-prod.ts} : Do not use node:fs APIs in tests; mutate files via dev.write, dev.patch, and dev.delete
Applied to files:
test/js/node/module/node-module-findPackageJSON.test.ts
📚 Learning: 2025-10-19T02:44:46.354Z
Learnt from: theshadow27
PR: oven-sh/bun#23798
File: packages/bun-otel/context-propagation.test.ts:1-1
Timestamp: 2025-10-19T02:44:46.354Z
Learning: In the Bun repository, standalone packages under packages/ (e.g., bun-vscode, bun-inspector-protocol, bun-plugin-yaml, bun-plugin-svelte, bun-debug-adapter-protocol, bun-otel) co-locate their tests with package source code using *.test.ts files. This follows standard npm/monorepo patterns. The test/ directory hierarchy (test/js/bun/, test/cli/, test/js/node/) is reserved for testing Bun's core runtime APIs and built-in functionality, not standalone packages.
Applied to files:
test/js/node/module/node-module-findPackageJSON.test.ts
📚 Learning: 2025-10-08T13:56:00.875Z
Learnt from: Jarred-Sumner
PR: oven-sh/bun#23373
File: src/bun.js/api/BunObject.zig:2514-2521
Timestamp: 2025-10-08T13:56:00.875Z
Learning: For Bun codebase: prefer using `bun.path` utilities (e.g., `bun.path.joinAbsStringBuf`, `bun.path.join`) over `std.fs.path` functions for path operations.
Applied to files:
src/bun.js/node/path.zig
🔇 Additional comments (3)
src/bun.js/node/path.zig (1)
3025-3043: Good overflow guard and file validation!The length check before memcpy (lines 3026-3030) and the stat + S.IFREG validation (lines 3035-3043) properly address the buffer overflow and file type concerns raised in previous reviews. This ensures only valid regular files are returned.
Based on learnings
test/js/node/module/node-module-findPackageJSON.test.ts (2)
1-44: Excellent test structure and coverage!The tests properly use
describe.concurrentandtest.concurrentfor parallel execution, employ portable assertions withpath.basename()andpath.isAbsolute()instead of brittle string contains checks, and explicitly asserttoBeNull()for the not-found case. All previous review comments have been addressed.
56-61: Good coverage of absolute path string input!This test properly verifies that the API accepts absolute path strings (not just file:// URLs), addressing the previous review feedback about covering this use case.
| describe.concurrent("Module.findPackageJSON", () => { | ||
| test.concurrent("finds package.json from file URL", () => { | ||
| const fileUrl = pathToFileURL(__filename).href; | ||
| const result = findPackageJSON(fileUrl); | ||
|
|
||
| expect(typeof result).toBe("string"); | ||
| expect(path.basename(result!)).toBe("package.json"); | ||
| expect(path.isAbsolute(result!)).toBe(true); | ||
| }); | ||
|
|
||
| test.concurrent("finds package.json from directory path", () => { | ||
| const dirUrl = pathToFileURL(import.meta.dir).href; | ||
| const result = findPackageJSON(dirUrl); | ||
|
|
||
| expect(typeof result).toBe("string"); | ||
| expect(path.basename(result!)).toBe("package.json"); | ||
| expect(path.isAbsolute(result!)).toBe(true); | ||
| }); | ||
|
|
||
| test.concurrent("finds package.json from nested file", () => { | ||
| const nestedPath = path.join(import.meta.dir, "../../.."); | ||
| const fileUrl = pathToFileURL(path.join(nestedPath, "some-file.js")).href; | ||
| const result = findPackageJSON(fileUrl); | ||
|
|
||
| expect(typeof result).toBe("string"); | ||
| expect(path.basename(result!)).toBe("package.json"); | ||
| expect(path.isAbsolute(result!)).toBe(true); | ||
| }); | ||
|
|
||
| test.concurrent("returns null when no package.json found", () => { | ||
| // Use a path that's unlikely to have a package.json | ||
| const rootPath = path.parse(import.meta.dir).root; | ||
| const deepPath = path.join(rootPath, "nonexistent", "deep", "path", "file.js"); | ||
| const fileUrl = pathToFileURL(deepPath).href; | ||
| const result = findPackageJSON(fileUrl); | ||
|
|
||
| // Should return null when not found | ||
| expect(result).toBeNull(); | ||
| }); | ||
|
|
||
| test.concurrent("works with absolute paths as file URLs", () => { | ||
| const absolutePath = path.resolve(import.meta.dir, "node-module-findPackageJSON.test.ts"); | ||
| const fileUrl = pathToFileURL(absolutePath).href; | ||
| const result = findPackageJSON(fileUrl); | ||
|
|
||
| expect(typeof result).toBe("string"); | ||
| expect(path.basename(result!)).toBe("package.json"); | ||
| expect(path.isAbsolute(result!)).toBe(true); | ||
| }); | ||
|
|
||
| test.concurrent("accepts absolute path string", () => { | ||
| const result = findPackageJSON(import.meta.dir); | ||
| expect(typeof result).toBe("string"); | ||
| expect(path.basename(result!)).toBe("package.json"); | ||
| expect(path.isAbsolute(result!)).toBe(true); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding edge case tests (optional).
The current test suite provides solid coverage of typical use cases. To further improve robustness, consider adding tests for:
- Relative path inputs (e.g.,
"./some/file.js") to verify they're handled appropriately - Paths with symbolic links to ensure resolution works correctly
- Invalid inputs (empty string, malformed URLs) to verify error handling
These are optional enhancements and not blockers for this PR.
🤖 Prompt for AI Agents
In test/js/node/module/node-module-findPackageJSON.test.ts around lines 6 to 62,
the test suite lacks coverage for several edge cases; add new concurrent tests
that (1) pass a relative path string (e.g., "./some/file.js") and assert correct
package.json resolution or null, (2) simulate a path with a symbolic link
(create a temp dir, create a symlink to it, call findPackageJSON on a file path
through the symlink and assert resolution), and (3) verify invalid inputs (empty
string and malformed URL) return null or throw a well-defined error per function
contract; keep tests isolated (use temp dirs and cleanup) and assert expected
types/values consistent with existing tests.
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Thank you for this
Can we go through the module resolution cache in resolver.zig instead of iterating through the directories? As-is, this will be significantly slower than what Node.js does when we have an opportunity to make it significantly faster and with fewer code changes overall.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/bun.js/modules/NodeModuleModule.cpp(3 hunks)src/bun.js/node/path.zig(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{cpp,h}
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.{cpp,h}: When exposing a JS class with public Constructor and Prototype, define three C++ types: class Foo : public JSC::DestructibleObject (if it has C++ fields), class FooPrototype : public JSC::JSNonFinalObject, and class FooConstructor : public JSC::InternalFunction
If the class has C++ data members, inherit from JSC::DestructibleObject and provide proper destruction; if it has no C++ fields (only JS properties), avoid a class and use JSC::constructEmptyObject(vm, structure) with putDirectOffset
Prefer placing the subspaceFor implementation in the .cpp file rather than the header when possible
Files:
src/bun.js/modules/NodeModuleModule.cpp
**/*.cpp
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.cpp: Include "root.h" at the top of C++ binding files to satisfy lints
Define prototype properties using a const HashTableValue array and declare accessors/functions with JSC_DECLARE_* macros
Prototype classes should subclass JSC::JSNonFinalObject, provide create/createStructure, DECLARE_INFO, finishCreation that reifies static properties, and set mayBePrototype on the Structure
Custom getters should use JSC_DEFINE_CUSTOM_GETTER, jsDynamicCast to validate this, and throwThisTypeError on mismatch
Custom setters should use JSC_DEFINE_CUSTOM_SETTER, validate this via jsDynamicCast, and store via WriteBarrier/set semantics
Prototype functions should use JSC_DEFINE_HOST_FUNCTION, validate this with jsDynamicCast, and return encoded JSValue
Constructors should subclass JSC::InternalFunction, return internalFunctionSpace in subspaceFor, set the prototype property as non-configurable/non-writable, and provide create/createStructure
Provide a setup function that builds the Prototype, Constructor, and Structure, and assigns them to the LazyClassStructure initializer
Use the cached Structure via globalObject->m_.get(globalObject) when constructing instances
Expose constructors to Zig via an extern "C" function that returns the constructor from the LazyClassStructure
Provide an extern "C" Bun____toJS function that creates an instance using the cached Structure and returns an EncodedJSValue
Files:
src/bun.js/modules/NodeModuleModule.cpp
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/bun.js/node/path.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/node/path.zig
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
When adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
src/**/*.zig: Use Zig private fields with the # prefix for encapsulation (e.g., struct { #foo: u32 })
Prefer Decl literals for initialization (e.g., const decl: Decl = .{ .binding = 0, .value = 0 };)
Place @import statements at the bottom of the file (formatter will handle ordering)
src/**/*.zig: In Zig code, manage memory carefully: use appropriate allocators and defer for cleanup
Cache JavaScriptCore class structures in ZigGlobalObject when adding new classes
Files:
src/bun.js/node/path.zig
🧬 Code graph analysis (1)
src/bun.js/modules/NodeModuleModule.cpp (2)
src/bun.js/bindings/ErrorCode.cpp (14)
throwError(1690-1694)throwError(1690-1690)INVALID_ARG_VALUE(846-864)INVALID_ARG_VALUE(846-846)INVALID_ARG_VALUE(887-904)INVALID_ARG_VALUE(887-887)INVALID_ARG_VALUE(907-943)INVALID_ARG_VALUE(907-907)INVALID_ARG_VALUE(945-972)INVALID_ARG_VALUE(945-945)INVALID_ARG_VALUE(974-999)INVALID_ARG_VALUE(974-974)INVALID_ARG_VALUE(1001-1022)INVALID_ARG_VALUE(1001-1001)src/bun.js/bindings/BunString.cpp (10)
toString(178-181)toString(178-178)toString(208-211)toString(208-208)toString(230-236)toString(230-230)toString(237-243)toString(237-237)toString(244-250)toString(244-244)
🔇 Additional comments (6)
src/bun.js/node/path.zig (2)
2981-3015: File vs directory detection looks correct.The logic properly handles:
- Regular file detection using
S.IFREGcheck- Non-existent paths with a separator heuristic
- Buffer length validation before the stat call
Past review concerns about buffer overflow and file type validation have been addressed.
3034-3044: Parent directory walk logic is correct.The loop properly:
- Uses platform-specific
dirnamefunctions to get the parent- Terminates when reaching the root (when
parent == search_dir)- Handles both Windows and POSIX paths correctly
src/bun.js/modules/NodeModuleModule.cpp (4)
36-36: Function declarations follow established patterns.The host function declaration and external C function signature are consistent with other functions in this module (e.g.,
jsFunctionNodeModuleCreateRequireand its corresponding external function).Also applies to: 292-292
301-327: Argument validation and URL handling are correct.The implementation properly:
- Validates argument count with appropriate error code
- Converts file:// URLs to filesystem paths
- Rejects invalid URLs and non-file protocols with clear error messages
- Follows the same pattern as
jsFunctionNodeModuleCreateRequire(lines 241-266)
329-344: Result handling is correct and follows established patterns.The implementation properly:
- Creates a
BunStringfrom the input path- Checks for empty result and returns
jsNull()appropriately- Handles WTF string memory management with the refCount check and deref (matching the pattern at lines 280-284)
- Returns a JS string for successful lookups
892-892: LUT entry is correctly formatted.The
findPackageJSONentry follows the same pattern as other function entries in the table (e.g., line 890createRequire) with the correct arity of 1.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📥 Commits
Reviewing files that changed from the base of the PR and between 2dc3e8e and fb35fe5eb5112e464f52ceb7427a241eb13c2e42.
📒 Files selected for processing (1)
src/bun.js/node/path.zig(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/javascriptcore-class.mdc)
**/*.zig: Declare the extern C symbol in Zig and export a Zig-friendly alias for use
Wrap the Bun____toJS extern in a Zig method that takes a JSGlobalObject and returns JSC.JSValue
Files:
src/bun.js/node/path.zig
src/bun.js/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
src/bun.js/**/*.zig: In Zig binding structs, expose generated bindings via pub const js = JSC.Codegen.JS and re-export toJS/fromJS/fromJSDirect
Constructors and prototype methods should return bun.JSError!JSC.JSValue to integrate Zig error handling with JS exceptions
Use parameter name globalObject (not ctx) and accept (*JSC.JSGlobalObject, *JSC.CallFrame) in binding methods/constructors
Implement getters as get(this, globalObject) returning JSC.JSValue and matching the .classes.ts interface
Provide deinit() for resource cleanup and finalize() that calls deinit(); use bun.destroy(this) or appropriate destroy pattern
Access JS call data via CallFrame (argument(i), argumentCount(), thisValue()) and throw errors with globalObject.throw(...)
For properties marked cache: true, use the generated Zig accessors (NameSetCached/GetCached) to work with GC-owned values
In finalize() for objects holding JS references, release them using .deref() before destroy
Files:
src/bun.js/node/path.zig
src/**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
When adding debug logs in Zig, create a scoped logger and log via Bun APIs:
const log = bun.Output.scoped(.${SCOPE}, .hidden);thenlog("...", .{})
src/**/*.zig: Use Zig private fields with the # prefix for encapsulation (e.g., struct { #foo: u32 })
Prefer Decl literals for initialization (e.g., const decl: Decl = .{ .binding = 0, .value = 0 };)
Place @import statements at the bottom of the file (formatter will handle ordering)
src/**/*.zig: In Zig code, manage memory carefully: use appropriate allocators and defer for cleanup
Cache JavaScriptCore class structures in ZigGlobalObject when adding new classes
Files:
src/bun.js/node/path.zig
| export fn Bun__findPackageJSON(globalObject: *jsc.JSGlobalObject, input_path: *bun.String, result: *bun.String) void { | ||
| var slice = input_path.toUTF8(bun.default_allocator); | ||
| defer slice.deinit(); | ||
|
|
||
| var current_dir = slice.slice(); | ||
| if (current_dir.len == 0) { | ||
| result.* = bun.String.empty; | ||
| return; | ||
| } |
There was a problem hiding this comment.
Validate that the input path is absolute.
The function documentation and PR objectives state that findPackageJSON should accept "file URLs or absolute paths." However, there is no validation that input_path is absolute before processing. If a relative path is passed (e.g., "src/file.js"), the resolver's readDirInfo may not function correctly, leading to undefined behavior or incorrect results.
Consider adding validation after line 2983:
var current_dir = slice.slice();
if (current_dir.len == 0) {
result.* = bun.String.empty;
return;
}
+
+ // Validate that the input is an absolute path
+ if (!isAbsolutePosixT(u8, current_dir) and !isAbsoluteWindowsT(u8, current_dir)) {
+ result.* = bun.String.empty;
+ return;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export fn Bun__findPackageJSON(globalObject: *jsc.JSGlobalObject, input_path: *bun.String, result: *bun.String) void { | |
| var slice = input_path.toUTF8(bun.default_allocator); | |
| defer slice.deinit(); | |
| var current_dir = slice.slice(); | |
| if (current_dir.len == 0) { | |
| result.* = bun.String.empty; | |
| return; | |
| } | |
| export fn Bun__findPackageJSON(globalObject: *jsc.JSGlobalObject, input_path: *bun.String, result: *bun.String) void { | |
| var slice = input_path.toUTF8(bun.default_allocator); | |
| defer slice.deinit(); | |
| var current_dir = slice.slice(); | |
| if (current_dir.len == 0) { | |
| result.* = bun.String.empty; | |
| return; | |
| } | |
| // Validate that the input is an absolute path | |
| if (!isAbsolutePosixT(u8, current_dir) and !isAbsoluteWindowsT(u8, current_dir)) { | |
| result.* = bun.String.empty; | |
| return; | |
| } |
🤖 Prompt for AI Agents
In src/bun.js/node/path.zig around lines 2975 to 2983, the code does not
validate that input_path is an absolute path or a file URL before proceeding;
add a validation immediately after line 2983 that accepts either a "file://"
prefix or an absolute filesystem path (e.g., starts with '/' on POSIX or matches
a Windows absolute form like /^[A-Za-z]:\\|\/]/), and if the check fails set
result.* = bun.String.empty and return (or raise the same JS error/handling
convention used elsewhere in this function) so relative paths are rejected early
and the resolver is not invoked with invalid input.
| if (!isAbsolutePosixT(u8, pkg_path) and !isAbsoluteWindowsT(u8, pkg_path)) { | ||
| continue; // Skip if not absolute and keep searching | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a comment explaining the continue behavior (optional).
When a package.json is found in the resolver cache but its path is not absolute, the code continues searching upward. While this defensive approach was added per previous review feedback, it could silently mask bugs in the resolver or cache. Consider adding a comment explaining this behavior, or alternatively, add a debug log statement to help diagnose such issues during development.
// Verify it's absolute as expected
if (!isAbsolutePosixT(u8, pkg_path) and !isAbsoluteWindowsT(u8, pkg_path)) {
+ // Skip non-absolute paths from cache and continue searching
+ // This is defensive coding - ideally the resolver always returns absolute paths
continue; // Skip if not absolute and keep searching
}🤖 Prompt for AI Agents
In src/bun.js/node/path.zig around lines 3038 to 3040, the code continues when a
cached package.json path is not absolute which can silently hide resolver/cache
issues; update the code by adding a concise comment above the continue
explaining that this is a defensive check to skip non-absolute cached paths and
the rationale (to keep searching upward and avoid using malformed cache
entries), and optionally add a debug-level log statement that includes the
offending pkg_path and a short message indicating the skip so developers can
trace unexpected non-absolute entries during debugging.
0a72254 to
d91bd21
Compare
|
Closing as stale: this PR predates the Rust rewrite. Every If the underlying change is still wanted, it will need to be redone against the current Rust/C++ tree. Apologies for the churn, and thank you for the contribution. |
3 participants
Closes: #23898
What does this PR do?
NodeModuleModule.cpp): AddedjsFunctionFindPackageJSONthat accepts file URLs or absolute paths and returns the path to the nearest package.json file or null if not foundpath.zig): ImplementedBun__findPackageJSONthat searches upward through the directory tree with platform-aware path operations (Windows/POSIX)The function traverses up the directory tree from the given path until it finds a package.json file, returning its absolute path. Returns null when no package.json is found.
How did you verify your code works?
Screenshot
