Enable CorePack Installing Package Managers from Private Registries

[kbukum1]

What are you trying to accomplish?

This PR addresses an issue where Corepack was unable to install a package manager from private registries. The problem was encountered in a private repository, and the proposed solution ensures that the correct registry and authentication token are provided for Corepack to successfully install the package manager.

By creating a RegistryHelper class and enabling it to parse and provide the required environment variables (COREPACK_NPM_REGISTRY and COREPACK_NPM_TOKEN), this change fixes the issue and adds support for private registries.

Anything you want to highlight for special attention from reviewers?

The following changes were implemented:

• Refactored RegistryHelper from a module to a class for better encapsulation.
• Added logic to prioritize dependabot.yml credentials, .npmrc, .yarnrc, yarnrc.yml, and lockfiles when determining the registry and authentication token.
• Ensured the implementation is compatible with Sorbet's strict type checking.
• Updated tests to verify the ability to set Corepack environment variables dynamically for private registries.

How will you know you've accomplished your goal?

The following steps demonstrate the success of this change:

• Verified Corepack can now successfully install package managers from private registries when the appropriate dependabot.yml credentials, .npmrc, .yarnrc, yarnrc.yml, or lockfile configurations are provided.
• Added tests to confirm registry and token are correctly parsed and passed to Corepack as environment variables.
• Reproduced the issue in a private repository and confirmed the fix resolves the error.

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]

Related issue:

• NPM Install from a private registry #11038

[jonabc]

The logic looks ok, but I'm not sure about adding another class and another level of code indirection to this ecosystem

[jonabc]

it's not entirely clear why this is its own class when it's calls are already fully encapsulated within PackageManagerHelper

If this is meant to be reused in other places, WDYT about providing a RegistryHelper instance to PackageManagerHelper.new instead of creating it in the constructor?

Otherwise if it's not reused elsewhere could this be collapsed into PackageManagerHelper? This could always be broken out into a separate class later if/when needed for reuse or portability.

[kbukum1]

can be moved into PackageManagerHelper

[kbukum1]

It is just moved.

[jakecoffman]

.yarnrc is usually space delimited https://github.com/search?q=path%3A.yarnrc&type=code

[kbukum1]

Checking

[kbukum1]

fixed.

[jakecoffman]

Not every NPM registry is also a reverse-proxy style registry, for instance npm.pkg.github.com which is usually scoped to a specific package. So if a customer had a private scoped package on a registry that wasn't a reverse-proxy, and they used dependabot.yml to provide credentials, corepack would try to download from there which would fail.

It might be better to look for replaces-base only which indicates it's a reverse-proxy style registry.

[kbukum1]

I’ve updated the code to ensure that the config_npm_registry_and_token method only returns the object if the registry is reverse-proxy-enabled (i.e., when replaces-base is true). Could you please confirm if this aligns with what you meant in your comment?

[jonabc]

Sorry if my previous comment was confusing, I was not referring to the location of the class. What I meant to ask was why this needed to be a standalone class at all?

The class contains two instance variables which are also known by PackageManagerHelper, and all other logic is entirely functional in that there are no changes to stored state. The class is also instantiated in one place only - the constructor of PackageManagerHelper.

The functional nature of the class combined with it's highly scoped usage makes me think that one of two things should be going on here:

1. If the registry helper is meant to be reusable outside of PackageManagerHelper then it should be instantiated outside of PackageManagerHelper and passed in as an argument. In this way the PackageManagerHelper is entirely agnostic to the idea of registry credentials and configurations

e.g. PackageManagerHelper.new(package_json, lockfiles, registry_helper)

2. If the RegistryHelper is only ever going to be used within the code of PackageManagerHelper, then RegistryHelper should not be a standalone class. If you want to isolate this logic, you could easily make the class into a module that gets included into PackageManagerHelper

e.g.

module RegistryHelper
  # expect anything including this helper to have accesors for registry_config_files and credentials
  ... 
end

class PackageManagerHelper
  include RegistryHelper
  attr_reader :registry_config_files, :credentials
    
  def initialize(package_json, lockfiles, registry_config_files, credentials)
    ...
    @registry_config_files = registry_config_files
    @credentials = credentials
    ...
  end
end

[kbukum1]

Thank you for the feedback! The reason I structured it this way is that the long-term goal is to centralize all registry-related logic within the RegistryHelper class. This approach allows us to handle all registry-related operations in one place, making it easier for others to extend or modify the logic in the future, should the need arise for additional registry-related functionality.

By encapsulating the related constants, variables, and logic in a dedicated class, I’m aiming to keep the code clean, modular, and reusable. Using a module would require passing parameters to each function or tightly coupling the logic to PackageManagerHelper, which I believe would reduce flexibility and reusability.

I hope this clarifies my rationale, and I’m happy to discuss further if needed!

[kbukum1]

I can pass registry helper as a object into package manager helper. That can be another way of using it but I am also using similar parameters such as lockfiles. So that's why I initiated the registry helper in the package manager helper.

[jakecoffman]

I suspect setting COREPACK_NPM_TOKEN_ENV is not necessary as the Proxy container will inject the needed credentials. But there may be folks out there still using the Ruby directly and handing it tokens. So fine to leave for now.

[jakecoffman]

Functionality seems good though I don't like the amount of parsing we need to do to extract the registry. Seems like a source of bugs in the future. Perhaps this will allow us to delete all this code some day: nodejs/corepack#540

[jonabc]

I'm not a fan of pre-optimizing code for unknown future usage, but if the solution works and @jakecoffman has no other feedback then I wouldn't block this moving forward