Add support for centralized package manager abstraction for `npm_and_yarn` ecosystem #10862

kbukum1 · 2024-10-28T23:07:12Z

What are you trying to accomplish?

This PR updates the npm_and_yarn ecosystem by introducing support for centralized package manager abstraction. Previously, the ecosystem relied on PackageManagerHelper for managing package managers like npm, yarn, and pnpm, without distinguishing them into separate classes.

With this change, we are introducing individual package manager classes (NpmPackageManager, YarnPackageManager, PNPMPackageManager) in line with the centralized package manager abstraction that is already used for other ecosystems such as Bundler and Composer. This brings npm_and_yarn in line with the architecture used across different ecosystems, providing a more uniform and extensible approach.

By centralizing the package manager handling for npm_and_yarn, we aim to standardize and simplify package manager management across ecosystems, improving code maintainability, consistency, and scalability.

Anything you want to highlight for special attention from reviewers?

We are refactoring how npm, yarn, and pnpm package managers are handled by creating distinct classes for each of them.
The PackageManagerHelper class is being refactored to use the new abstraction, and this change should align with the existing centralized model in use for other ecosystems.
Focus should be on ensuring that this change integrates well with the existing framework and doesn't introduce regressions, particularly around how package managers are identified and utilized.

How will you know you've accomplished your goal?

The npm_and_yarn ecosystem will have distinct package manager classes for npm, yarn, and pnpm, similar to how it is done for Bundler and Composer ecosystems.
All package manager handling for the npm_and_yarn ecosystem will be centralized, making it consistent with the rest of the project.
The refactored code passes all relevant tests, and functionality remains consistent across the npm, yarn, and pnpm package managers.

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-10-30T20:14:35Z

npm_and_yarn/lib/dependabot/npm_and_yarn/file_fetcher.rb

-                                     lockfiles: { npm: package_lock || shrinkwrap, yarn: yarn_lock, pnpm: pnpm_lock }
-                                   ), T.nilable(PackageManager))
+      sig { returns(PackageManagerHelper) }
+      def package_manager_helper


We change the naming as package_manager_helper to reduce confusion.

This class was used as utility to install package manager for file_fetcher

We created Package Manager for each npm, yarn and pnpm and the helper is going to also help to create instance for package manager.

kbukum1 · 2024-10-30T20:16:18Z

npm_and_yarn/lib/dependabot/npm_and_yarn/file_parser.rb

+          ),
+          T.nilable(Ecosystem)
+        )
+      end


This method is used in dependency_snapshot to acquire information related to ecosystem, package manager and in upcoming PRs is going to be language information. That is also used for deprecation and unsupported checks however currently for npm_and_yarn they are disabled.

kbukum1 · 2024-10-30T20:18:12Z

npm_and_yarn/lib/dependabot/npm_and_yarn/file_parser.rb

+          f.name == "pnpm-lock.yaml"
+        end, T.nilable(Dependabot::DependencyFile))
+      end
+


To create an instance from the package manager helper, we require dependency_files. This helper is already being used in the file_fetcher, and to avoid duplication and maintain consistency, we're using the same helper in the file_parser.

kbukum1 · 2024-10-30T20:19:12Z

npm_and_yarn/lib/dependabot/npm_and_yarn/helpers.rb

      def self.npm_version_numeric_npm6_or_higher(lockfile)
-        lockfile_content = T.must(lockfile.content)


If the lockfile is missing content, we want to return the default version rather than triggering a Sorbet type check that causes an error.

kbukum1 · 2024-10-30T20:23:36Z

npm_and_yarn/lib/dependabot/npm_and_yarn/package_manager.rb

+      YarnPackageManager::NAME => YarnPackageManager,
+      PNPMPackageManager::NAME => PNPMPackageManager
+    }.freeze
+


We created a dedicated Package Manager for each package manager in npm_and_yarn to support a centralized structure. This provides ecosystem, package manager details, as well as deprecation and unsupported version information to dependabot-common.

kbukum1 · 2024-10-30T20:25:10Z

npm_and_yarn/lib/dependabot/npm_and_yarn/package_manager.rb

+          return manager_name.to_s if @engines[manager_name.to_s]
+        end
+      end
+    end


This class is created to detect which package manager is going to be used by dependabot to run update process.

kbukum1 · 2024-10-30T20:26:02Z

npm_and_yarn/lib/dependabot/npm_and_yarn/package_manager.rb

