feat(groups): handle negation for packages option

[ardelato]

Description

This modifies matchesPackages to make use of the pre-existing matchesKnowList helper function, thus allowing matchesPackages the ability to handle negated patterns.

Justification

I ran into an issue with isBanned and a misunderstanding with the documentation surrounding the optional packages setting.

From the documentation, it stated that packages will utilize minimatch to find matches. In addition, I noted that dependencyTypes and specifierTypes supported an array of negated values. I mistakenly conflated this support to include packages. Thus, I was planning to have an array of negated package names to act as a blacklist from the ban rule, but it did not work as expected.

After taking a look at the source code, I noted that unlike matchesSpecifierTypes or matchesDependencyTypes, matchesPackages does not handle negated patterns well. Even though minimatch is capable of handling negated patterns, its implementation within matchesPackages limits its capabilities.

syncpack/src/guards/can-add-to-group.ts

Lines 41 to 45 in e85b234

	function matchesPackages(packages: unknown, instance: Instance) {
	// matches if not defined
	if (!isNonEmptyArray(packages)) return true;
	return packages.some((pattern) => minimatch(instance.pkgName, pattern));
	}

With the current implementation, the only method to exempt packages from any rule is to use a single negated pattern in the array or treat the packages array as a whitelist. For example, either ["!packageA"] or ["packageB", "packageC", "packageD"]

The former is difficult if there is no common pattern that could match all packages you'd want to exempt. The latter would be hard to maintain as you would need to constantly add to it if you create a new package in your monorepo.

As such, it seems reasonable to use the same helper function as matchesSpecifierTypes and matchesDependencyTypes to allow matchesPackages the ability to handle multiple negated patterns.

I further modified matchesKnownList to use minimatch instead of includes for matching against the values. This extends the options for specifierTypes and dependencyTypes while retaining the same behavior for packages of allowing scoped package patterns -- i.e. @my-scope/**.

How Can This Be Tested?

I tried writing a unit test specifically for can-add-to-group.ts but I was having a hard time creating the arguments for canAddToGroup.

syncpack/src/guards/can-add-to-group.ts

Lines 8 to 11 in e85b234

	export function canAddToGroup(
	packageJsonFilesByName: Ctx['packageJsonFilesByName'],
	group: SemverGroup.Any | VersionGroup.Any,
	instance: Instance,

I was further confused on how, if at all, I could make use of the create-scenario helper function:
https://github.com/JamieMason/syncpack/blob/main/test/lib/create-scenario.ts

I did extend the banned.spec.ts test but I was unsure about adding it since the underlying changes affect all Group types not just BannedVersionGroup

banned.spec.ts diff

diff --git a/src/version-group/banned.spec.ts b/src/version-group/banned.spec.ts
index d1177f8..b8db515 100644
--- a/src/version-group/banned.spec.ts
+++ b/src/version-group/banned.spec.ts
@@ -124,3 +124,100 @@ describe('mismatches', () => {
     });
   });
 });
+
+describe('mixed matches', () => {
+  describe('when a banned dependency is used outside negated packages', () => {
+    const getScenario = createScenario({
+      '.syncpackrc': {
+        versionGroups: [
+          {
+            dependencies: ['foo'],
+            packages: ['!b', '!@my-scope/**'],
+            isBanned: true,
+          },
+        ],
+      },
+      'package.json': {
+        name: 'a',
+        version: '0.0.0',
+        dependencies: {
+          foo: '0.1.0',
+        },
+      },
+      'packages/b/package.json': {
+        name: 'b',
+        version: '0.0.0',
+        dependencies: {
+          foo: '0.1.0',
+          bar: '0.1.0',
+        },
+      },
+      'packages/c/package.json': {
+        name: '@my-scope/c',
+        version: '0.0.0',
+        dependencies: {
+          foo: '0.1.0',
+        },
+      },
+      'packages/d/package.json': {
+        name: '@my-scope/d',
+        version: '0.0.0',
+        dependencies: {
+          foo: '0.1.0',
+        },
+      },
+      'packages/e/package.json': {
+        name: 'e',
+        version: '0.0.0',
+        dependencies: {
+          foo: '0.1.0',
+        },
+      }
+    });
+
+    it('is invalid because it should not be used', async () => {
+      const reports = await getScenario().getVersionReports();
+      expect(reports).toHaveLength(8);
+      expect(reports).toHaveProperty('0.name', 'foo');
+      expect(reports).toHaveProperty('0.reports.0._tag', 'Banned');
+    });
+
+    describe('lint', () => {
+      it('exits 1', async () => {
+        const scenario = getScenario();
+        await Effect.runPromiseExit(lint(scenario));
+        expect(scenario.io.process.exit).toHaveBeenCalledWith(1);
+      });
+    });
+
+    describe('list', () => {
+      it('exits 1', async () => {
+        const scenario = getScenario();
+        await Effect.runPromiseExit(list(scenario));
+        expect(scenario.io.process.exit).toHaveBeenCalledWith(1);
+      });
+    });
+
+    describe('list-mismatches', () => {
+      it('exits 1', async () => {
+        const scenario = getScenario();
+        await Effect.runPromiseExit(listMismatches(scenario));
+        expect(scenario.io.process.exit).toHaveBeenCalledWith(1);
+      });
+    });
+
+    describe('fix-mismatches', () => {
+      it('removes them', async () => {
+        const scenario = getScenario();
+        await Effect.runPromiseExit(fixMismatches(scenario));
+        expect(scenario.readPackages()).not.toHaveProperty('a.dependencies.foo');
+        expect(scenario.readPackages()).toHaveProperty('b.dependencies.foo', '0.1.0');
+        expect(scenario.readPackages()).toHaveProperty('b.dependencies.bar', '0.1.0');
+        expect(scenario.readPackages()).toHaveProperty('@my-scope/c.dependencies.foo', '0.1.0');
+        expect(scenario.readPackages()).toHaveProperty('@my-scope/d.dependencies.foo', '0.1.0');
+        expect(scenario.readPackages()).not.toHaveProperty('e.dependencies.foo');
+        expect(scenario.io.process.exit).not.toHaveBeenCalled();
+      });
+    });
+  });
+});

That said, that tests still continued to pass even after the changes.

[JamieMason]

Love it @ardelato, thanks a lot. I'll break off from the Rust rewrite and get this out on its own. Another excellent PR, thanks so much. Yes the testing setup is a little awkward, I really want full integration tests as much as possible, without using a real file system, and the way it's done isn't ideal. Thanks though for looking at those.

[JamieMason]

Released in 12.4.0