feat(npm): support pnpm catalogs #33376

fpapado · 2025-01-02T17:31:10Z

Changes

This PR adds support for pnpm catalogs, extracting dependencies from / updating dependencies in pnpm-workspace.yaml files.

I took @rarkins' suggestion in #30079 (comment), and added this as part of the npm manager. I tried to keep the functionality contained in pnpm files inside the relevant modules, to make it easier to split out in the future, if needed.

I took care of the following things:

pnpm-workspace.yaml files are picked up by default; their dependencies are extracted, and any updates are applied
Updates should preserve formatting as much as possible (tests back this up, but YAML is complicated)
pnpm-workspace.yaml files can be renamed and specified in fileMatch. The npm manager tries to parse a workspace YAML file first, before falling back to pakcage.json-like parsing.
The pnpmShrinkwrap file gets picked up, to use for updating. I believe I implemented that correctly, but I might have missed something.
Locked catalog dependencies are resolved from the lockfile. This should allow range replacements and other more advanced use-cases.

These questions have been resolved; I leave the text here for posterity

Open question 1: ways of picking up `pnpm-workspace.yaml` for extraction

I have placed pnpm-workspace.yaml in the fileMatch defaultConfig of the npm manager, and added an extra branch to extractAllPackageFiles.

An alternative would be to do this work in postExtract, by looking up sibling/parent pnpm-workspace.yaml file, and doing similar processing. This avoids the need for changing the config, but might have downsides that I cannot think of. What do you think?

Open question 2: `depType` and allowing granular updates to named catalogs

In the current version, I gave catalog-related upgrades a depType of pnpm.catalog, just for namespacing. However, I am wondering whether we could go more granular with this, such as pnpm.catalog.<catalogName>

One of the pnpm examples for catalogs, is allowing incremental updates, by defining named catalogs:

catalogs:
  # Can be referenced through "catalog:react17"
  react17:
    react: ^17.0.2
    react-dom: ^17.0.2

  # Can be referenced through "catalog:react18"
  react18:
    react: ^18.2.0
    react-dom: ^18.2.0

In such a scenario, I imagine users wanting to keep react 17 and 18 each in their range (allowing for bugfixes), without bumping the major. To do that, we would need to expose the catalog name to the user for matching, such as {matchDepTypes: ['pnpm.catalog.react17']}.

Have you had similar scenarios in the past? I noticed that pnpm.overrides kind of works like this, when using the pkg-a>pkg-b syntax (reference, example), but based on the depName. This could probably be done in a follow-up PR, but it's good to think about 💭

Context

Closes #30079

pnpm catalogs are described in the pnpm docs

This is my first Renovate contribution, I'm open to any and all ideas 😊

Documentation (please check one with an [x])

I have updated the documentation, or
[] No documentation update is required

I made a small documentation addition for the readme.md of the npm manager, in order to mention pnpm.catalogs.<name> as a depType.

How I've tested my work (please select one)

I have verified these changes via:

Code inspection only, or
Newly added/modified unit tests, or
No unit tests but ran on a real repository, or
Both unit tests + ran on a real repository

I added unit tests for the extraction and updates, the latter being a similar set to the package.json update tests. I will double-check the code coverage on CI, I think I missed something when running coverage locally.

I did a fair bit of repository testing at /~https://github.com/fpapado/renovate-testing-catalogs/pulls, to understand how the npm extraction/update phases work for package.json files.

lib/modules/manager/npm/update/dependency/pnpm.ts

lib/util/yaml.ts

fpapado · 2025-01-05T09:32:14Z

Ah, I noticed now in my test repo that the lockfile does not get updated after a standalone catalog dependency upgrade. I missed that codepath when trying to keep the functionality minimal. I'll add it to my todo 🗒️

Update: this has since been fixed 🎉

fpapado · 2025-01-09T08:13:32Z

@secustor and @viceice, apologies for the ping, do you have any pointers for the open questions here? If you think the PR is not in a reviewable state, I'm happy to implement with my gut feeling and re-request a review later down the line. I'm asking because I don't want to waste your time if something ends up being unworkable or unidiomatic, but I also don't want this to be stuck in limbo 😌

viceice · 2025-01-09T19:40:17Z

we usually don't review draft PRs, will try to have a look tomorrow

secustor

I had it on my plate, but haven't come around to finish it.

Open question 1: ways of picking up pnpm-workspace.yaml for extraction

pnpm-workspace.yaml should be part of fileMatch.
The expectation is that users will have use cases which deviating filenames e.g. because they are overwriting this name in a config.
Ideally we detect the fact that it is a workspace file via schema or other marker and are not relying on it. That way we are not bound to the filename and it "magically" works.

Open question 2: depType and allowing granular updates to named catalogs

Having pnpm.catalog.foo is preferable as with string pattern matching users can match all catalogs if needed.
Can you add an example of this in the readme.md that way it will show up in the manager docs.

lib/modules/manager/npm/extract/pnpm.ts

lib/modules/manager/npm/update/dependency/pnpm.spec.ts

lib/modules/manager/npm/update/dependency/pnpm.ts

lib/modules/manager/npm/update/dependency/common.ts

rarkins · 2025-01-10T12:38:30Z

@secuster can't we discover the pnpm-workspace.yaml file automatically instead of needing it in fileMatch?

secustor · 2025-01-10T13:10:54Z

@secuster can't we discover the pnpm-workspace.yaml file automatically instead of needing it in fileMatch?

If we are fine with only supporting this file name, we can drop it out of fileMatch.
There are requests regarding the possiblitly to change the file name and workarounds

rarkins · 2025-01-10T13:18:15Z

Ideally the name should be referenced from package.json. If not, I agree that we need to support it in fileMatch, but it's undesirable

lib/modules/manager/npm/extract/types.ts

lib/modules/manager/npm/update/dependency/pnpm.spec.ts

fpapado · 2025-01-14T20:24:50Z

lib/modules/manager/npm/post-update/index.ts

+        packageFile.path.endsWith('package.json') ||
+        packageFile.path.endsWith('pnpm-workspace.yaml')


The hardcoding here tripped me up, I kept not seeing the lockfiles changing 😅 Does this not beat some of the point of allowing pnpm-workspace.yaml (and package.json) being renamed via fileMatch? I wonder what the original context for the check was.

K, I think this will lead to too much changes in this PR. :/

Let's support pnpm-workspace.yaml and then support more names as needed.

Yeah, it turned out to be a bit complicated indeed 😅 Would you rather I keep pnpm-workspace.yaml in fileMatch still? It probably does not hurt to keep it there, just to avoid any more changes, but I'm happy to remove it as well.

fpapado · 2025-01-14T20:27:50Z

Thank you all kindly for your earler reviews! They helped clarify the direction quite a bit. I've made relevant changes now, tidied up the tests, implemented lockfile updating and locked version detection, and the coverage should be ok too.

I'll put this up for a review now. Let me know if you spot anything that can be improved, and I'll get to it 🔧

lib/modules/manager/npm/__fixtures__/lockfile-parsing/pnpm-lock-v9.yaml

lib/modules/manager/npm/extract/index.ts

lib/modules/manager/npm/extract/pnpm.spec.ts

lib/modules/manager/npm/extract/pnpm.ts

secustor

Sry, for the back and forth.

secustor · 2025-01-22T09:14:44Z

lib/modules/manager/npm/post-update/index.ts

+        packageFile.path.endsWith('package.json') ||
+        packageFile.path.endsWith('pnpm-workspace.yaml')


K, I think this will lead to too much changes in this PR. :/

Let's support pnpm-workspace.yaml and then support more names as needed.

lib/modules/manager/npm/extract/pnpm.spec.ts

Now that we use the CST for substitutions, we can accept inputs that do not end in newlines. Even though that is not per the yaml spec, it does mean that we leave any source files formatting intact.