+      sig { returns(Ecosystem::VersionManager) }
+      def package_manager
+        name = @package_manager_detector.detect_package_manager || DEFAULT_PACKAGE_MANAGER
+        package_manager_by_name(name)
      end


After detection package manager we want to return the created package manager information that can be used for metrics collection or other purposes.

kbukum1 · 2024-10-30T20:26:59Z

npm_and_yarn/lib/dependabot/npm_and_yarn/package_manager.rb

+        version = Helpers.send(:"#{name}_version_numeric", @lockfiles[name.to_sym])
+
+        package_manager_class.new(version.to_s)
+      end


We want to initiate package manager by it is name and for default we are using npm if we are unable to detect package manager name from dependency_files.

abdulapopoola · 2024-10-31T21:32:53Z

npm_and_yarn/lib/dependabot/npm_and_yarn/file_fetcher.rb

@@ -182,25 +182,36 @@ def inferred_npmrc # rubocop:disable Metrics/PerceivedComplexity

      sig { returns(T.nilable(T.any(Integer, String))) }
      def npm_version
-        @npm_version ||= T.let(package_manager.setup("npm"), T.nilable(T.any(Integer, String)))
+        @npm_version ||= T.let(package_manager_helper.setup("npm"), T.nilable(T.any(Integer, String)))


do we have constants for npm, pnpm, and yarn?

Yes, I created them. Will replace constants.

abdulapopoola · 2024-10-31T21:34:42Z

npm_and_yarn/lib/dependabot/npm_and_yarn/helpers.rb

@@ -37,7 +37,7 @@ module Helpers
      # Determines the npm version depends to the feature flag
      # If the feature flag is enabled, we are going to use the minimum version npm 8
      # Otherwise, we are going to use old versionining npm 6
-      sig { params(lockfile: DependencyFile).returns(Integer) }
+      sig { params(lockfile: T.nilable(DependencyFile)).returns(Integer) }


any reason why this is nilable?

Yes, npm is our default package manager when there’s no lockfile. This method returns the default version if the lockfile is nil. We need it to be nilable to ensure that if only a package.json is present, it correctly returns the default npm version.

After further review, nilable is needed for all version checks (npm, yarn, pnpm). The package manager can be specified in package.json -> engines or package.json -> packageManager even without a lockfile, and in these cases, Helper provides the default version. Since Helper handles version detection for all package managers, keeping lockfile as nilable keeps the logic unified, avoiding the need for callers to handle default versions separately.

abdulapopoola · 2024-10-31T21:35:33Z

npm_and_yarn/lib/dependabot/npm_and_yarn/helpers.rb

+        lockfile_content = lockfile&.content
+
+        return NPM_V8 if lockfile_content.nil? || lockfile_content.strip.empty?
+


can this be simplified some more?

We need a stronger check here, including a nil check, because JSON.parse(lockfile_content) won’t accept a nil value, and Sorbet will flag this as an issue. Additionally, we want to ensure the content is not an empty string.

Sorry I was not clearer; I meant the conditions on lines 53 and 55 can be simplified together

abdulapopoola · 2024-10-31T21:35:59Z

npm_and_yarn/lib/dependabot/npm_and_yarn/helpers.rb

@@ -85,8 +88,12 @@ def self.npm_version_numeric_npm8_or_higher(lockfile)
        NPM_DEFAULT_VERSION # Fallback to default npm version if parsing fails
      end

-      sig { params(yarn_lock: DependencyFile).returns(Integer) }
+      sig { params(yarn_lock: T.nilable(DependencyFile)).returns(Integer) }


any reason to introduce nilable? I'd prefer to avoid if possible

removed nillable from all other methods. Only npm version checks can't be strict because when there is no lock file we set default npm and need to return the default version.

After further review, nilable is needed for all version checks (npm, yarn, pnpm). The package manager can be specified in package.json -> engines or package.json -> packageManager even without a lockfile, and in these cases, Helper provides the default version. Since Helper handles version detection for all package managers, keeping lockfile as nilable keeps the logic unified, avoiding the need for callers to handle default versions separately.

I think we should do another pass in a separate arch clean up or refactoring pass to eliminate as many nilables as we can. OK to punt the discussion on nilables to that time.

I will take a note. Another refactor PR we can try to solve this.

abdulapopoola · 2024-10-31T21:37:41Z

npm_and_yarn/lib/dependabot/npm_and_yarn/package_manager.rb

