Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(upgrade): use nypm in upgrade and info commands #618

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Colonel-Sandvich
Copy link
Contributor

@Colonel-Sandvich Colonel-Sandvich commented Dec 27, 2024

πŸ”— Linked issue

resolves #617

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

This PR replaces old code with nymp's apis of detectPackageManager and addDependency.

As a bonus, we're now also supporting deno for free since nypm does

Unfortunately, nypm doesn't return which lock file is in use for bun so we still have to search for that ourselves.
@codecov-commenter
Copy link

codecov-commenter commented Dec 27, 2024

Codecov Report

Attention: Patch coverage is 0% with 47 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@a3bbac6). Learn more about missing BASE report.

Files with missing lines Patch % Lines
src/commands/upgrade.ts 0.00% 37 Missing ⚠️
src/utils/fs.ts 0.00% 6 Missing ⚠️
src/commands/info.ts 0.00% 4 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##             main    #618   +/-   ##
======================================
  Coverage        ?   1.31%           
======================================
  Files           ?      44           
  Lines           ?    2887           
  Branches        ?      44           
======================================
  Hits            ?      38           
  Misses          ?    2808           
  Partials        ?      41           

β˜” View full report in Codecov by Sentry.
πŸ“’ Have feedback on the report? Share it here.

@danielroe danielroe requested a review from Copilot December 27, 2024 07:05
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (3)

src/commands/upgrade.ts:205

  • [nitpick] The function name 'normaliseLockFile' should be renamed to 'normalizeLockFile' to follow American English spelling.
function normaliseLockFile(cwd: string, lockFiles: string | Array<string> | undefined) {

src/commands/info.ts:22

  • The non-null assertion operator (!) is unnecessary here. The value of 'name' is already checked to be truthy.
const npmName = name!.split('/').splice(0, 2).join('/')

src/commands/info.ts:9

  • The variables 'dependencies' and 'devDependencies' should be checked for nullish values to avoid potential issues.
getDepVersion = (name: string) => getPkg(name, cwd)?.version || dependencies[name] || devDependencies[name]


if (lockFile === undefined) {
consola.error(`Unable to find any lock files in ${cwd}`)
return undefined
Copy link
Preview

Copilot AI Dec 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning 'undefined' here can lead to unintended behavior when 'undefined' is added to the 'toRemove' array. Consider handling this case more gracefully.

Suggested change
return undefined
return ''

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
Further testing revealed that `utimes` throws an error if the file doesn't exist, this seemingly wasn't caught because package managers create the lockfile upon the install command
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fix(upgrade): use nypm to detect package managers
2 participants