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.

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

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

Merged
merged 8 commits into from
Nov 21, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
12814b4
fix default npm version as 10
kbukum1 Nov 21, 2024
08551eb
Merge branch 'main' into kamil/fix_run_default_npm_version_as_latest_10
kbukum1 Nov 21, 2024
bd522e3
select right npm version considering lockfile version
kbukum1 Nov 21, 2024
295d269
Merge branch 'main' into kamil/fix_run_default_npm_version_as_latest_10
kbukum1 Nov 21, 2024
42ce2f4
fix lockfileVersion for the spec
kbukum1 Nov 21, 2024
5d1bb41
refine the version detection method
kbukum1 Nov 21, 2024
759c24e
change variable name to be precise
kbukum1 Nov 21, 2024
59d2cfe
comments are refined
kbukum1 Nov 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion npm_and_yarn/lib/dependabot/npm_and_yarn/file_parser.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
require "dependabot/npm_and_yarn/native_helpers"
require "dependabot/npm_and_yarn/version"
require "dependabot/npm_and_yarn/requirement"
require "dependabot/npm_and_yarn/package_manager"
require "dependabot/npm_and_yarn/registry_parser"
require "dependabot/git_metadata_fetcher"
require "dependabot/git_commit_checker"
Expand Down Expand Up @@ -477,4 +478,4 @@ def requirement_class
end

Dependabot::FileParsers
.register("npm_and_yarn", Dependabot::NpmAndYarn::FileParser)
.register(Dependabot::NpmAndYarn::ECOSYSTEM, Dependabot::NpmAndYarn::FileParser)
35 changes: 35 additions & 0 deletions npm_and_yarn/lib/dependabot/npm_and_yarn/helpers.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

NPM_V8 = 8
NPM_V6 = 6
NPM_DEFAULT_VERSION = NPM_V8
Expand All @@ -40,6 +41,10 @@ module Helpers
# Otherwise, we are going to use old versionining npm 6
sig { params(lockfile: T.nilable(DependencyFile)).returns(Integer) }
def self.npm_version_numeric(lockfile)
if Dependabot::Experiments.enabled?(:enable_corepack_for_npm_and_yarn)
return npm_version_numeric_latest(lockfile)
end

fallback_version_npm8 = Dependabot::Experiments.enabled?(:npm_fallback_version_above_v6)

return npm_version_numeric_npm8_or_higher(lockfile) if fallback_version_npm8
Expand Down Expand Up @@ -91,6 +96,36 @@ def self.npm_version_numeric_npm8_or_higher(lockfile)
NPM_DEFAULT_VERSION # Fallback to default npm version if parsing fails
end

# rubocop:disable Metrics/PerceivedComplexity
sig { params(lockfile: T.nilable(DependencyFile)).returns(Integer) }
def self.npm_version_numeric_latest(lockfile)
lockfile_content = lockfile&.content

# Return npm 10 as the default if the lockfile is missing or empty
return NPM_V10 if lockfile_content.nil? || lockfile_content.strip.empty?

# Parse the lockfile content to extract the `lockfileVersion`
parsed_lockfile = JSON.parse(lockfile_content)
lockfile_version = parsed_lockfile["lockfileVersion"]&.to_i

# Determine the appropriate npm version based on `lockfileVersion`
if lockfile_version.nil?
NPM_V10 # Use npm 10 if `lockfileVersion` is missing or nil
elsif lockfile_version >= 3
NPM_V10 # Use npm 10 for lockfileVersion 3 or higher
elsif lockfile_version >= 2
NPM_V8 # Use npm 8 for lockfileVersion 2
elsif lockfile_version >= 1
# Use npm 8 if the fallback version flag is enabled, otherwise use npm 6
Dependabot::Experiments.enabled?(:npm_fallback_version_above_v6) ? NPM_V8 : NPM_V6
else
NPM_V10 # Default to npm 10 for unexpected or unsupported versions
end
rescue JSON::ParserError
NPM_V8 # Fallback to npm 8 if the lockfile content cannot be parsed
end
Copy link
Contributor Author

@kbukum1 kbukum1 Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@honeyankit honeyankit Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

the logic is correct. Ignore me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screenshot 2024-11-21 at 2 40 13 PM

# rubocop:enable Metrics/PerceivedComplexity

sig { params(yarn_lock: T.nilable(DependencyFile)).returns(Integer) }
def self.yarn_version_numeric(yarn_lock)
lockfile_content = yarn_lock&.content
Expand Down
4 changes: 3 additions & 1 deletion npm_and_yarn/lib/dependabot/npm_and_yarn/package_manager.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,13 +26,15 @@ class NpmPackageManager < Ecosystem::VersionManager
NPM_V7 = "7"
NPM_V8 = "8"
NPM_V9 = "9"
NPM_V10 = "10"

# Keep versions in ascending order
SUPPORTED_VERSIONS = T.let([
Version.new(NPM_V6),
Version.new(NPM_V7),
Version.new(NPM_V8),
Version.new(NPM_V9)
Version.new(NPM_V9),
Version.new(NPM_V10)
].freeze, T::Array[Dependabot::Version])

DEPRECATED_VERSIONS = T.let([].freeze, T::Array[Dependabot::Version])
Expand Down
5 changes: 5 additions & 0 deletions npm_and_yarn/spec/dependabot/npm_and_yarn/file_parser_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,14 @@
# Variable to control the npm fallback version feature flag
let(:npm_fallback_version_above_v6_enabled) { true }

# Variable to control the enabling feature flag for the corepack fix
let(:enable_corepack_for_npm_and_yarn) { true }

before do
allow(Dependabot::Experiments).to receive(:enabled?)
.with(:npm_fallback_version_above_v6).and_return(npm_fallback_version_above_v6_enabled)
allow(Dependabot::Experiments).to receive(:enabled?)
.with(:enable_corepack_for_npm_and_yarn).and_return(enable_corepack_for_npm_and_yarn)
end

after do
Expand Down
2 changes: 1 addition & 1 deletion npm_and_yarn/spec/dependabot/npm_and_yarn/file_updater/npm_lockfile_updater_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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"
      }
    }
  }
}

end

Expand Down
4 changes: 2 additions & 2 deletions ...fixtures/projects/npm8/workspace_outdated_deps_not_in_root_package_json/package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading