fix: link aliased dependencies to their real package#2214
fix: link aliased dependencies to their real package#2214thealxlabs wants to merge 4 commits intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
📝 WalkthroughWalkthroughA new utility function Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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
🧹 Nitpick comments (2)
test/unit/app/utils/npm/alias.spec.ts (1)
4-62: Please add one component-level regression around the rendered links.These assertions lock the parser down well, but the bug fixed by this PR lives in
Dependencies.vue. A small Vitest component test that renders an aliased dependency and checks the generatedtotarget would protect the actual navigation users click on.As per coding guidelines
**/*.{test,spec}.{ts,tsx}: Write unit tests for core functionality usingvitest.app/components/Package/Dependencies.vue (1)
118-123: Pull the alias route resolution out of the template.The same
parseNpmAlias(spec)?.name ?? .../parseNpmAlias(spec)?.range ?? ...branching is repeated across direct, peer, and optional dependency links. A tiny helper or precomputed row shape would keep the routing rules in one place, avoid reparsing the same spec several times per row, and make future link changes much harder to miss.♻️ One possible shape
+function getDependencyPackageName(depName: string, spec: string) { + return parseNpmAlias(spec)?.name ?? depName +} + +function getDependencyVersion(spec: string) { + const alias = parseNpmAlias(spec) + return alias ? (alias.range || undefined) : spec +}Then the bindings can call
packageRoute(getDependencyPackageName(dep, version), getDependencyVersion(version)).Also applies to: 177-182, 240-244, 252-257, 310-314, 318-323
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cfc0ff95-cbe8-45f2-900a-0b0a1ceeb0be
📒 Files selected for processing (3)
app/components/Package/Dependencies.vueapp/utils/npm/alias.tstest/unit/app/utils/npm/alias.spec.ts
| export function parseNpmAlias(version: string): { name: string; range: string } | null { | ||
| if (!version.startsWith('npm:')) return null | ||
| const spec = version.slice(4) // strip 'npm:' | ||
| // Handle scoped packages like @scope/pkg@1.0.0 — find @ after position 0 | ||
| const atIdx = spec.startsWith('@') ? spec.indexOf('@', 1) : spec.indexOf('@') | ||
| if (atIdx === -1) return { name: spec, range: '' } | ||
| return { name: spec.slice(0, atIdx), range: spec.slice(atIdx + 1) } |
There was a problem hiding this comment.
Return null for empty alias targets.
npm: currently parses to { name: '', range: '' }, which would send callers such as Dependencies.vue down the alias path with an empty package name instead of letting them fall back cleanly. Please treat blank specs as invalid and add a regression case for it.
🩹 Suggested guard
export function parseNpmAlias(version: string): { name: string; range: string } | null {
if (!version.startsWith('npm:')) return null
const spec = version.slice(4) // strip 'npm:'
+ if (!spec.trim()) return null
// Handle scoped packages like `@scope/pkg`@1.0.0 — find @ after position 0
const atIdx = spec.startsWith('@') ? spec.indexOf('@', 1) : spec.indexOf('@')
if (atIdx === -1) return { name: spec, range: '' }
return { name: spec.slice(0, atIdx), range: spec.slice(atIdx + 1) }
}📝 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 function parseNpmAlias(version: string): { name: string; range: string } | null { | |
| if (!version.startsWith('npm:')) return null | |
| const spec = version.slice(4) // strip 'npm:' | |
| // Handle scoped packages like @scope/pkg@1.0.0 — find @ after position 0 | |
| const atIdx = spec.startsWith('@') ? spec.indexOf('@', 1) : spec.indexOf('@') | |
| if (atIdx === -1) return { name: spec, range: '' } | |
| return { name: spec.slice(0, atIdx), range: spec.slice(atIdx + 1) } | |
| export function parseNpmAlias(version: string): { name: string; range: string } | null { | |
| if (!version.startsWith('npm:')) return null | |
| const spec = version.slice(4) // strip 'npm:' | |
| if (!spec.trim()) return null | |
| // Handle scoped packages like `@scope/pkg`@1.0.0 — find @ after position 0 | |
| const atIdx = spec.startsWith('@') ? spec.indexOf('@', 1) : spec.indexOf('@') | |
| if (atIdx === -1) return { name: spec, range: '' } | |
| return { name: spec.slice(0, atIdx), range: spec.slice(atIdx + 1) } | |
| } |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
1 participant
🔗 Linked issue
resolves #2010
🧭 Context
When a package uses npm alias syntax (e.g.
"my-react": "npm:react@^18.0.0"), the dependencies list linked to the alias namemy-reactrather than to the real packagereact. This made aliased dependencies unlinkable and gave a confusing user experience.📚 Description
parseNpmAlias(version)which parses thenpm:real-pkg@rangealias syntax, handling both regular and scoped packages.app/utils/npm/alias.tsfor testability and reuse.Package/Dependencies.vueto import from the utility and useparseNpmAlias(version)?.name ?? depwhen constructing package route links, so aliased dependencies route to the real package.Unit tests in
test/unit/app/utils/npm/alias.spec.tscover:null(not an alias)npm:real-pkg@^1.0.0→{ name: 'real-pkg', range: '^1.0.0' }npm:@scope/pkg@^1.0.0→{ name: '@scope/pkg', range: '^1.0.0' }npm:real-pkg(no version) →{ name: 'real-pkg', range: '' }