It turns out that including managerData in one of the extraction deps, would cause the top-level (packageFile) managerData to be overwritten for the corresponding upgrade. In practice, this meant that {managerData: {pnpmShrinkWrap: ''}} was not found, and no lockfile update was triggered.

Adding pnpm-workspace.yaml to the default matchFiles means that a few of the assertions were failing.

…ockfile This ensures that the lockfile update command will see the updated workspace file, and will update correctly. There was a hardcoded package.json check, that was previously precluding that.

Co-authored-by: Sebastian Poxhofer <secustor@users.noreply.github.com>

This required a few more changes in the rest of the file, in order to mock fs and use the utils consistently.

This introduces a tryParsePnpmWorkspaceYaml function, which parses a string as YAML, and validates it via a schema, returning the result, and whether parsing succeeded. This allows the npm manager extraction call-site to only parse the content once, and pass on the parsed contents (if any) for extraction.

Previously, this was being tested _incidentally_, due to not mocking the filesystem calls.

fpapado commented Jan 2, 2025

View reviewed changes

lib/modules/manager/npm/update/dependency/pnpm.ts Outdated Show resolved Hide resolved

fpapado commented Jan 2, 2025

View reviewed changes

lib/util/yaml.ts Show resolved Hide resolved

fpapado marked this pull request as ready for review January 2, 2025 17:47

secustor self-requested a review January 2, 2025 21:18

viceice self-requested a review January 4, 2025 16:11

rarkins marked this pull request as draft January 8, 2025 12:27

secustor reviewed Jan 9, 2025

View reviewed changes

viceice reviewed Jan 11, 2025

View reviewed changes

lib/modules/manager/npm/extract/types.ts Outdated Show resolved Hide resolved

lib/modules/manager/npm/update/dependency/pnpm.spec.ts Outdated Show resolved Hide resolved

fpapado commented Jan 14, 2025

View reviewed changes

fpapado marked this pull request as ready for review January 14, 2025 20:27

fpapado requested review from secustor and viceice January 14, 2025 20:27

secustor reviewed Jan 17, 2025

View reviewed changes

fpapado requested a review from secustor January 19, 2025 18:12

secustor reviewed Jan 22, 2025

View reviewed changes

fpapado mentioned this pull request Jan 22, 2025

test(npm): use fs test utils in extract/pnpm.spec.ts #33756

Merged

6 tasks

fpapado added 7 commits January 22, 2025 18:54

WIP: prototype extraction in its own manager

c2fcb07

Move pnpm catalog extraction to npm manager

3455291

Remove pnpmCatalog from api.ts

86799c3

Implement updatePnpmCatalogDependency in updateDependency

0aab1ce

refactor: move updatePnpmCatalogDependency to its own module

f30a58a

refactor(npm): move git and npm alias resolution to common module

8c8726c

chore: small test and typechecking fixups

2b06e4b

fpapado and others added 29 commits January 22, 2025 18:54

chore: inline expectations in pnpm update spec

7bc9eb7

fix: use a CST for updating catalogs, in order to preserve formatting

2036a0c

fix: avoid double-parsing in update

379fd7b

test: shuffle update tests, remove yamlCodeBlock

a2f0e63

Now that we use the CST for substitutions, we can accept inputs that do not end in newlines. Even though that is not per the yaml spec, it does mean that we leave any source files formatting intact.

refactor: move PnpmCatalog interface to shared types.ts

365eb89

refactor: move pnpmCatalogsSchema to schema.ts

801b3ec

types: drop Partial from PnpmWorkspaceFile

056e118

feat: duck-type pnpm workspace files via schema, to allow renaming

4fdeb9d

feat: use a depName of pnpm.catalog.<name>, to allow pattern matching

2b2ddaa

docs: add mention of pnpm.catalog depName in npm readme.md

faf5c46

fix: ensure that pnpm-workspace.yaml duck-typing uses full schema

9000146

test: fix pnpm tests, by awaiting and by changing the object shape

