-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Update npm_and_yarn
deprecation and unsupported checks for npm
, pnpm
, and yarn
package managers
#11240
Update npm_and_yarn
deprecation and unsupported checks for npm
, pnpm
, and yarn
package managers
#11240
Conversation
@@ -174,7 +172,7 @@ def self.fetch_yarnrc_yml_value(key, default_value) | |||
def self.npm8?(package_lock) | |||
return true unless package_lock&.content | |||
|
|||
if Dependabot::Experiments.enabled?(:enable_corepack_for_npm_and_yarn) | |||
if Dependabot::Experiments.enabled?(:npm_v6_deprecation_warning) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Tip:
The method npm_version_numeric_latest
is used for deprecation/unsupported checks. However, for consistency across package managers, we should use npm_version_numeric
and conditionally delegate to npm_version_numeric_latest
based on the feature flag npm_v6_deprecation_warning
. This aligns with the detection method naming conventions used by other package managers, such as #{name}_version_numeric
.
Detection methods for reference:
npm_version_numeric
pnpm_version_numeric
yarn_version_numeric
This approach ensures consistent version detection logic across all package managers in the npm_and_yarn
ecosystem.
|
||
detected_version&.to_s | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Tip:
Added a proper detection method that should be used for detection of version instead of directly using Helper detection method.
@amazimbe : FYI
if Dependabot::Experiments.enabled?(:enable_corepack_for_npm_and_yarn) | ||
return npm_version_numeric_latest(lockfile) | ||
end | ||
return npm_version_numeric_latest(lockfile) if Dependabot::Experiments.enabled?(:npm_v6_deprecation_warning) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Tip:
We need to use npm_v6_deprecation_warning
feature flag to use the new method to detect version and raise deprecation/unsupported notification if necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious if :enable_corepack_for_npm_and_yarn is no longer needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is more for corepack installation and doesn't matter for this part.
@@ -72,14 +72,16 @@ class NpmPackageManager < Ecosystem::VersionManager | |||
|
|||
sig do | |||
params( | |||
raw_version: String, | |||
detected_version: T.nilable(String), | |||
raw_version: T.nilable(String), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Tip:
- Detected Version: Extracted from manifest or lock files to determine the project's specified package manager version.
- Raw Version: Obtained using versioning commands to identify the active package manager version at runtime.
This distinction ensures precise deprecation checks and reliable metrics collection.
This PR modifies the npm v6 deprecation/unsupported checks by updating the detection method. It would be great if you could review the changes and, if everything looks good, proceed with deployment. Additionally, you can verify that your previous work functions as expected, which will also serve as a test for my changes as well. Thanks! |
super( | ||
name: NAME, | ||
version: Version.new(raw_version), | ||
detected_version: detected_version ? Version.new(detected_version) : nil, | ||
version: raw_version ? Version.new(raw_version) : nil, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the raw_version is nil wouldn't we want to set the version to the detected version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to show that we don't have raw_version on metrics. Later when we add detection version we can see what we are detecting and what we are running on.
What are you trying to accomplish?
This PR updates the handling of deprecation and unsupported checks for the
npm_and_yarn
ecosystem's package managers:npm
,pnpm
, andyarn
.The changes include:
npm
that prioritizes checking thepackageManager
field,engines
, and lockfiles in that order to determine the version.npm
to ensure alignment with the ecosystem's requirements.pnpm
andyarn
consistently returnfalse
for deprecation and unsupported checks, as no such logic applies to them.Why?
packageManager
andengines
fields along with lockfiles fornpm
.npm_and_yarn
ecosystem package managers in Dependabot.Anything you want to highlight for special attention from reviewers?
Version Detection for
npm
:packageManager
field,engines
field, and lockfiles in sequence.Deprecation and Unsupported Logic:
npm
: Adjusted logic to use the detected version for determining deprecation and unsupported status.pnpm
andyarn
: Explicitly set deprecation and unsupported checks to always returnfalse
.Test Updates:
npm
, tests now cover cases where versions are determined usingpackageManager
,engines
, or lockfiles.pnpm
andyarn
validate consistent return values (false
) for deprecation and unsupported checks.How will you know you've accomplished your goal?
npm_and_yarn
ecosystem package managers pass successfully.npm
.false
) forpnpm
andyarn
.Checklist