Set Default `npm` Version to 10 Under enable_corepack_for_npm_and_yarn Feature Flag #10985

kbukum1 · 2024-11-21T19:35:13Z

What are you trying to accomplish?

This PR addresses general issues with npm version detection and provides a robust solution to handle version determination based on the package-lock.json file. It updates the enable_corepack_for_npm_and_yarn feature by setting the default npm version to 10 when the feature flag is enabled. Previously, the default npm version was set to 8. This update ensures compatibility with the latest npm features while solving broader problems related to npm version detection.

Why is this change needed?

To address inconsistencies in detecting the installed npm version across different lockfile scenarios.
To align with the latest npm version (10) for improved functionality and support.
To provide a seamless user experience by handling various edge cases, including missing or malformed lockfiles.

Anything you want to highlight for special attention from reviewers?

The new method npm_version_numeric_latest determines the npm version to use based on the lockfileVersion field in the package-lock.json file. It defaults to npm 10 in cases where the field is missing or invalid.
Conditional logic was added in npm_version_numeric to call npm_version_numeric_latest when the feature flag is enabled.
Edge cases, such as empty or incorrectly formatted lockfiles, have been considered in the implementation.

How will you know you've accomplished your goal?

Tests confirm the correct npm version is detected based on the lockfile version and that the default is set to npm 10 under the enable_corepack_for_npm_and_yarn feature flag.
The implementation handles scenarios with and without valid lockfiles gracefully.
Users experience consistent and accurate npm version detection, with npm 10 as the default when the feature flag is active.

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 · 2024-11-21T22:11:39Z

npm_and_yarn/lib/dependabot/npm_and_yarn/helpers.rb

+        end
+      rescue JSON::ParserError
+        NPM_DEFAULT_VERSION # Fallback to default npm version if parsing fails
+      end


Review Tip:

When the lockfileVersion is 3 or higher, npm 9 or npm 10 could be selected. Since npm 10 maintains compatibility with previous versions, we are opting for the latest version, npm 10.

When the lockfileVersion is 2, npm 7 or npm 8 could be chosen. However, given npm 8's compatibility with older versions, we are selecting the higher version, npm 8.

When the lockfileVersion is 1, npm 6 is typically expected. However, if the feature flag npm_fallback_version_above_v6 is enabled, we select npm 8 instead. Currently, this feature flag is disabled, but it will be enabled after monitoring metrics.

In all other scenarios, we default to npm 10, the latest version. If the lockfile cannot be parsed, npm 8 is used as a fallback version to ensure stability.

@kbukum1 : There is a bug in the logic for determining the appropriate npm version based on lockfile_version:
• Issue: If lockfile_version == 2, the current logic incorrectly assigns NPM_V10 instead of NPM_V8.

the logic is correct. Ignore me.

I am checking it but it seems to be npm 8? Are we looking in different place?

/~https://github.com/dependabot/dependabot-core/pull/10985/files/5d1bb41a83abec00e9f77fb2ef0bd94482b85d5c#diff-85ec3865f2fdb48045a2536b37b847156ed3bd3960afdc8bd900ae568de9acd7R116

jonabc

a couple points of feedback but nothing blocking!

jonabc · 2024-11-21T22:25:02Z

npm_and_yarn/lib/dependabot/npm_and_yarn/helpers.rb

+        if lockfile_version.nil?
+          NPM_V10 # Default if lockfileVersion is missing or nil
+        elsif lockfile_version >= 3
+          NPM_V10
+        elsif lockfile_version >= 2
+          NPM_V8
+        elsif lockfile_version >= 1
+          Dependabot::Experiments.enabled?(:npm_fallback_version_above_v6) ? NPM_V8 : NPM_V6
+        else
+          NPM_V10 # Fallback to npm 10 for unsupported or unexpected versions
+        end


not really applicable at the moment, but as a personal preference I think this would be a little cleaner and easier to maintain as a hash if/when the experiment on npm_fallback_version_above_v6 is removed.

NPM_VERSION_FOR_LOCKFILE_VERSION={ 1: NPM_V8, 2: NPM_V8, 3: NPM_V10, }.freeze return NPM_V10 unless NPM_VERSION_FOR_LOCKFILE_VERSION.has_key?(lockfile_version) NPM_VERSION_FOR_LOCKFILE_VERSION[lockfile_version]

I think this would be identical to the description you gave below and covers the >= 3, nil and any other cases of not-explicitly set keys by returning V10.

Also, it's worth calling out that between the multiple experiment flags being used as well as the different "default" versions of V8 vs V10, it will be fairly confusing to manually debug what the expected npm version is in any specific case

After this fix, we plan to monitor metrics for npm 6 and eventually remove it entirely once the feature flag is enabled. At that time, I intend to make those adjustments. However, for now, this fix focuses on handling versions running on npm 8 instead of npm 10. We are getting error because of running on npm 8 for some customers after the corepack fix, #10944

jonabc · 2024-11-21T22:31:39Z

npm_and_yarn/lib/dependabot/npm_and_yarn/helpers.rb

@@ -16,6 +16,7 @@ module Helpers
        /^.*(?<error>The "yarn-path" option has been set \(in [^)]+\), but the specified location doesn't exist)/

      # NPM Version Constants
+      NPM_V10 = 10


Is it necessary to define these constants here as integers and in npm_and_yarn/lib/dependabot/npm_and_yarn/package_manager.rb as strings? Could they be defined in one location and business logic makes the necessary conversion of number<->string as needed?

Eventually, we’ll need to refactor this to define all versions as strings instead of integers. However, this cleanup will take place after we finish removing all feature flags related to versioning and deprecate the old ecosystem metrics collection. For now, the specs require the method to return an integer, and this change is outside the scope of the current issue.

kbukum1 · 2024-11-21T22:32:01Z

npm_and_yarn/spec/dependabot/npm_and_yarn/file_updater/npm_lockfile_updater_spec.rb

@@ -1089,7 +1089,7 @@
    let(:previous_requirements) { requirements }

    it "raises a helpful error" do
-      expect { updated_npm_lock_content }.to raise_error(Dependabot::DependencyFileNotResolvable)
+      expect { updated_npm_lock_content }.to raise_error(Dependabot::InconsistentRegistryResponse)
    end


The error being thrown has changed because the lockfileVersion for these dependency files is 3. Previously, the spec used npm 8, but with this update, it now defaults to npm 10 to align with the latest version for lockfileVersion 3 and above.

Example lockfile:

{ "name": "DependencyNotFound", "version": "0.36.1", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "DependencyNotFound", "version": "0.36.1", "license": "MIT", "dependencies": {}, "devDependencies": { "eslint": "^9.5.0" }, "engines": { "node": ">=12" } } } }

honeyankit

/~https://github.com/dependabot/dependabot-core/pull/10985/files#r1852977132

kbukum1 added 2 commits November 21, 2024 11:30

fix default npm version as 10

12814b4

Merge branch 'main' into kamil/fix_run_default_npm_version_as_latest_10

08551eb

github-actions bot added the L: javascript label Nov 21, 2024

kbukum1 added 2 commits November 21, 2024 13:24

select right npm version considering lockfile version

bd522e3

Merge branch 'main' into kamil/fix_run_default_npm_version_as_latest_10

295d269

kbukum1 requested review from Nishnha and abdulapopoola November 21, 2024 21:31

fix lockfileVersion for the spec

42ce2f4

kbukum1 marked this pull request as ready for review November 21, 2024 21:35

kbukum1 requested a review from a team as a code owner November 21, 2024 21:35

refine the version detection method

5d1bb41

kbukum1 commented Nov 21, 2024

View reviewed changes

kbukum1 added 2 commits November 21, 2024 14:17

change variable name to be precise

759c24e

comments are refined

59d2cfe

kbukum1 requested review from jonabc and honeyankit November 21, 2024 22:20

jonabc approved these changes Nov 21, 2024

View reviewed changes

kbukum1 commented Nov 21, 2024

View reviewed changes

honeyankit reviewed Nov 21, 2024

View reviewed changes

kbukum1 requested a review from honeyankit November 21, 2024 22:44

kbukum1 self-assigned this Nov 21, 2024

kbukum1 merged commit 4a97987 into main Nov 21, 2024
248 checks passed

kbukum1 deleted the kamil/fix_run_default_npm_version_as_latest_10 branch November 21, 2024 23:03

steve-gombos mentioned this pull request Dec 2, 2024

NPM Install from a private registry #11038

Open

1 task

chbiel mentioned this pull request Jan 6, 2025

NPM version detection broken #11234

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set Default `npm` Version to 10 Under enable_corepack_for_npm_and_yarn Feature Flag #10985

Set Default `npm` Version to 10 Under enable_corepack_for_npm_and_yarn Feature Flag #10985

kbukum1 commented Nov 21, 2024 •

edited

Loading

kbukum1 Nov 21, 2024 •

edited

Loading

honeyankit Nov 21, 2024 •

edited

Loading

honeyankit Nov 21, 2024

kbukum1 Nov 21, 2024

kbukum1 Nov 21, 2024

jonabc left a comment

jonabc Nov 21, 2024

jonabc Nov 21, 2024

kbukum1 Nov 21, 2024

jonabc Nov 21, 2024

kbukum1 Nov 21, 2024

kbukum1 Nov 21, 2024

honeyankit left a comment

Set Default npm Version to 10 Under enable_corepack_for_npm_and_yarn Feature Flag #10985

Set Default npm Version to 10 Under enable_corepack_for_npm_and_yarn Feature Flag #10985

Conversation

kbukum1 commented Nov 21, 2024 • edited Loading

What are you trying to accomplish?

Why is this change needed?

Anything you want to highlight for special attention from reviewers?

How will you know you've accomplished your goal?

Checklist

kbukum1 Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

honeyankit Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonabc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

honeyankit left a comment

Choose a reason for hiding this comment

Set Default `npm` Version to 10 Under enable_corepack_for_npm_and_yarn Feature Flag #10985

Set Default `npm` Version to 10 Under enable_corepack_for_npm_and_yarn Feature Flag #10985

kbukum1 commented Nov 21, 2024 •

edited

Loading

kbukum1 Nov 21, 2024 •

edited

Loading

honeyankit Nov 21, 2024 •

edited

Loading