+        @engines = package_json.fetch("engines", nil)
+      end
+
+      sig { returns(T.nilable(String)) }


can this return a strongly-typed value to minimize the risk of typos?

other then npm all others are replaced to be String can't be nil. Also current flow doesn't allow other then npm, yarn, and pnpm to return here. Since we have npm as default in other cases then this package manager we are returning the default npm.

…, aligning with centralized package manager abstraction used across ecosystems.

…as `npm`.

abdulapopoola · 2024-11-01T16:06:16Z

npm_and_yarn/lib/dependabot/npm_and_yarn/package_manager.rb

-    class PackageManager
+    ECOSYSTEM = "npm_and_yarn"
+    MANIFEST_FILENAME = "package.json"
+    LERNA_JSON_FILENAME = "lerna.json"


Lerna should be its own package manager?

We don't need lerna because we are using lerna as workspace management. After passing the found project into it is package manager.

abdulapopoola · 2024-11-01T16:06:58Z

npm_and_yarn/lib/dependabot/npm_and_yarn/file_parser.rb

+      sig { returns(T.nilable(Dependabot::DependencyFile)) }
+      def package_lock
+        @package_lock ||= T.let(dependency_files.find do |f|
+          f.name == "package-lock.json"


I think these are already constants somewhere; can we replace with the actual constant references?

Also OK to do these in a separate follow up PR

This is done.

…d_package_manager

kbukum1 · 2024-11-04T18:06:41Z

Note: We need to open another PR to refactor the versioning for npm_and_yarn. Currently for file_fetcher and file_parser we are using different versioning so we need to use same version for each process. Also for following commen we need refactoring to possibly remove nilable from the sorbet typings.

Add support for centralized package manager abstraction for npm_and_yarn ecosystem #10862 (comment)

…d_package_manager

honeyankit

Looks good @kbukum1. How we will revert in case of any failures?

honeyankit · 2024-11-05T00:27:20Z

@kbukum1 : Can we deploy tomorrow, not right now.

kbukum1 · 2024-11-05T00:33:36Z

@kbukum1 : Can we deploy tomorrow, not right now.

That's works. In case if there is any issue we are going to revert the changes which are on package manager.

…d_package_manager

…attribute in manifest file

github-actions bot added L: php:composer Issues and code for Composer L: ruby:bundler RubyGems via bundler L: javascript labels Oct 28, 2024

kbukum1 changed the base branch from main to kamil/update_package_manager_abstraction_for_bundler_and_composer October 28, 2024 23:07

kbukum1 mentioned this pull request Oct 28, 2024

Draft PR: Update package managers (bundler, composer, and npm_and_yarn) to support generalized abstraction #10825

Closed

5 tasks

kbukum1 force-pushed the kamil/update_npm_and_yarn_support_centralized_package_manager branch from 02ad7aa to 6383a57 Compare October 29, 2024 22:49

kbukum1 marked this pull request as ready for review October 30, 2024 17:10

kbukum1 requested a review from a team as a code owner October 30, 2024 17:10

kbukum1 requested review from abdulapopoola and jakecoffman October 30, 2024 17:10

kbukum1 commented Oct 30, 2024

View reviewed changes

Base automatically changed from kamil/update_package_manager_abstraction_for_bundler_and_composer to main October 31, 2024 21:07

kbukum1 force-pushed the kamil/update_npm_and_yarn_support_centralized_package_manager branch from 9363a35 to b55394d Compare October 31, 2024 21:12

abdulapopoola reviewed Oct 31, 2024

View reviewed changes

kbukum1 removed L: php:composer Issues and code for Composer L: ruby:bundler RubyGems via bundler labels Oct 31, 2024

kbukum1 force-pushed the kamil/update_npm_and_yarn_support_centralized_package_manager branch 2 times, most recently from cce7bcd to 88997f5 Compare October 31, 2024 23:47

Refactor npm_and_yarn to use separate classes for npm, yarn, and pnpm…

788c271

…, aligning with centralized package manager abstraction used across ecosystems.

kbukum1 force-pushed the kamil/update_npm_and_yarn_support_centralized_package_manager branch from 88997f5 to 788c271 Compare October 31, 2024 23:50

kbukum1 added 3 commits October 31, 2024 17:03

fixed wrong constant variable

e686647

make all version numerics input lockfile DependecyFile name nillable

875a15c

changed spec according to change of default detected package manager …

e8eef8a

…as `npm`.

kbukum1 requested a review from abdulapopoola November 1, 2024 00:38

abdulapopoola reviewed Nov 1, 2024

View reviewed changes

kbukum1 added 3 commits November 4, 2024 08:41

simplified the condition

adc1457

Merge branch 'main' into kamil/update_npm_and_yarn_support_centralize…

05643f5

…d_package_manager

move strings into constants

3b9ad7c

kbukum1 force-pushed the kamil/update_npm_and_yarn_support_centralized_package_manager branch from 9754203 to 3b9ad7c Compare November 4, 2024 17:12

kbukum1 requested a review from abdulapopoola November 4, 2024 18:08

Merge branch 'main' into kamil/update_npm_and_yarn_support_centralize…

e4439e8

…d_package_manager

honeyankit approved these changes Nov 5, 2024

View reviewed changes

kbukum1 added 3 commits November 5, 2024 08:36

Merge branch 'main' into kamil/update_npm_and_yarn_support_centralize…

cba1cb6

…d_package_manager

fix for returning package manager name problem when checking engines …

0b3eb23

…attribute in manifest file

improve spec for package manager detector

345ad01

honeyankit approved these changes Nov 5, 2024

View reviewed changes

kbukum1 merged commit 2cb56bd into main Nov 5, 2024
86 checks passed

kbukum1 deleted the kamil/update_npm_and_yarn_support_centralized_package_manager branch November 5, 2024 20:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for centralized package manager abstraction for `npm_and_yarn` ecosystem #10862

Add support for centralized package manager abstraction for `npm_and_yarn` ecosystem #10862

kbukum1 commented Oct 28, 2024 •

edited

Loading

kbukum1 Oct 30, 2024

kbukum1 Oct 30, 2024

kbukum1 Oct 30, 2024

kbukum1 Oct 30, 2024 •

edited

Loading

kbukum1 Oct 30, 2024

kbukum1 Oct 30, 2024

kbukum1 Oct 30, 2024

kbukum1 Oct 30, 2024

abdulapopoola Oct 31, 2024

kbukum1 Oct 31, 2024

kbukum1 Oct 31, 2024

abdulapopoola Oct 31, 2024

kbukum1 Nov 1, 2024

kbukum1 Nov 1, 2024

abdulapopoola Oct 31, 2024

kbukum1 Nov 1, 2024

abdulapopoola Nov 1, 2024

kbukum1 Nov 4, 2024

abdulapopoola Oct 31, 2024

kbukum1 Nov 1, 2024

kbukum1 Nov 1, 2024

abdulapopoola Nov 1, 2024

kbukum1 Nov 4, 2024 •

edited

Loading

abdulapopoola Oct 31, 2024

kbukum1 Nov 1, 2024

abdulapopoola Nov 1, 2024

kbukum1 Nov 4, 2024

abdulapopoola Nov 1, 2024

kbukum1 Nov 4, 2024

kbukum1 commented Nov 4, 2024

honeyankit left a comment

honeyankit commented Nov 5, 2024

kbukum1 commented Nov 5, 2024

		def self.npm_version_numeric_npm6_or_higher(lockfile)
		lockfile_content = T.must(lockfile.content)

		lockfile_content = lockfile&.content

		return NPM_V8 if lockfile_content.nil? \|\| lockfile_content.strip.empty?

Add support for centralized package manager abstraction for npm_and_yarn ecosystem #10862

Add support for centralized package manager abstraction for npm_and_yarn ecosystem #10862

Conversation

kbukum1 commented Oct 28, 2024 • edited Loading

What are you trying to accomplish?

Anything you want to highlight for special attention from reviewers?

How will you know you've accomplished your goal?

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbukum1 Oct 30, 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

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

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

Choose a reason for hiding this comment

kbukum1 Nov 4, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbukum1 commented Nov 4, 2024

honeyankit left a comment

Choose a reason for hiding this comment

honeyankit commented Nov 5, 2024

kbukum1 commented Nov 5, 2024

Add support for centralized package manager abstraction for `npm_and_yarn` ecosystem #10862

Add support for centralized package manager abstraction for `npm_and_yarn` ecosystem #10862

kbukum1 commented Oct 28, 2024 •

edited

Loading

kbukum1 Oct 30, 2024 •

edited

Loading

kbukum1 Nov 4, 2024 •

edited

Loading