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.

Sign up for GitHub

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

Jump to bottom

Fix Unsupported Error When Npm Version Not Detected #11430

Merged

kbukum1 merged 7 commits into main from kamil/fix-npm-unsupported-error

Jan 29, 2025

+19 −18

Contributor

kbukum1 commented Jan 29, 2025 •

edited

Loading

What are you trying to accomplish?

This change addresses the issue where the NPM version detection failed when the version is empty or set to 0. The fix ensures that the error is handled gracefully, allowing Dependabot to properly detect and work with NPM versions that were previously unsupported.

Anything you want to highlight for special attention from reviewers?

The fix specifically targets the issue where an unsupported error occurred due to the version being empty or 0. It ensures that these edge cases are handled without causing a disruption in the update process.

How will you know you've accomplished your goal?

The issue of failing to detect NPM versions that are empty or 0 should be resolved.
The system should now handle these cases without raising errors or affecting other functionality.
Automated tests should pass successfully, and manual validation should show that the detection now works correctly for edge cases.

Checklist

I have run the complete test suite to ensure all tests and linters pass.
I have thoroughly tested my code changes to ensure they work as expected, including adding additional tests for new functionality.
I have written clear and descriptive commit messages.
I have provided a detailed description of the changes in the pull request, including the problem it addresses, how it fixes the problem, and any relevant details about the implementation.
I have ensured that the code is well-documented and easy to understand.

kbukum1 marked this pull request as ready for review

January 29, 2025 00:30

kbukum1 requested a review from a team as a code owner

January 29, 2025 00:30

github-actions bot added the L: javascript label

kbukum1 commented

View reviewed changes

npm_and_yarn/lib/dependabot/npm_and_yarn/npm_package_manager.rb Show resolved Hide resolved

kbukum1 commented

View reviewed changes

npm_and_yarn/lib/dependabot/npm_and_yarn/npm_package_manager.rb Show resolved Hide resolved

kbukum1 commented

View reviewed changes

npm_and_yarn/lib/dependabot/npm_and_yarn/package_manager.rb Outdated

    
            @@ -308,17 +308,19 @@ def detect_version(name)
          
                      end

                      # if "packageManager" have no version specified, we check if we can extract "engines" information

                      detected_version = check_engine_version(name) if !detected_version || detected_version.empty?

                      detected_version = check_engine_version(name) if detected_version&.empty?

Contributor Author

kbukum1 Jan 29, 2025

Review Tip: Simplified

kbukum1 commented

View reviewed changes

npm_and_yarn/lib/dependabot/npm_and_yarn/package_manager.rb Outdated

    
                      # if "packageManager" and "engines" both are not present, we check if we can infer the version

                      # from the manifest file lockfileVersion

                      detected_version = guessed_version(name) if !detected_version || detected_version.empty?

                      detected_version = guessed_version(name) if detected_version&.empty?

Contributor Author

kbukum1 Jan 29, 2025 •

edited

Loading

Review Tip: Simplified

kbukum1 commented

View reviewed changes

npm_and_yarn/lib/dependabot/npm_and_yarn/package_manager.rb Outdated


		detected_version&.to_s
		return nil if detected_version.nil? \|\| detected_version.to_s.empty?

Contributor Author

kbukum1 Jan 29, 2025

Review Tip: We want to be sure we return nil instead of empty when there is no detected version. Note that detected version can be string or integer.

kbukum1 commented

View reviewed changes

npm_and_yarn/lib/dependabot/npm_and_yarn/package_manager.rb Show resolved Hide resolved

kbukum1 commented

View reviewed changes

npm_and_yarn/lib/dependabot/npm_and_yarn/package_manager.rb Outdated

    
            @@ -327,7 +329,7 @@ def package_manager_by_name(name)
          
                      detected_version = detect_version(name)

                      # if we have a detected version, we check if it is deprecated or unsupported

                      if detected_version

                      unless detected_version&.empty?

Contributor Author

kbukum1 Jan 29, 2025 •

edited

Loading

Review Tip: Check if detected_version has value.

kbukum1 commented

View reviewed changes

npm_and_yarn/lib/dependabot/npm_and_yarn/npm_package_manager.rb Show resolved Hide resolved

kbukum1 commented

View reviewed changes

npm_and_yarn/lib/dependabot/npm_and_yarn/package_manager.rb Show resolved Hide resolved

kbukum1 mentioned this pull request

NPM version detection broken #11234

Closed

1 task

Contributor Author

kbukum1 commented Jan 29, 2025

Fix for #11234


          ensure detected version is valid version before passing into package …

bc37097

…manager

kbukum1 force-pushed the kamil/fix-npm-unsupported-error branch from 3eadd3a to bc37097 Compare

January 29, 2025 01:03

kbukum1 requested review from amazimbe and abdulapopoola

January 29, 2025 01:05

kbukum1 added 5 commits

January 28, 2025 17:09


          fix the wrong checks

102ff3b


          revert back the check to original

ac48f45


          fix the wrong check

1f5d982


          We only need to check if detect_version is not nil

c387414


          Merge branch 'main' into kamil/fix-npm-unsupported-error

74a55ab

amazimbe approved these changes

View reviewed changes

bcomnes commented Jan 29, 2025 •

edited

Loading

Can you clarify which version of npm you will default to when none is specified? Whatever npm version ships with with the current node LTS seems reasonable.

This sounds like this will fix the issue introduced last week.

Also related: #11373

Thanks for getting a fix out for this.

Contributor Author

kbukum1 commented Jan 29, 2025

Can you clarify which version of npm you will default to when none is specified? Whatever npm version ships with with the current node LTS seems reasonable.

This sounds like this will fix the issue introduced last week.

Also related: #11373

Thanks for getting a fix out for this.

Default npm Version When None is Specified:
- If there is no lockfile or the lockfile is empty, we default to npm 10.
- If the lockfile exists and contains a valid lockfileVersion, we determine the npm version as follows:
  - lockfileVersion >= 3 → npm 10
  - lockfileVersion == 2 → npm 8
  - lockfileVersion == 1 → npm 8
- If a lockfile is present but cannot be parsed, we fall back to npm 8.
Note that these versions are used for making decisions related to how the manifest and lockfile are structured. However, when running npm for operations such as install, we do not use the detected version. Instead, we explicitly run npm with NPM_VERSION=9.6.5.

Let me know if you need any further clarification!

Contributor Author

kbukum1 commented Jan 29, 2025

Fixes:


          Merge branch 'main' into kamil/fix-npm-unsupported-error

35426a2

bcomnes commented Jan 29, 2025

Great. Works for me thank you.

kbukum1 merged commit 5367275 into main

66 checks passed

kbukum1 deleted the kamil/fix-npm-unsupported-error branch

January 29, 2025 19:40

Contributor Author

kbukum1 commented Jan 29, 2025 •

edited

Loading

The fix has been shipped. Could you check your repository to see if it's working now? You may need to rerun the process to reflect the changes, as the fix was just released.

kbukum1 commented

View reviewed changes

Contributor Author

kbukum1 left a comment

commented review tips

kbukum1 mentioned this pull request

Dependabot fails to update a package.json without a lock file #11373

Closed

1 task

bcomnes commented Jan 29, 2025

Confirmed working as before. Thank you @kbukum1

kbukum1 mentioned this pull request

Fix npm_and_yarn engine detection #11392

Merged

5 tasks

Contributor Author

kbukum1 commented Jan 29, 2025

Confirmed working as before. Thank you @kbukum1

Thanks for the confirmation.

kbukum1 self-assigned this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels