fix: get npm client and install command correctly #383
Conversation
🦋 Changeset detectedLatest commit: f57d8a0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
WalkthroughA new utility for detecting the npm client and generating an appropriate install command was introduced. The rule for reporting extraneous dependencies now uses this utility to provide more accurate, client-specific install instructions. Supporting types and exports were added, and a changeset documents the update. Changes
Sequence Diagram(s)sequenceDiagram
participant Rule as no-extraneous-dependencies Rule
participant Utils as npm-client Utility
Rule->>Utils: getNpmInstallCommand(packageName)
Utils->>Utils: getNpmClient()
Utils-->>Rule: install command string (e.g., "yarn add packageName")
Rule-->>Developer: Error message with correct install command
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected. Suggested labels
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/rules/no-extraneous-dependencies.tsOops! Something went wrong! :( ESLint: 9.28.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/lib/index.js' imported from /eslint.config.js src/types.tsOops! Something went wrong! :( ESLint: 9.28.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/lib/index.js' imported from /eslint.config.js src/utils/npm-client.tsOops! Something went wrong! :( ESLint: 9.28.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/lib/index.js' imported from /eslint.config.js
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes how the npm client and install command are determined, ensuring that the proper command is suggested in error messages.
- Introduces a new npm-client module with getNpmClient and getNpmInstallCommand functions
- Updates exports and integration in the rules to utilize the new npm-client utility
- Adds a new SetValue type to support inferred values for npm clients
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/utils/npm-client.ts | Adds functions to determine the npm client and form the install command |
| src/utils/index.ts | Re-exports the npm-client module |
| src/types.ts | Adds a SetValue helper type |
| src/rules/no-extraneous-dependencies.ts | Integrates the dynamic install command in an error message |
| .changeset/ready-rules-attack.md | Updates changeset to reflect the fix |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
commit: |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 0dadf6f in 1 minute and 58 seconds. Click for details.
- Reviewed
90lines of code in5files - Skipped
0files when reviewing. - Skipped posting
0draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
Workflow ID: wflow_hgMdUWLikYofJRIz
You can customize 
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #383 +/- ##
=======================================
Coverage 96.02% 96.03%
=======================================
Files 94 95 +1
Lines 4907 4917 +10
Branches 1842 1826 -16
=======================================
+ Hits 4712 4722 +10
Misses 194 194
Partials 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
src/utils/npm-client.ts (1)
27-29:⚠️ Potential issue
getNpmInstallCommandrelies on a global that is (still)undefinedBecause the cache is never populated, the ternaries always take the non-npm / non-deno branches, emitting
addand omitting thenpm:prefix. Capture the client locally instead of peeking at the mutable global:-export const getNpmInstallCommand = (packageName: string) => - `${getNpmClient()} ${npmClient === NPM ? 'i' : 'add'} ${npmClient === 'deno' ? `${NPM}:` : ''}${packageName}` +export const getNpmInstallCommand = (packageName: string) => { + const client = getNpmClient() + const verb = client === NPM ? 'i' : 'add' + const prefix = client === 'deno' ? `${NPM}:` : '' + return `${client} ${verb} ${prefix}${packageName}` +}src/rules/no-extraneous-dependencies.ts (1)
393-402: 🛠️ Refactor suggestionInstall command is frozen at module load & duplicated work is done for each import
Evaluating
getNpmInstallCommand('{{packageName}}')inside the message literal:
- Computes the client only once, so
eslint --fixunder a different client produces stale advice.- Bakes the literal
'{{packageName}}'into the command, resulting innpm add {{packageName}}even for npm (see bug above).Delegate the formatting to runtime via an extra placeholder:
- missing: `'{{packageName}}' should be listed in the project's dependencies. Run '${getNpmInstallCommand('{{packageName}}')}' to add it`, + missing: `'{{packageName}}' should be listed in the project's dependencies. Run '{{installCommand}}' to add it`,and when reporting:
context.report({ node, messageId: 'missing', data: { packageName, + installCommand: getNpmInstallCommand(packageName), }, })This keeps advice accurate for every package and every execution context.
🧹 Nitpick comments (1)
.changeset/ready-rules-attack.md (1)
1-6: Nit: give the changeset a human-readable summary lineConventional-changelog tools surface only the first line; consider:
fix: detect npm client correctly and show proper install command
🛑 Comments failed to post (1)
src/utils/npm-client.ts (1)
17-25:
⚠️ Potential issue
getNpmClientnever populates the cache – every call re-parses the env var
npmClientis leftundefined, so subsequent calls pay the parsing cost again and the rest of the module (e.g.getNpmInstallCommand) receivesundefinedinstead of the detected client.-export const getNpmClient = (): NpmClient => { - if (npmClient) { - return npmClient - } - - const client = process.env.npm_config_user_agent?.split('/')[0] - - return client && NPM_CLIENTS.has(client) ? (client as NpmClient) : NPM -} +export const getNpmClient = (): NpmClient => { + if (npmClient) return npmClient + + const detected = + process.env.npm_config_user_agent?.split('/')[0] as NpmClient | undefined + + npmClient = + detected && NPM_CLIENTS.has(detected) ? detected : (NPM as NpmClient) + + return npmClient +}📝 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 const getNpmClient = (): NpmClient => { if (npmClient) return npmClient const detected = process.env.npm_config_user_agent?.split('/')[0] as NpmClient | undefined npmClient = detected && NPM_CLIENTS.has(detected) ? detected : (NPM as NpmClient) return npmClient }🤖 Prompt for AI Agents
In src/utils/npm-client.ts around lines 17 to 25, the function getNpmClient does not assign the detected client to the npmClient cache variable, causing repeated parsing of the environment variable and returning undefined to other parts of the module. Fix this by assigning the resolved client value to npmClient before returning it, ensuring the cache is populated and subsequent calls return the cached client without re-parsing.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 4276e6f in 1 minute and 25 seconds. Click for details.
- Reviewed
92lines of code in5files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/rules/no-extraneous-dependencies.ts:397
- Draft comment:
Using getNpmInstallCommand to dynamically generate the install command is neat. Consider caching the result of getNpmClient() into a local variable inside getNpmInstallCommand (or here) to ensure consistency rather than relying on the global 'npmClient' later in the template. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. src/types.ts:160
- Draft comment:
The new SetValue utility type is clear and follows common TypeScript patterns. Good addition for extracting the value type of a Set. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. src/utils/npm-client.ts:29
- Draft comment:
In getNpmInstallCommand, the function calls getNpmClient() and then uses the global 'npmClient' in subsequent checks. For clarity and to avoid any future pitfalls, consider storing the result of getNpmClient() in a local variable and using that consistently (e.g., const client = getNpmClient(); then use client in place of global npmClient). - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
4. src/utils/npm-client.ts:22
- Draft comment:
The parsing of process.env.npm_config_user_agent is straightforward. It might be helpful to add a brief comment describing the expected format of this environment variable for future maintainers. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_YbeN3J9U6BA0QHu6
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed f57d8a0 in 48 seconds. Click for details.
- Reviewed
45lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. test/utils/npm-client.spec.ts:17
- Draft comment:
Consider adding tests for additional package managers (e.g., pnpm, bun) to ensure full coverage as mentioned in the changelog. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_iyT1eZaLa6O5ybRK
You can customize
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/utils/npm-client.spec.ts (2)
10-10: Prefer assignment overdeletefor env varsThe linter (Biome
noDelete) is right here: deleting a property fromprocess.envis slower than re-assigning.
In this case a simpleprocess.env.npm_config_user_agent = undefinedsuffices and keeps the key available while signalling “no value”.🧰 Tools
🪛 Biome (1.9.4)
[error] 10-10: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
22-26: Consider adding coverage for all supported clients (pnpm, bun, etc.)
npm-client.tsrecognises more thannpm,yarn, anddeno.
Adding assertions forpnpm,bun, and an uppercase variant (e.g.YARN/4.0.0) would harden regression protection with minimal cost.Also applies to: 34-38
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.changeset/ready-rules-attack.md(1 hunks)src/rules/no-extraneous-dependencies.ts(2 hunks)src/types.ts(1 hunks)src/utils/index.ts(1 hunks)src/utils/npm-client.ts(1 hunks)test/utils/npm-client.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- .changeset/ready-rules-attack.md
- src/rules/no-extraneous-dependencies.ts
- src/types.ts
- src/utils/index.ts
- src/utils/npm-client.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
test/utils/npm-client.spec.ts (1)
src/utils/npm-client.ts (3)
getNpmClient(17-27)NpmClient(13-13)getNpmInstallCommand(29-30)
🪛 Biome (1.9.4)
test/utils/npm-client.spec.ts
[error] 10-10: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (21)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on windows-latest
- GitHub Check: Lint and Test with Node.js lts/* and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js lts/* and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 24 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8.56 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 8 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 8 on macos-latest
- GitHub Check: Lint and Test with Node.js 22 and ESLint 9 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 and ESLint 9 on macos-latest
- GitHub Check: Lint and Test with Node.js 20 and ESLint 8.56 on macos-latest
1 participant
close #78
Important
Enhance error messages with correct npm install commands by detecting npm clients and updating
no-extraneous-dependencies.ts.no-extraneous-dependencies.tsnow usegetNpmInstallCommand()to generate the correct install command based on the detected npm client.getNpmClient()andgetNpmInstallCommand()innpm-client.tsto detect npm clients (npm, yarn, pnpm, bun, deno) and generate install commands.SetValuetype intypes.tsfor extracting value types from aSet.npm-client.spec.tsto testgetNpmClient()andgetNpmInstallCommand()for different npm clients.This description was created by
for f57d8a0. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit