Skip to content

feat: pnpm add option to add new entries to catalogs #9484

Merged
zkochan merged 74 commits intomainfrom
save-catalog-9425
May 14, 2025
Merged

feat: pnpm add option to add new entries to catalogs #9484
zkochan merged 74 commits intomainfrom
save-catalog-9425

Conversation

@KSXGitHub
Copy link
Contributor

@KSXGitHub KSXGitHub commented May 4, 2025

Resolves #9425


TODO:

  • Save new entries as catalog: in package.json.
  • Add new catalogs to pnpm-workspace.yaml.
  • Create pnpm-workspace.yaml.
  • Fix catalogs not appearing in the lockfile:
    newLockfile.catalogs = getCatalogSnapshots(Object.values(resolvedImporters).flatMap(({ directDependencies }) => directDependencies))
    • Uncomment disabled tests.
  • Don't overwrite existing catalogs.
  • Detect and remove duplicated writes to pnpm-workspace.yaml: There is 1 unused (possibly because I didn't create the right condition to trigger it) but there is no duplication.
  • Test
    • pnpm add <pkg>.
    • pnpm add <pkg>@<range>.
    • pnpm add <pkg>@<git>.
    • pnpm add <pkg>@jsr:*.
    • pnpm add <local-pkg> (should not save as catalogs).
    • addDefaultCatalogs.
    • Doesn't overwrite existing catalogs.
    • Combining with manually editing package.json.
    • Shared lockfile workspace.
    • Multiple lockfiles workspace.
    • Non workspace.
  • Changeset.

@KSXGitHub KSXGitHub marked this pull request as ready for review May 13, 2025 19:55
@KSXGitHub KSXGitHub requested review from gluxon and zkochan May 13, 2025 19:59
@zkochan
Copy link
Member

zkochan commented May 14, 2025

So I did not think it through. We cannot make --save-catalog both a boolean and a string. Because we can't tell if pnpm add --save-catalog lodash ramda should install lodash as a dependency or use the lodash catalog name.

We need a separate flag for specifying the catalog name. Maybe --catalog-name=<name>?

Alternatively, we could use --default-catalog instead of --save-catalog but that kind of goes against the conventions we have (like --save-dev).

@KSXGitHub
Copy link
Contributor Author

So I did not think it through. We cannot make --save-catalog both a boolean and a string. Because we can't tell if pnpm add --save-catalog lodash ramda should install lodash as a dependency or use the lodash catalog name.

--save-catalog lodash ramda should be the same as --save-catalog=default lodash ramda, which is different from --save-catalog=lodash ramda.

In other words, we can have --save-catalog=foo and --save-catalog foo act differently.

Do you think there is a problem with this?

@zkochan
Copy link
Member

zkochan commented May 14, 2025

In other words, we can have --save-catalog=foo and --save-catalog foo act differently.

Do you think there is a problem with this?

I don't think we should allow this because currently the = sign is optional for flags. So, it will introduce confusion.

@zkochan
Copy link
Member

zkochan commented May 14, 2025

I think --save-catalog-name should be fine. It probably won't be frequently used.

@zkochan zkochan requested a review from Copilot May 14, 2025 14:37
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements a new CLI option to add catalog entries when installing packages with pnpm. It integrates a new saveCatalog/saveCatalogName option throughout the dependency resolution, installation, and workspace manifest updates, along with corresponding tests and documentation updates.

  • Introduced a new flag to save dependencies as catalog entries in package.json and pnpm-workspace.yaml.
  • Updated dependency resolution and installation functions to merge new catalog data.
  • Added tests and configuration updates to support the new catalog functionality.

Reviewed Changes

Copilot reviewed 25 out of 26 changed files in this pull request and generated no comments.

Show a summary per file
File Description
pkg-manager/resolve-dependencies/src/resolveDependencyTree.ts Updated options and added newCatalogs integration for catalogs.
pkg-manager/resolve-dependencies/src/resolveDependencies.ts Propagated and merged new catalog data into the lockfile.
pkg-manager/plugin-commands-installation/test/saveCatalog.ts Added tests for verifying catalog saving behavior.
pkg-manager/plugin-commands-installation/src/* Updated CLI options and docs to support the new catalog feature.
pkg-manager/core/src/* Updated dependency parsing and installation methods to include saveCatalog.
config/config/src/* Added new configuration for save-catalog-name.
.changeset/tangy-nails-hide.md Documented the new CLI options and module version bumps.
Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)

pkg-manager/resolve-dependencies/src/resolveDependencyTree.ts:122

  • [nitpick] Consider aligning the naming for the catalog saving option across the codebase, as some modules use 'saveCatalog' while others use 'saveCatalogName'. For clarity, choose a single consistent name.
saveCatalog?: string

@zkochan zkochan merged commit c8341cc into main May 14, 2025
16 of 20 checks passed
@zkochan zkochan deleted the save-catalog-9425 branch May 14, 2025 16:32
gluxon added a commit that referenced this pull request Jun 6, 2025
gluxon added a commit that referenced this pull request Jun 7, 2025
gluxon added a commit that referenced this pull request Jun 7, 2025
gluxon added a commit that referenced this pull request Jun 8, 2025
@sebastienbarre
Copy link

Alternatively, I wouldn't mind just relying on the codemod to find any 2 packages with the same dependency and move said dependency to catalog. This is something I would run periodically for example. Unfortunately, it doesn't behave that way at the moment, see fix: do not move packages that are not shared to catalog by mcous · Pull Request #6 · pnpm/codemod/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add option to save to catalog

6 participants