fix: detect package manager for collection install suggestion#476
Open
fix: detect package manager for collection install suggestion#476
Conversation
The missing icon collection warning always suggested `npm i -D`, regardless of the project's actual package manager. This detects the package manager by checking lockfiles at build time and passes the correct install command to the runtime via runtimeConfig. Now suggests: - `pnpm add -D` when pnpm-lock.yaml exists - `yarn add -D` when yarn.lock exists - `bun add -D` when bun.lockb or bun.lock exists - `npm i -D` as fallback Fixes nuxt#473
📝 WalkthroughWalkthroughThe PR adds package-manager detection to the Nuxt Icon module. A new helper Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 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/module.ts`:
- Around line 234-245: The detectInstallCommand function only checks lockfiles
in the provided rootDir and misses parent workspace/monorepo layouts; update
detectInstallCommand to walk up parent directories from rootDir to filesystem
root (or stop at a workspace boundary) and check for known indicators in each
directory—pnpm-lock.yaml or pnpm-workspace.yaml => 'pnpm add -D', yarn.lock or
yarn workspaces indicators or package.json with "workspaces" => 'yarn add -D',
bun.lockb or bun.lock => 'bun add -D', and fallback to package.json's
"packageManager" field if present before finally returning 'npm i -D'; reference
detectInstallCommand and the helper pathResolve/existsSync utilities when adding
the upward directory traversal and checks.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 55c2d7ef-22ae-498f-ade8-80b10be71616
📒 Files selected for processing (2)
src/module.tssrc/runtime/server/api.ts
Comment on lines
+234
to
+245
| function detectInstallCommand(rootDir: string): string { | ||
| if (existsSync(pathResolve(rootDir, 'pnpm-lock.yaml'))) { | ||
| return 'pnpm add -D' | ||
| } | ||
| if (existsSync(pathResolve(rootDir, 'yarn.lock'))) { | ||
| return 'yarn add -D' | ||
| } | ||
| if (existsSync(pathResolve(rootDir, 'bun.lockb')) || existsSync(pathResolve(rootDir, 'bun.lock'))) { | ||
| return 'bun add -D' | ||
| } | ||
| return 'npm i -D' | ||
| } |
There was a problem hiding this comment.
Package manager detection misses common monorepo layouts.
This only checks one directory. If the lockfile lives in a parent workspace directory, the warning still falls back to npm i -D, so detection is incorrect for those projects.
Suggested fix
function detectInstallCommand(rootDir: string): string {
- if (existsSync(pathResolve(rootDir, 'pnpm-lock.yaml'))) {
- return 'pnpm add -D'
- }
- if (existsSync(pathResolve(rootDir, 'yarn.lock'))) {
- return 'yarn add -D'
- }
- if (existsSync(pathResolve(rootDir, 'bun.lockb')) || existsSync(pathResolve(rootDir, 'bun.lock'))) {
- return 'bun add -D'
- }
+ const lockfileCommandPairs: Array<[string, string]> = [
+ ['pnpm-lock.yaml', 'pnpm add -D'],
+ ['yarn.lock', 'yarn add -D'],
+ ['bun.lockb', 'bun add -D'],
+ ['bun.lock', 'bun add -D'],
+ ]
+
+ let currentDir = rootDir
+ while (true) {
+ for (const [lockfile, command] of lockfileCommandPairs) {
+ if (existsSync(pathResolve(currentDir, lockfile))) {
+ return command
+ }
+ }
+
+ const parentDir = pathResolve(currentDir, '..')
+ if (parentDir === currentDir) {
+ break
+ }
+ currentDir = parentDir
+ }
+
return 'npm i -D'
}📝 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.
Suggested change
| function detectInstallCommand(rootDir: string): string { | |
| if (existsSync(pathResolve(rootDir, 'pnpm-lock.yaml'))) { | |
| return 'pnpm add -D' | |
| } | |
| if (existsSync(pathResolve(rootDir, 'yarn.lock'))) { | |
| return 'yarn add -D' | |
| } | |
| if (existsSync(pathResolve(rootDir, 'bun.lockb')) || existsSync(pathResolve(rootDir, 'bun.lock'))) { | |
| return 'bun add -D' | |
| } | |
| return 'npm i -D' | |
| } | |
| function detectInstallCommand(rootDir: string): string { | |
| const lockfileCommandPairs: Array<[string, string]> = [ | |
| ['pnpm-lock.yaml', 'pnpm add -D'], | |
| ['yarn.lock', 'yarn add -D'], | |
| ['bun.lockb', 'bun add -D'], | |
| ['bun.lock', 'bun add -D'], | |
| ] | |
| let currentDir = rootDir | |
| while (true) { | |
| for (const [lockfile, command] of lockfileCommandPairs) { | |
| if (existsSync(pathResolve(currentDir, lockfile))) { | |
| return command | |
| } | |
| } | |
| const parentDir = pathResolve(currentDir, '..') | |
| if (parentDir === currentDir) { | |
| break | |
| } | |
| currentDir = parentDir | |
| } | |
| return 'npm i -D' | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/module.ts` around lines 234 - 245, The detectInstallCommand function only
checks lockfiles in the provided rootDir and misses parent workspace/monorepo
layouts; update detectInstallCommand to walk up parent directories from rootDir
to filesystem root (or stop at a workspace boundary) and check for known
indicators in each directory—pnpm-lock.yaml or pnpm-workspace.yaml => 'pnpm add
-D', yarn.lock or yarn workspaces indicators or package.json with "workspaces"
=> 'yarn add -D', bun.lockb or bun.lock => 'bun add -D', and fallback to
package.json's "packageManager" field if present before finally returning 'npm i
-D'; reference detectInstallCommand and the helper pathResolve/existsSync
utilities when adding the upward directory traversal and checks.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/module.ts (1)
241-252:⚠️ Potential issue | 🟠 MajorWorkspace lockfiles in parent dirs are still missed.
On Line 241, detection is limited to
rootDir, so monorepo/workspace apps with lockfiles in parent directories will still get wrong suggestions (npm i -Dfallback).Suggested adjustment
function detectInstallCommand(rootDir: string): string { - if (existsSync(pathResolve(rootDir, 'pnpm-lock.yaml'))) { - return 'pnpm add -D' - } - if (existsSync(pathResolve(rootDir, 'yarn.lock'))) { - return 'yarn add -D' - } - if (existsSync(pathResolve(rootDir, 'bun.lockb')) || existsSync(pathResolve(rootDir, 'bun.lock'))) { - return 'bun add -D' - } - return 'npm i -D' + const candidates: Array<[string, string]> = [ + ['pnpm-lock.yaml', 'pnpm add -D'], + ['yarn.lock', 'yarn add -D'], + ['bun.lockb', 'bun add -D'], + ['bun.lock', 'bun add -D'], + ] + + let currentDir = rootDir + while (true) { + for (const [lockfile, command] of candidates) { + if (existsSync(pathResolve(currentDir, lockfile))) { + return command + } + } + + const parentDir = pathResolve(currentDir, '..') + if (parentDir === currentDir) { + break + } + currentDir = parentDir + } + + return 'npm i -D' }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/module.ts` around lines 241 - 252, detectInstallCommand currently checks only the provided rootDir; update it to walk up parent directories to the filesystem root and check each directory for lockfiles (pnpm-lock.yaml, yarn.lock, bun.lockb or bun.lock) using the existing existsSync and pathResolve helpers. Return 'pnpm add -D', 'yarn add -D', or 'bun add -D' as soon as a matching lockfile is found in any ancestor directory (use path.dirname to iterate parents and stop when the directory no longer changes), otherwise fall back to 'npm i -D'. Keep function name detectInstallCommand and the same lockfile checks but perform them for each ancestor directory.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/module.ts`:
- Around line 241-252: detectInstallCommand currently checks only the provided
rootDir; update it to walk up parent directories to the filesystem root and
check each directory for lockfiles (pnpm-lock.yaml, yarn.lock, bun.lockb or
bun.lock) using the existing existsSync and pathResolve helpers. Return 'pnpm
add -D', 'yarn add -D', or 'bun add -D' as soon as a matching lockfile is found
in any ancestor directory (use path.dirname to iterate parents and stop when the
directory no longer changes), otherwise fall back to 'npm i -D'. Keep function
name detectInstallCommand and the same lockfile checks but perform them for each
ancestor directory.
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.
Summary
Fixes #473
The dev warning for missing icon collections always suggests
npm i -D @iconify-json/<collection>, regardless of the project's actual package manager.Fix
Detects the package manager at build time by checking lockfiles in the project root, then passes the install command to the server runtime via
runtimeConfig.pnpm-lock.yamlpnpm add -Dyarn.lockyarn add -Dbun.lockb/bun.lockbun add -Dnpm i -DBefore
After (in a pnpm project)