-
Notifications
You must be signed in to change notification settings - Fork 52
Support node_modules created with bun install --linker=isolated
#136
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
Conversation
|
WalkthroughUpdated getProjectRootDirectoryFromNodeModules in simple-git-hooks.js to recognize Bun’s .bun installation layout and return the correct project root. Added a unit test in simple-git-hooks.test.js to validate Bun path handling alongside existing pnpm/deno cases. No changes to exported/public declarations. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Hook/Caller
participant SGH as simple-git-hooks
participant FS as Filesystem Path
Caller->>SGH: getProjectRootDirectoryFromNodeModules(path)
SGH->>FS: Inspect path segments
alt Bun layout detected (.bun)
SGH-->>Caller: Return path up to .bun parent
else pnpm/deno/other layouts
SGH-->>Caller: Apply existing rules (.pnpm/.deno/.store/etc.)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
simple-git-hooks.test.js (1)
76-82: Add a companion Bun test (without trailing/node_modules/simple-git-hooks) for parity with pnpmYou already cover the full Bun path; consider also testing just
.../node_modules/.bun/simple-git-hooks@<version>like we do for pnpm. This guards against regressions and asserts the function works when given the package-dir path itself.Apply this diff to add the extra test:
it("return correct dir when installed using bun with --linker=isolated", () => { expect( simpleGitHooks.getProjectRootDirectoryFromNodeModules( `var/my-project/node_modules/.bun/simple-git-hooks@${packageVersion}/node_modules/simple-git-hooks` ) ).toBe('var/my-project'); }) + + it("return correct dir when installed using bun with --linker=isolated (package dir only)", () => { + expect( + simpleGitHooks.getProjectRootDirectoryFromNodeModules( + `var/my-project/node_modules/.bun/simple-git-hooks@${packageVersion}` + ) + ).toBe("var/my-project"); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
simple-git-hooks.js(1 hunks)simple-git-hooks.test.js(1 hunks)
🔇 Additional comments (1)
simple-git-hooks.js (1)
117-120: Bun isolated linker support: logic and placement look correctThe
.bundetection mirrors existing.pnpm/.denohandling and is correctly placed before the.storecheck. The slice up toindexOfBunDir - 1resolves the proper project root for paths like.../node_modules/.bun/<pkg>@<ver>/node_modules/<pkg>. LGTM.
|
I need this!! |
toplenboren
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
|
hello @toplenboren could you publish this change? |
Bun v1.2.19 added support for isolated installs similar to pnpm. This install strategy puts packages in
./node_modules/.bun. For example,[email protected]would exist in./node_modules/.bun/[email protected]/node_modules/simple-git-hooksThis pr adds detection for
.buningetProjectRootDirectoryFromNodeModules(oven-sh/bun#21754)