fix: command for create-* packages in Deno#2357
fix: command for create-* packages in Deno#2357knotbin wants to merge 2 commits intonpmx-dev:mainfrom
create-* packages in Deno#2357Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
Hello! Thank you for opening your first PR to npmx, @knotbin! 🚀 Here’s what will happen next:
|
knotbin
changed the title
create-* packages in Deno
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughThe Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 (1)
test/unit/app/utils/install-command.spec.ts (1)
397-417: Consider adding test coverage for Deno scoped create packages.The scoped create package tests only cover npm. Since the Deno implementation now includes the
npm:prefix handling (lines 140-142 in install-command.ts), it would be beneficial to verify this works correctly for scoped packages like@vue/create-app→['deno', 'create', 'npm:@vue/app'].🧪 Suggested test additions
it('handles `@scope/create` pattern', () => { expect( getExecuteCommandParts({ packageName: '@angular/create', packageManager: 'npm', isCreatePackage: true, }), ).toEqual(['npm', 'create', '@angular']) }) + + it('handles `@scope/create-app` pattern for deno with npm: prefix', () => { + expect( + getExecuteCommandParts({ + packageName: '@vue/create-app', + packageManager: 'deno', + isCreatePackage: true, + }), + ).toEqual(['deno', 'create', 'npm:`@vue/app`']) + }) })
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e552c239-b5fb-47f2-941a-926cabf91abb
📒 Files selected for processing (2)
app/utils/install-command.tstest/unit/app/utils/install-command.spec.ts
| if (options.packageManager === 'deno') { | ||
| return ['deno', 'create', `npm:${shortName}`] | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how Install.vue handles create commands for Deno
rg -n -A 15 'getCreatePartsForPM' --type vueRepository: npmx-dev/npmx.dev
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Search for all usages of pm.create to see if there are other places needing updates
rg -n 'pm\.create' --type ts --type vueRepository: npmx-dev/npmx.dev
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Locate the files mentioned in the review
fd -e ts -e vue install-command
fd -e vue InstallRepository: npmx-dev/npmx.dev
Length of output: 261
🏁 Script executed:
#!/bin/bash
# Read the install-command.ts file around lines 140-142
cat app/utils/install-command.ts | head -n 150 | tail -n 20Repository: npmx-dev/npmx.dev
Length of output: 814
🏁 Script executed:
#!/bin/bash
# Search for the Install.vue file and read it
find . -name "Install.vue" -type f 2>/dev/null | head -5Repository: npmx-dev/npmx.dev
Length of output: 98
🏁 Script executed:
#!/bin/bash
# Read the Install.vue file to check the getCreatePartsForPM function around lines 61-79
cat -n app/components/Terminal/Install.vue | sed -n '50,100p'Repository: npmx-dev/npmx.dev
Length of output: 2074
🏁 Script executed:
#!/bin/bash
# Search for usages of getCreatePartsForPM and getExecuteCommandParts
rg -n 'getCreatePartsForPM|getExecuteCommandParts' app/components/Terminal/Install.vue app/utils/install-command.tsRepository: npmx-dev/npmx.dev
Length of output: 555
🏁 Script executed:
#!/bin/bash
# Check if Install.vue imports anything from install-command.ts
rg -n 'import.*install-command|from.*install-command' app/components/Terminal/Install.vueRepository: npmx-dev/npmx.dev
Length of output: 126
Fix Deno create package handling inconsistency between install-command.ts and Install.vue.
The getExecuteCommandParts function in install-command.ts (lines 140–142) correctly generates ['deno', 'create', 'npm:${shortName}'] for Deno, but Install.vue's getCreatePartsForPM function (lines 61–79) duplicates this logic without the npm: prefix, producing ['deno', 'create', shortName] instead. This inconsistency mirrors the existing Deno handling in getTypesInstallPartsForPM (line 88), which correctly uses the npm: prefix.
Update getCreatePartsForPM in Install.vue to add Deno-specific handling:
if (pmId === 'deno') {
return ['deno', 'create', `npm:${shortName}`]
}
return [...pm.create.split(' '), shortName]
Alternatively, refactor Install.vue to use getExecuteCommandParts from install-command.ts to eliminate duplication.
ghostdevv
left a comment
ghostdevv
left a comment
There was a problem hiding this comment.
Could you take a look at the coderabbit suggestion?
|
@ghostdevv So sorry, I should have scrolled farther when I got the CodeRabbit review, I assumed the first comment was its full review. 😅 |
| if (options.packageManager === 'deno') { | ||
| return ['deno', 'create', `npm:${shortName}`] | ||
| } |
There was a problem hiding this comment.
Could we instead set the shortName to npm:? How are we handling the other cases where we need to prefix with npm: for deno?
no worries! |
2 participants
🔗 Linked issue
resolves #2337
🧭 Context
For example package
create-example, Current Deno commanddeno run exampleis incorrect and errors out.This PR fixes that and changes it to the correct
deno createcommand.📚 Description
This is a simple change that just changes the Deno command for create to the correct
deno create npm:examplecommand.