fix: use detected package manager in install error/hint messages#187
Conversation
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
packages/vinext/src/cli.ts
Outdated
| console.log( | ||
| " No linter found. Install eslint or oxlint:\n\n" + | ||
| " npm install -D eslint eslint-config-next\n" + | ||
| " " + detectPackageManagerName(process.cwd()) + " add -D eslint eslint-config-next\n" + |
There was a problem hiding this comment.
Bug: detectPackageManagerName() returns the bare name ("npm", "yarn", etc.), and you append " add -D". But npm's subcommand is install, not add. For npm users this would produce:
npm add -D eslint eslint-config-next
While npm add technically works (it's an undocumented alias for npm install), it's non-standard and inconsistent with how detectPackageManager() returns "npm install -D" for the other call sites.
Use detectPackageManager() instead (same as check.ts and index.ts), which already returns the correct install command with flags:
| " " + detectPackageManagerName(process.cwd()) + " add -D eslint eslint-config-next\n" + | |
| " " + detectPackageManager(process.cwd()) + " eslint eslint-config-next\n" + |
packages/vinext/src/cli.ts
Outdated
| " " + detectPackageManagerName(process.cwd()) + " add -D eslint eslint-config-next\n" + | ||
| " # or\n" + | ||
| " npm install -D oxlint\n", | ||
| " " + detectPackageManagerName(process.cwd()) + " add -D oxlint\n", |
There was a problem hiding this comment.
Same issue — use detectPackageManager() for consistency:
| " " + detectPackageManagerName(process.cwd()) + " add -D oxlint\n", | |
| " " + detectPackageManager(process.cwd()) + " oxlint\n", |
packages/vinext/src/cli.ts
Outdated
| import { pathToFileURL } from "node:url"; | ||
| import { createRequire } from "node:module"; | ||
| import { execFileSync } from "node:child_process"; | ||
| import { detectPackageManagerName } from "./utils/project.js"; |
There was a problem hiding this comment.
If you switch to detectPackageManager per the suggestions below, update this import accordingly:
| import { detectPackageManagerName } from "./utils/project.js"; | |
| import { detectPackageManager } from "./utils/project.js"; |
|
Review submitted. Here's a summary: Requested changes on PR #187 The PR correctly identifies that hardcoded Bug in |
Error messages and install hints in index.ts, check.ts, and cli.ts were hardcoded to use `npm install`. This PR uses the existing `detectPackageManager()` / `detectPackageManagerName()` utilities (already used correctly in deploy.ts) so that Yarn, pnpm, and Bun users see the right install command. Before (on a Yarn project): Run: npm install -D @vitejs/plugin-rsc After: Run: yarn add -D @vitejs/plugin-rsc Fixes cloudflare#186
Replace detectPackageManagerName() + manual 'add -D' with detectPackageManager() which already returns the correct install command. This avoids producing 'npm add -D' (undocumented alias) for npm users and stays consistent with check.ts and index.ts.
southpolesteve
force-pushed
the
fix/use-detected-package-manager-in-error-messages
branch
from
2bd76d3 to
84e4670
Compare
commit: |
2 participants
Problem
Error messages and install hints in
index.ts,check.ts, andcli.tswere hardcoded to usenpm install, even when the project uses Yarn, pnpm, or Bun.For example, on a Yarn workspace project:
Solution
Use the existing
detectPackageManager()anddetectPackageManagerName()utilities fromutils/project.ts— which already correctly detect the package manager from lockfiles and are already used indeploy.ts.After this fix (on a Yarn project):
Changes
src/index.ts— 3 error/warning messages:@vitejs/plugin-rscnot installed (×2),@mdx-js/rollupnot installedsrc/check.ts— install hint shown to users duringvinext checksrc/cli.ts— eslint/oxlint install suggestionsRelated
Closes #186
Relates to #109