feat: implement catalog protocol reads (feature flagged) by gluxon · Pull Request #8020 · pnpm/pnpm

gluxon · 2024-04-26T07:43:24Z

Changes

This implements the following items from the tracking issue #7072.

Support the catalog: and catalog:default specifiers.
Support catalog:<name> specifiers.
Changing a catalog entry in pnpm-workspace.yaml causes the lock file to not be up to date.

I've organized this PR into different commits that make sense by themselves. This PR should be easier to review commit by commit rather than all files changed at once.

When should we transform the catalog protocol?

The hardest part of this PR was determining the right place for the catalog: → ^1.2.3 translation internally.

We could model this as a read package hook, which would be similar to how pnpm.overrides and packageExtensions work. I think this is too early of a place for the protocol to be resolved since it means the pnpm-lock.yaml file won't have the original catalog: specifier that was declared in package.json.
We could rewrite it at the @pnpm/default-resolver level, but I think that's too deep. It means the default resolver would need to understand catalogs, which leaks the catalogs abstraction and makes the resolvers more complicated.

I settled on performing the catalog: protocol replacement right before we request dependencies from the store. I think that strikes the right balance.

pnpm/pkg-manager/resolve-dependencies/src/resolveDependencies.ts

Lines 1135 to 1141 in ade62a9

    
           if (catalogLookup != null) { 
        
             wantedDependency.pref = catalogLookup.entrySpecifier 
        
           } 
        
           if (!options.update && currentPkg.version && currentPkg.pkgId?.endsWith(`@${currentPkg.version}`)) { 
        
             wantedDependency.pref = replaceVersionInPref(wantedDependency.pref, currentPkg.version) 
        
           } 
        
           pkgResponse = await ctx.storeController.requestPackage(wantedDependency, {

gluxon · 2024-04-26T20:29:00Z

catalogs/types/README.md

@@ -0,0 +1,3 @@
+# @pnpm/catalogs.types
+
+> Types related to the pnpm catalogs feature.


Instead of creating a new @pnpm/catalogs.types package, we could also stuff the small definitions here into the existing @pnpm/types package. I opted for a new package for now since we're doing something similar for hooks: @pnpm/hooks.types.

sounds good to me

Thanks. I'll separate the new packages out to a different PR so you don't have to keep looking at these changes when re-reviewing this PR.

feat: create new @pnpm/catalogs.types package #8026

feat: create new @pnpm/catalogs.protocol-parser package for parsing catalog specifiers #8027

gluxon · 2024-04-26T20:34:08Z

catalogs/protocol-parser/README.md

@@ -0,0 +1,3 @@
+# @pnpm/catalogs.protocol-parser


At the moment, this small package gets used in:

pkg-manager/core/src/install/index.ts

pkg-manager/resolve-dependencies/src/getCatalogResolver.ts

Thought it was worth breaking it out into a new package. The publishing packages will likely need this utility too in a future PR.

gluxon · 2024-04-26T21:12:57Z

lockfile/lockfile-types/src/index.ts

+  /**
+   * The concrete version that the requested specifier resolved to. Ex: 1.2.3
+   */
+  readonly version: string


In the pnpm-lock.yaml file, this will look like:

catalogs: default: foo: specifier: ^1.2.3 version: 1.2.3

This PR writes the version part. A future PR will read from it to optimize new usages of the catalog protocol. I think the logic will be similar to the existing replaceVersionInPref logic.

what if there will be multiple named catalogs with the same specs? will the entries be duplicated? Like

catalogs: default: foo: specifier: ^1.2.3 version: 1.2.3 named: foo: specifier: ^1.2.3 version: 1.2.3

Yup, they'll be duplicated. I think that's okay and somewhat by design if someone duplicates a catalog entry across multiple named catalogs.

gluxon · 2024-04-26T21:15:50Z

pkg-manager/plugin-commands-installation/test/catalog.ts

+  })
+})
+
+test('lockfile catalog snapshots should keep unused entries', async () => {


Intentionally keeping around unused entries for now. Otherwise we'd wipe out catalog: dependencies in a filtered install. I think we'll want to change this logic in another PR to prevent stale references though.

zkochan · 2024-04-27T20:08:39Z

catalogs/types/README.md

@@ -0,0 +1,3 @@
+# @pnpm/catalogs.types
+
+> Types related to the pnpm catalogs feature.


sounds good to me

zkochan · 2024-04-27T20:15:51Z

lockfile/lockfile-types/src/index.ts

+  /**
+   * The concrete version that the requested specifier resolved to. Ex: 1.2.3
+   */
+  readonly version: string


what if there will be multiple named catalogs with the same specs? will the entries be duplicated? Like

catalogs: default: foo: specifier: ^1.2.3 version: 1.2.3 named: foo: specifier: ^1.2.3 version: 1.2.3

zkochan · 2024-04-27T20:17:04Z

pkg-manager/core/src/install/extendInstallOptions.ts

+
+  useBetaCatalogsFeat?: boolean


Is there a need for a setting? It is opt-in anyway, as the new field in pnpm-workspace.yaml should be added.

It's mostly to avoid bug reports that catalogs aren't working on the next version (ex: 9.1.0). I was thinking it'd be better for it to not work entirely rather than partially work.

Without the setting here, the catalog: protocol would work when defined pnpm-workspace.yaml, but we wouldn't replace it on publish. I'm working on the publish replacing part next.

Alternatively we could:

Use an environment variable to feature flag this.

Avoid merging any catalog PRs until it's completely finished.

I thought the temporaryuseBetaCatalogsFeat argument was the best out of those 3 options, but open to other thoughts!

zkochan · 2024-04-27T20:25:27Z

pkg-manager/get-context/src/index.ts

  const hoistPattern = importersContext.currentHoistPattern ?? opts.hoistPattern
+
+  const [catalogs, lockfiles] = await Promise.all([
+    readCatalogsFromWorkspaceManifest(opts.lockfileDir, opts.useBetaCatalogsFeat ?? false),


the workspace manifest is already read outside of @pnpm/core. The catalogs should be passed to core through options.

Yeah that's a better idea.

Spent some time thinking through the best way to do this. It's a bit complicated, which is why I avoided it in this PR. But I do think it's a good idea to de-duplicate pnpm-workspace.yaml reads. I was planning on doing it after this PR, but it's probably better to do it beforehand.

Here's our only usage of readWorkspaceManifest in pnpm:

pnpm/workspace/find-packages/src/index.ts

Lines 39 to 44 in 9719a42

export async function findWorkspacePackagesNoCheck (workspaceRoot: string, opts?: { patterns?: string[] }): Promise<Project[]> {

let patterns = opts?.patterns

if (patterns == null) {

const workspaceManifest = await readWorkspaceManifest(workspaceRoot)

patterns = workspaceManifest?.packages

}

A few approaches to refactor that:

We could read WorkspaceManifest higher up and pass it to findWorkspacePackages. This would involve refactoring all the installation commands: add, install, import, etc that are currently calling findWorkspacePackages in each of its handlers.

We could create a new readWorkspaceManifestCached option that findWorkspacePackages uses.

I prefer option 1 since 2 requires a longer lived module-level cache, but option 1 involves much more refactoring.

@zkochan I can do approach 1 in a separate PR if that sounds good?

approach 1 is the right one. Maybe we can read it in @pnpm/config. We already read the root manifest there too.

Thanks! Next part here starting this here:

feat(find-workspace-packages): accept cached workspaceManifest option #8120

zkochan · 2024-04-27T20:35:42Z

pkg-manager/plugin-commands-installation/test/catalog.ts

+        specifier: 'catalog:',
+        version: '1.0.0',


if the version is also present in the catalogs object, then why is it duplicated here? I guess it makes sense to duplicated it in case when is-positive has peer dependencies. Probably should have such test when the same catalog is used in two projects and has peers that resolve to different versions.

Right, it's the peer dependencies part of this. Will do — I'll add a test for that.

zkochan · 2024-04-27T20:37:31Z

pkg-manager/resolve-dependencies/src/getCatalogResolver.ts

+export function getCatalogResolver (catalogs: Catalogs): CatalogResolver {
+  return function resolveFromCatalog (wantedDependency: WantedDependency): CatalogResolution | undefined {


no need to use a nested function. Just use bind.

Suggested change

export function getCatalogResolver (catalogs: Catalogs): CatalogResolver {

return function resolveFromCatalog (wantedDependency: WantedDependency): CatalogResolution | undefined {

export function resolveFromCatalog (catalogs: Catalogs, wantedDependency: WantedDependency): CatalogResolution | undefined {

gluxon · 2024-04-28T05:27:49Z

Thanks for the review. I'll move to draft while I work on the feedback.

zkochan · 2024-04-30T09:51:35Z

In your PR you mention several issues with filtered install. I think we need to change how filtering works as we have many issues with it now. If the lockfile is not up-to-date, when a filtered install happens, we should do a non-filtered full resolution only install first. This will be less terrible in pnpm v9 as in v9 pnpm doesn't download the package tarballs during resolution.

gluxon · 2024-05-23T23:02:50Z

In your PR you mention several issues with filtered install. I think we need to change how filtering works as we have many issues with it now. If the lockfile is not up-to-date, when a filtered install happens, we should do a non-filtered full resolution only install first. This will be less terrible in pnpm v9 as in v9 pnpm doesn't download the package tarballs during resolution.

I like this idea. It should also help with the folks confused by how --filter works when dedupe-peer-dependents is enabled.

I'm going to reopen this PR without the feature flag since we're merging into a feature branch.

gluxon force-pushed the catalogs branch from 0dc4ca9 to 09486ab Compare April 26, 2024 07:44

gluxon mentioned this pull request Apr 26, 2024

feat: read catalog configs from workspace manifest (feature flagged) #8019

Merged

gluxon changed the title ~~feat: implement catalog protocl reads (feature flagged)~~ feat: implement catalog protocol reads (feature flagged) Apr 26, 2024

feat: create new @pnpm/catalogs.types package

43a173c

gluxon force-pushed the catalogs branch 2 times, most recently from c3c6b50 to f7d198f Compare April 26, 2024 20:18

gluxon added 9 commits April 26, 2024 16:30

feat: create new @pnpm/catalogs.protocol-parser package

befc765

feat: implement catalog protocol parser

bd7df76

feat: define feature flag to enable catalogs in tests

d575cbc

feat: read and store catalogs config in PnpmContext

5dee13d

feat: implement catalog resolver and replace prefs

ec7dfb2

feat: record catalog lookup snapshots through propagated metadata

4fee31d

refactor: factor out isWantedDepPrefSame to extend in a future commit

07a2e87

feat: update projects when catalogs config changes

ca6b6f8

test: add catalog protocol tests

93d5599

gluxon force-pushed the catalogs branch from f7d198f to 93d5599 Compare April 26, 2024 20:30

gluxon commented Apr 26, 2024

View reviewed changes

gluxon marked this pull request as ready for review April 26, 2024 21:16

gluxon requested a review from zkochan as a code owner April 26, 2024 21:16

zkochan reviewed Apr 27, 2024

View reviewed changes

gluxon marked this pull request as draft April 28, 2024 05:27

This was referenced Apr 28, 2024

feat: create new @pnpm/catalogs.types package #8026

Merged

feat: create new @pnpm/catalogs.protocol-parser package for parsing catalog specifiers #8027

Closed

zkochan changed the base branch from main to catalogs April 29, 2024 13:31

gluxon mentioned this pull request May 23, 2024

feat(find-workspace-packages): accept cached workspaceManifest option #8120

Closed

gluxon deleted the branch pnpm:catalogs May 23, 2024 22:58

gluxon closed this May 23, 2024

This was referenced May 23, 2024

feat: create new @pnpm/catalogs.protocol-parser package #8124

Merged

refactor: factor out isWantedDepPrefSame to extend in a future commit #8125

Merged

gluxon deleted the catalogs branch May 24, 2024 18:50

zkochan mentioned this pull request Jun 3, 2024

Filtered install should work only on an up-to-date lockfile #8165

Closed

jakebailey mentioned this pull request Jun 12, 2024

fix: a filtered install should update the lockfile first #8183

Merged

This was referenced Jun 16, 2024

refactor: store workspace manifest in @pnpm/config for reuse #8213

Merged

feat: implement catalog protocol for install #8221

Merged

	if (catalogLookup != null) {
	wantedDependency.pref = catalogLookup.entrySpecifier
	}
	if (!options.update && currentPkg.version && currentPkg.pkgId?.endsWith(`@${currentPkg.version}`)) {
	wantedDependency.pref = replaceVersionInPref(wantedDependency.pref, currentPkg.version)
	}
	pkgResponse = await ctx.storeController.requestPackage(wantedDependency, {

		@@ -0,0 +1,3 @@
		# @pnpm/catalogs.types

		> Types related to the pnpm catalogs feature.

	export async function findWorkspacePackagesNoCheck (workspaceRoot: string, opts?: { patterns?: string[] }): Promise<Project[]> {
	let patterns = opts?.patterns
	if (patterns == null) {
	const workspaceManifest = await readWorkspaceManifest(workspaceRoot)
	patterns = workspaceManifest?.packages
	}

		export function getCatalogResolver (catalogs: Catalogs): CatalogResolver {
		return function resolveFromCatalog (wantedDependency: WantedDependency): CatalogResolution \| undefined {

	export function getCatalogResolver (catalogs: Catalogs): CatalogResolver {
	return function resolveFromCatalog (wantedDependency: WantedDependency): CatalogResolution \| undefined {
	export function resolveFromCatalog (catalogs: Catalogs, wantedDependency: WantedDependency): CatalogResolution \| undefined {

Uh oh!

Conversation

gluxon commented Apr 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

When should we transform the catalog protocol?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gluxon Apr 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gluxon Apr 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gluxon commented Apr 28, 2024

Uh oh!

zkochan commented Apr 30, 2024

Uh oh!

gluxon commented May 23, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gluxon commented Apr 26, 2024 •

edited

Loading

gluxon Apr 28, 2024 •

edited

Loading

gluxon Apr 28, 2024 •

edited

Loading