fix: `pnpm outdated` should work with `catalog:` protocol by gluxon · Pull Request #8304 · pnpm/pnpm

gluxon · 2024-07-12T05:16:53Z

Problem

Dependencies using the catalog: protocol were accidentally skipped over in pnpm outdated.

For example, cd pkg-manager/core && pnpm outdated in this repo shows no output:

Changes

The pnpm outdated and pnpm outdated --recursive commands should now work as expected when using catalogs.

gluxon · 2024-07-14T01:56:41Z

reviewing/plugin-commands-outdated/test/index.ts

 const hasMajorOutdatedDepsFixture = f.find('has-major-outdated-deps')
 const hasNoLockfileFixture = f.find('has-no-lockfile')
 const withPnpmUpdateIgnore = f.find('with-pnpm-update-ignore')
+const hasOutdatedDepsUsingCatalogProtocol = f.find('has-outdated-deps-using-catalog-protocol')


I think we should use preparePackage for these tests?

I setup an in-repo fixture since the other tests in plugin-commands-outdated followed that structure, but I personally prefer programmatically creating the test fixture in a temp dir, which preparePackage does.

I have no objections to dynamically generate the fixture with preparePackage

I'm realizing why the plugin-commands-outdated tests all rely on fixtures. After I tried to use preparePackage, I hit this error:

To create a pnpm-lock.yaml, we could import @pnpm/core here and run mutateModules, but I don't see any other tests in the pnpm repo doing that other than the tests in @pnpm/core itself. We'd also need to copy pkg-manager/core/test/utils/testDefaults.ts.

I could see that being valuable in general though. Perhaps a new @pnpm/test-defaults package might be useful.

gluxon · 2024-07-14T01:57:15Z

reviewing/outdated/src/outdated.ts

+          const userWrittenPref = allDeps[alias]
+          const pref = replaceCatalogProtocolIfNecessary(opts.catalogs ?? {}, alias, userWrittenPref)
          // If the npm resolve parser cannot parse the spec of the dependency,
          // it means that the package is not from a npm-compatible registry.
          // In that case, we can't check whether the package is up-to-date
          if (
-            parsePref(allDeps[alias], alias, 'latest', pickRegistryForPackage(opts.registries, name)) == null
+            parsePref(pref, alias, 'latest', pickRegistryForPackage(opts.registries, name)) == null


This is the primary fix.

zkochan · 2024-07-15T00:33:31Z

reviewing/plugin-commands-outdated/test/index.ts

 `)
 })
+
+test.only('pnpm outdated: catalog protocol', async () => {


Suggested change

test.only('pnpm outdated: catalog protocol', async () => {

test('pnpm outdated: catalog protocol', async () => {

Fixed! Thank you for catching.

zkochan · 2024-07-15T00:33:54Z

reviewing/plugin-commands-outdated/test/index.ts

 const hasMajorOutdatedDepsFixture = f.find('has-major-outdated-deps')
 const hasNoLockfileFixture = f.find('has-no-lockfile')
 const withPnpmUpdateIgnore = f.find('with-pnpm-update-ignore')
+const hasOutdatedDepsUsingCatalogProtocol = f.find('has-outdated-deps-using-catalog-protocol')


I have no objections to dynamically generate the fixture with preparePackage

zkochan · 2024-07-15T00:37:33Z

reviewing/outdated/src/outdated.ts

+          const userWrittenPref = allDeps[alias]
+          const pref = replaceCatalogProtocolIfNecessary(opts.catalogs ?? {}, alias, userWrittenPref)


I think something like this would be better as I don't see any reason to pass "userWrittenPref" to the function

Suggested change

const userWrittenPref = allDeps[alias]

const pref = replaceCatalogProtocolIfNecessary(opts.catalogs ?? {}, alias, userWrittenPref)

const pref = tryResolveCatalog(opts.catalogs ?? {}, alias) ?? allDeps[alias]

I think we need to pass userWrittenPref for a few reasons?

The userWrittenPref might be something like catalog:foo. The tryResolveCatalog function will need that to know which catalog to look up.

If the userWrittenPref/allDeps[alias] isn't using the catalog: protocol, we'll want it to be returned as-is, even if there is a catalog entry for that alias.

@Version

@Version

@Version

@Version

@Version

gluxon added 2 commits July 12, 2024 00:57

fix: pnpm outdated works with catalog: protocol

c25aac0

test: add has-outdated-deps-using-catalog-protocol fixture

e339393

gluxon commented Jul 14, 2024

View reviewed changes

gluxon marked this pull request as ready for review July 14, 2024 01:57

gluxon requested a review from zkochan as a code owner July 14, 2024 01:57

zkochan reviewed Jul 15, 2024

View reviewed changes

test: add pnpm outdated catalog protocol test

1a8bdb7

gluxon force-pushed the gluxon/catalog-outdated branch from 8cdae94 to 8dd4d29 Compare July 15, 2024 05:22

chore: changeset

a3c7a3a

gluxon force-pushed the gluxon/catalog-outdated branch from 8dd4d29 to a3c7a3a Compare July 15, 2024 05:53

refactor: outdated

9a14da7

zkochan approved these changes Jul 15, 2024

View reviewed changes

zkochan merged commit f6e7ace into pnpm:main Jul 15, 2024

gluxon mentioned this pull request Oct 13, 2024

Catalogs Tracking Issue #7072

Open

18 tasks

gluxon deleted the gluxon/catalog-outdated branch March 2, 2025 03:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: `pnpm outdated` should work with `catalog:` protocol#8304

fix: `pnpm outdated` should work with `catalog:` protocol#8304
zkochan merged 5 commits intopnpm:mainfrom
gluxon:gluxon/catalog-outdated

gluxon commented Jul 12, 2024 •

edited

Loading

Uh oh!

gluxon Jul 14, 2024

Uh oh!

zkochan Jul 15, 2024

Uh oh!

gluxon Jul 15, 2024

Uh oh!

gluxon Jul 14, 2024

Uh oh!

zkochan Jul 15, 2024

Uh oh!

gluxon Jul 15, 2024

Uh oh!

zkochan Jul 15, 2024

Uh oh!

zkochan Jul 15, 2024

Uh oh!

gluxon Jul 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	test.only('pnpm outdated: catalog protocol', async () => {
	test('pnpm outdated: catalog protocol', async () => {

		const userWrittenPref = allDeps[alias]
		const pref = replaceCatalogProtocolIfNecessary(opts.catalogs ?? {}, alias, userWrittenPref)

	const userWrittenPref = allDeps[alias]
	const pref = replaceCatalogProtocolIfNecessary(opts.catalogs ?? {}, alias, userWrittenPref)
	const pref = tryResolveCatalog(opts.catalogs ?? {}, alias) ?? allDeps[alias]

Uh oh!

Conversation

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

Uh oh!

Problem

Changes

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gluxon commented Jul 12, 2024 •

edited

Loading