1adcba2

test: fix tests that do assertions on the shape of npm manager config

57eacb6

Adding pnpm-workspace.yaml to the default matchFiles means that a few of the assertions were failing.

test: address coverage in lib/modules/manager/npm/update

1fc6200

chore: remove done TODO

aeae3f3

test: address coverage in lib/modules/manager/npm/extract/

75a7e46

feat: extract locked versions from catalogs

789135c

chore: remove done TODO

62cfbdf

test: tidy up coverage

41722c8

fix: ensure that pnpm-workspace.yaml artifacts get written prior to l…

2a4abb5

…ockfile This ensures that the lockfile update command will see the updated workspace file, and will update correctly. There was a hardcoded package.json check, that was previously precluding that.

test: add explicit tests for comments in pnpm workspace updates

8ec76f0

chore: remove errant logging

33fd424

Apply suggestions from code review

1bb24b9

Co-authored-by: Sebastian Poxhofer <secustor@users.noreply.github.com>

test: inline npm/__fixtures__/lockfile-parsing/pnpm-lock-v9.yaml

0d38875

test: use fs from test utils in npm/extract/pnpm.spec.ts

d4b2319

This required a few more changes in the rest of the file, in order to mock fs and use the utils consistently.

chore: add explicit type annotation inside pnpmCatalogsToArray

07bc33f

test: explicitly test lockfile acquisition in pnpm-workspace extraction

a0316a3

Previously, this was being tested _incidentally_, due to not mocking the filesystem calls.

fpapado force-pushed the feat/30079-support-pnpm-catalogs branch from 33b479c to a0316a3 Compare January 22, 2025 17:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(npm): support pnpm catalogs #33376

feat(npm): support pnpm catalogs #33376

fpapado commented Jan 2, 2025 •

edited

Loading

fpapado commented Jan 5, 2025 •

edited

Loading

fpapado commented Jan 9, 2025 •

edited

Loading

viceice commented Jan 9, 2025

secustor left a comment •

edited

Loading

rarkins commented Jan 10, 2025

secustor commented Jan 10, 2025

rarkins commented Jan 10, 2025

fpapado Jan 14, 2025

secustor Jan 22, 2025

fpapado Jan 22, 2025

fpapado commented Jan 14, 2025

secustor left a comment

secustor Jan 22, 2025

		packageFile.path.endsWith('package.json') \|\|
		packageFile.path.endsWith('pnpm-workspace.yaml')

feat(npm): support pnpm catalogs #33376

Are you sure you want to change the base?

feat(npm): support pnpm catalogs #33376

Conversation

fpapado commented Jan 2, 2025 • edited Loading

Changes

Open question 1: ways of picking up pnpm-workspace.yaml for extraction

Open question 2: depType and allowing granular updates to named catalogs

Context

Documentation (please check one with an [x])

How I've tested my work (please select one)

fpapado commented Jan 5, 2025 • edited Loading

fpapado commented Jan 9, 2025 • edited Loading

viceice commented Jan 9, 2025

secustor left a comment • edited Loading

Choose a reason for hiding this comment

rarkins commented Jan 10, 2025

secustor commented Jan 10, 2025

rarkins commented Jan 10, 2025

fpapado Jan 14, 2025

Choose a reason for hiding this comment

secustor Jan 22, 2025

Choose a reason for hiding this comment

fpapado Jan 22, 2025

Choose a reason for hiding this comment

fpapado commented Jan 14, 2025

secustor left a comment

Choose a reason for hiding this comment

secustor Jan 22, 2025

Choose a reason for hiding this comment

fpapado commented Jan 2, 2025 •

edited

Loading

Open question 1: ways of picking up `pnpm-workspace.yaml` for extraction

Open question 2: `depType` and allowing granular updates to named catalogs

fpapado commented Jan 5, 2025 •

edited

Loading

fpapado commented Jan 9, 2025 •

edited

Loading

secustor left a comment •

edited

Loading