feat: implement catalog protocol for install by gluxon · Pull Request #8221 · pnpm/pnpm

gluxon · 2024-06-17T18:28:15Z

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, {

Edit: We pivoted to resolveDependenciesOfImporters (which is still somewhere in the middle) after the discussion below. #8221 (comment)

Changes from Previous PR

This PR was previously open at #8020. A few things have changed since the last PR.

@zkochan changed --filter to update the pnpm-lock.yaml file first, which eliminates potential problems with the catalog: protocol resolving to stale values in pnpm-lock.yaml.
Reading the pnpm-workspace.yaml for catalogs is now done in @pnpm/config instead of @pnpm/context. feat: implement catalog protocol reads (feature flagged) #8020 (comment)

zkochan · 2024-06-17T19:59:03Z

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

I am not sure about this approach. That code is executed for every single resolved package, while catalogs are only applied to the projects themselves (not to the dependencies). So, maybe it should happen somewhere here

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

Lines 166 to 203 in 347c79a

    
           const resolveArgs: ImporterToResolve[] = importers.map((importer) => { 
        
             const projectSnapshot = opts.wantedLockfile.importers[importer.id] 
        
             // This may be optimized. 
        
             // We only need to proceed resolving every dependency 
        
             // if the newly added dependency has peer dependencies. 
        
             const proceed = importer.id === '.' || importer.hasRemovedDependencies === true || importer.wantedDependencies.some((wantedDep: any) => wantedDep.isNew) // eslint-disable-line @typescript-eslint/no-explicit-any 
        
             const resolveOpts: ImporterToResolveOptions = { 
        
               currentDepth: 0, 
        
               parentPkg: { 
        
                 installable: true, 
        
                 nodeId: importer.id as unknown as NodeId, 
        
                 optional: false, 
        
                 pkgId: importer.id as unknown as PkgResolutionId, 
        
                 rootDir: importer.rootDir, 
        
               }, 
        
               parentIds: [importer.id as unknown as PkgResolutionId], 
        
               proceed, 
        
               resolvedDependencies: { 
        
                 ...projectSnapshot.dependencies, 
        
                 ...projectSnapshot.devDependencies, 
        
                 ...projectSnapshot.optionalDependencies, 
        
               }, 
        
               updateDepth: -1, 
        
               updateMatching: importer.updateMatching, 
        
               prefix: importer.rootDir, 
        
               supportedArchitectures: opts.supportedArchitectures, 
        
               updateToLatest: opts.updateToLatest, 
        
             } 
        
             return { 
        
               updatePackageManifest: importer.updatePackageManifest, 
        
               parentPkgAliases: Object.fromEntries( 
        
                 importer.wantedDependencies.filter(({ alias }) => alias).map(({ alias }) => [alias, true]) 
        
               ) as ParentPkgAliases, 
        
               preferredVersions: importer.preferredVersions ?? {}, 
        
               wantedDependencies: importer.wantedDependencies, 
        
               options: resolveOpts, 
        
             } 
        
           })

Or in the resolveRootDependencies function, which won't be executed on dependencies.

gluxon · 2024-06-17T20:31:05Z

I am not sure about this approach. That code is executed for every single resolved package, while catalogs are only applied to the projects themselves (not to the dependencies). So, maybe it should happen somewhere here

I was also leaning towards resolveDependenciesOfImporters for a similar reason. The catalog: protocol only works on importers, so I thought made sense made sense there too.

Here's the call stack for reference. I generated this a few weeks ago when trying to think through where it made sense.

resolveDependencies (src/index.ts)
  resolveDependencyTree
    resolveRootDependencies
      resolveDependenciesOfImporters
        resolveDependenciesOfDependency
          resolveDependency
            resolveAndFetch - ctx.storeController.requestPackage
              createResolver() - default-resolver
      resolveDependencies (src/resolveDependencies.ts)

One other reason that I settled on resolveDependency is because this made it easy to populate the catalogMetadata field on the response of a specific dependency which is used to generate the catalogs section of the lock file.

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

Line 1251 in 3aca66d

catalogLookup,

I can try that on resolveDependenciesOfImporters instead, but a bit worried it might be come out messier than in resolveDependency.

I do think we should throw a clear error when the catalog: protocol is seen in an external dependency. Was going to work on that in a different PR.

gluxon · 2024-06-17T20:33:52Z

I'm recalling a big reason I put the replacement in resolveDependency is because that's where the existing wantedDependency.pref replacement was happening, so it made sense in my mind to keep those together.

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

Lines 1158 to 1163 in 4bf157f

    
           if (catalogLookup != null) { 
        
             wantedDependency.pref = catalogLookup.specifier 
        
           } 
        
           if (!options.update && currentPkg.version && currentPkg.pkgId?.endsWith(`@${currentPkg.version}`)) { 
        
             wantedDependency.pref = replaceVersionInPref(wantedDependency.pref, currentPkg.version) 
        
           }

zkochan · 2024-06-17T20:39:27Z

I'm recalling a big reason I put the replacement in resolveDependency is because that's where the existing wantedDependency.pref replacement was happening, so it made sense in my mind to keep those together.

That's a different thing though. That one is applied on all levels, while catalogs are only applied on importers.

I was also leaning towards resolveDependenciesOfImporters for a similar reason. The catalog: protocol only works on importers, so I thought made sense made sense there too.

That function sounds good as well.

gluxon · 2024-06-17T22:08:24Z

Thanks @zkochan. I have to head out for a few hours, but will update this PR with that improvement when I get back tonight. 👍

zkochan · 2024-06-20T14:38:28Z

Looks like compilation fails.

gluxon · 2024-06-20T14:44:54Z

Sorry about that — fixing now.

gluxon · 2024-06-20T16:14:58Z

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

+        })
+
+        if (catalogLookup != null) {
+          extendedWantedDep.wantedDependency.pref = catalogLookup.specifier


I don't love the mutation happening here, but following a similar pattern in resolveDependencies, which we were discussing a few days ago.

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

Line 1145 in 74c1057

wantedDependency.pref = replaceVersionInPref(wantedDependency.pref, currentPkg.version)

Let me know if we should recreate the extendedWantedDep object instead.

gluxon · 2024-06-20T16:15:25Z

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

+        // If the catalog protocol was used, store metadata about the catalog
+        // lookup to use in the lockfile.
+        if (result.resolveDependencyResult != null && catalogLookup != null) {
+          result.resolveDependencyResult.catalogLookup = catalogLookup


I similarly don't love the mutation here. Open to ideas on alternatives.

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

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

gluxon · 2024-06-23T17:14:28Z

Latest push removes the redundant newline and nested function.

Let me get back to you about simplifying getCatalogSnapshotsForResolvedDeps. Have to head out for a few hours but will be back to look at that soon today.

config/config/src/index.ts

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

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

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

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

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

…ting

gluxon · 2024-06-25T20:55:14Z

Latest push should address all feedback so far except:

Whether catalog config normalization should happen in @pnpm/config or @pnpm/workspace.read-manifest. I do lean towards keeping it in @pnpm/config, but open to more discussion on that. feat: implement catalog protocol for install #8221 (comment)

zkochan · 2024-06-25T21:50:30Z

ok, I'll probably check tomorrow.

zkochan · 2024-06-26T12:06:41Z

pkg-manager/core/test/catalogs.ts

+    const { allProjects } = await filterPackagesFromDir(
+      this.workspaceDir,
+      opts?.filter?.map(parentDir => ({ parentDir })) ?? [])


why do you need the filterPackagesFromDir function? All the data is already available. The tests in core are low level. You already passed all the required objects to preparePackages.

You can see in multipleImporters.ts tests how it is done. I think duplicating code in tests is fine. It is easier to understand. While helpers force to additionally learn how the helpers work.

merged accidentally. Still, filterPackagesFromDir is not needed as a new dev dep in core

Got it. I'll remove this from core in a PR now.

why do you need the filterPackagesFromDir function? All the data is already available. The tests in core are low level. You already passed all the required objects to preparePackages.

I wanted something that re-read the manifests off of disk after the calling the updateProjectManifest method. I also wanted to avoid other people having to edit this test if something about the Project interface changes.

You can see in multipleImporters.ts tests how it is done. I think duplicating code in tests is fine. It is easier to understand. While helpers force to additionally learn how the helpers work.

I agree with this principle. Sometimes helpers do make code harder to understand.

refactor: remove filter-packages-from-dir dependency from core tests #8244

* feat: add catalogs to @pnpm/config * refactor: factor out resolveDependenciesOfImporterDependency function * feat: implement catalog resolver and replace prefs * revert: work around CI setup not compiling orphan packages before testing * feat: record catalog lookup snapshots through propagated metadata * feat: update projects when catalogs config changes * test: add catalog protocol install tests

* feat: create new @pnpm/catalogs.types package (#8026) * feat: read catalog configs from workspace manifest (#8123) * refactor: move InvalidWorkspaceManifestError to its own file * feat: read catalogs config from workspace manifest * chore: add changeset for new catalog config parsing * feat: create new `@pnpm/catalogs.protocol-parser` package (#8124) This works around a problem with pnpm's CI setup not compiling packages that are not dependencies of the main pnpm package before running these tests. #8027 (comment) * refactor: factor out isWantedDepPrefSame to extend in a future commit (#8125) * feat: create new `@pnpm/catalogs.config` package (#8220) * refactor: remove single default catalog check This check will happen in `@pnpm/catalogs.config` instead. * feat: create new @pnpm/catalogs.config package * fix: work around CI setup not compiling orphan packages before testing This works around a problem with pnpm's CI setup not compiling packages that are not dependencies of the main pnpm package before running these tests. #8027 (comment) * feat: create new `@pnpm/catalogs.resolver` package (#8219) * feat: create new @pnpm/catalogs.resolver package * fix: work around CI setup not compiling orphan packages before testing This works around a problem with pnpm's CI setup not compiling packages that are not dependencies of the main pnpm package before running these tests. #8027 (comment) * feat: implement catalog protocol for publish (#8225) * feat: implement catalog protocol for install (#8221) * feat: add catalogs to @pnpm/config * refactor: factor out resolveDependenciesOfImporterDependency function * feat: implement catalog resolver and replace prefs * revert: work around CI setup not compiling orphan packages before testing * feat: record catalog lookup snapshots through propagated metadata * feat: update projects when catalogs config changes * test: add catalog protocol install tests * refactor: remove filter-packages-from-dir dependency from core tests (#8244) * refactor: remove filter-packages-from-dir dependency from core tests * test: refactor * test: refactor --------- Co-authored-by: Zoltan Kochan <z@kochan.io> --------- Co-authored-by: Zoltan Kochan <z@kochan.io>

gluxon marked this pull request as ready for review June 17, 2024 19:36

gluxon requested a review from zkochan as a code owner June 17, 2024 19:36

gluxon force-pushed the gluxon/catalog-protocol-install branch 3 times, most recently from 8fc37e4 to 4bf157f Compare June 17, 2024 20:12

gluxon force-pushed the gluxon/catalog-protocol-install branch from 4bf157f to 8db6388 Compare June 20, 2024 08:16

gluxon force-pushed the gluxon/catalog-protocol-install branch 3 times, most recently from 32c090d to 80a9036 Compare June 20, 2024 16:12

gluxon commented Jun 20, 2024

View reviewed changes

gluxon force-pushed the gluxon/catalog-protocol-install branch 2 times, most recently from 93143aa to 80a9036 Compare June 21, 2024 04:02

zkochan reviewed Jun 23, 2024

View reviewed changes

pkg-manager/resolve-dependencies/src/resolveDependencies.ts Outdated Show resolved Hide resolved

pkg-manager/resolve-dependencies/src/resolveDependencies.ts Outdated Show resolved Hide resolved

pkg-manager/resolve-dependencies/src/getCatalogSnapshots.ts Outdated Show resolved Hide resolved

gluxon force-pushed the gluxon/catalog-protocol-install branch from 80a9036 to e6a09b8 Compare June 23, 2024 17:12

gluxon force-pushed the gluxon/catalog-protocol-install branch from e6a09b8 to d348749 Compare June 23, 2024 17:17

yyx990803 mentioned this pull request Jun 24, 2024

Catalogs Tracking Issue #7072

Open

18 tasks

zkochan reviewed Jun 24, 2024

View reviewed changes

gluxon force-pushed the gluxon/catalog-protocol-install branch from 3141668 to 05993a8 Compare June 25, 2024 07:48

gluxon added 4 commits June 25, 2024 13:51

feat: add catalogs to @pnpm/config

f1d82eb

refactor: factor out resolveDependenciesOfImporterDependency function

e5bbd81

feat: implement catalog resolver and replace prefs

c08bbc7

revert: work around CI setup not compiling orphan packages before tes…

1e56cf2

…ting

gluxon added 3 commits June 25, 2024 13:51

feat: record catalog lookup snapshots through propagated metadata

b02e1c6

feat: update projects when catalogs config changes

ced8288

test: add catalog protocol install tests

0a65b30

gluxon force-pushed the gluxon/catalog-protocol-install branch from 05993a8 to 0a65b30 Compare June 25, 2024 20:52

zkochan reviewed Jun 26, 2024

View reviewed changes

zkochan approved these changes Jun 26, 2024

View reviewed changes

zkochan merged commit 413721e into catalogs Jun 26, 2024

zkochan deleted the gluxon/catalog-protocol-install branch June 26, 2024 13:50

gluxon mentioned this pull request Jun 26, 2024

feat: catalogs #8122

Merged

gluxon mentioned this pull request Jun 27, 2024

refactor: remove filter-packages-from-dir dependency from core tests #8244

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, {

Uh oh!

Conversation

gluxon commented Jun 17, 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?

Changes from Previous PR

Uh oh!

zkochan commented Jun 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gluxon commented Jun 17, 2024

Uh oh!

gluxon commented Jun 17, 2024

Uh oh!

zkochan commented Jun 17, 2024

Uh oh!

gluxon commented Jun 17, 2024

Uh oh!

zkochan commented Jun 20, 2024

Uh oh!

gluxon commented Jun 20, 2024

Uh oh!

gluxon Jun 20, 2024

Choose a reason for hiding this comment

Uh oh!

gluxon Jun 20, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gluxon commented Jun 23, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gluxon commented Jun 25, 2024

Uh oh!

zkochan commented Jun 25, 2024

Uh oh!

zkochan Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

zkochan Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

gluxon Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

gluxon Jun 27, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gluxon commented Jun 17, 2024 •

edited

Loading

zkochan commented Jun 17, 2024 •

edited

Loading

gluxon Jun 27, 2024 •

edited

Loading