Fixes the changes patch-commit generates

.yarn/versions/434fcaba.yml

@@ -0,0 +1,33 @@
+releases:
+  "@yarnpkg/cli": major
+  "@yarnpkg/core": minor
+  "@yarnpkg/plugin-patch": major
+
+declined:
+  - "@yarnpkg/plugin-compat"
+  - "@yarnpkg/plugin-constraints"
+  - "@yarnpkg/plugin-dlx"
+  - "@yarnpkg/plugin-essentials"
+  - "@yarnpkg/plugin-exec"
+  - "@yarnpkg/plugin-file"
+  - "@yarnpkg/plugin-git"
+  - "@yarnpkg/plugin-github"
+  - "@yarnpkg/plugin-http"
+  - "@yarnpkg/plugin-init"
+  - "@yarnpkg/plugin-interactive-tools"
+  - "@yarnpkg/plugin-link"
+  - "@yarnpkg/plugin-nm"
+  - "@yarnpkg/plugin-npm"
+  - "@yarnpkg/plugin-npm-cli"
+  - "@yarnpkg/plugin-pack"
+  - "@yarnpkg/plugin-pnp"
+  - "@yarnpkg/plugin-pnpm"
+  - "@yarnpkg/plugin-stage"
+  - "@yarnpkg/plugin-typescript"
+  - "@yarnpkg/plugin-version"
+  - "@yarnpkg/plugin-workspace-tools"
+  - "@yarnpkg/builder"
+  - "@yarnpkg/doctor"
+  - "@yarnpkg/nm"
+  - "@yarnpkg/pnpify"
+  - "@yarnpkg/sdks"

packages/acceptance-tests/pkg-tests-specs/sources/commands/patchCommit.test.ts

@@ -54,5 +54,69 @@ describe(`Commands`, () => {
         });
       }),
     );
+
+    test(
+      `it should reference patches from the workspace dependencies when possible`,
+      makeTemporaryEnv({
+        dependencies: {
+          [`one-fixed-dep`]: `1.0.0`,
+        },
+      }, async ({path, run, source}) => {
+        await run(`install`);
+
+        const {stdout} = await run(`patch`, `one-fixed-dep`, `--json`);
+        const {path: updateFolderN} = JSON.parse(stdout);
+
+        const updateFolder = npath.toPortablePath(updateFolderN);
+        const updateFile = ppath.join(updateFolder, `index.js` as Filename);
+
+        const fileSource = await xfs.readFilePromise(updateFile, `utf8`);
+        const fileUser = fileSource.replace(`require(dep)`, `42`);
+        await xfs.writeFilePromise(updateFile, fileUser);
+
+        await run(`patch-commit`, `-s`, npath.fromPortablePath(updateFolder));
+
+        const manifest = await xfs.readJsonPromise(ppath.join(path, Filename.manifest));
+
+        expect(manifest.dependencies).toEqual({
+          [`one-fixed-dep`]: `patch:[email protected]#.yarn/patches/one-fixed-dep-npm-1.0.0-b02516a4af.patch`,
+        });
+
+        expect(manifest).not.toHaveProperty(`resolutions`);
+      }),
+    );
+
+    test(
+      `it should reference patches using the 'resolutions' field when required`,
+      makeTemporaryEnv({
+        dependencies: {
+          [`one-fixed-dep`]: `1.0.0`,
+        },
+      }, async ({path, run, source}) => {
+        await run(`install`);
+
+        const {stdout} = await run(`patch`, `no-deps`, `--json`);
+        const {path: updateFolderN} = JSON.parse(stdout);
+
+        const updateFolder = npath.toPortablePath(updateFolderN);
+        const updateFile = ppath.join(updateFolder, `index.js` as Filename);
+
+        const fileSource = await xfs.readFilePromise(updateFile, `utf8`);
+        const fileUser = fileSource.replace(`require(dep)`, `42`);
+        await xfs.writeFilePromise(updateFile, fileUser);
+
+        await run(`patch-commit`, `-s`, npath.fromPortablePath(updateFolder));
+
+        const manifest = await xfs.readJsonPromise(ppath.join(path, Filename.manifest));
+
+        expect(manifest.dependencies).toEqual({
+          [`one-fixed-dep`]: `1.0.0`,
+        });
+
+        expect(manifest.resolutions).toEqual({
+          [`[email protected]`]: `patch:[email protected]#.yarn/patches/no-deps-npm-1.0.0-cf533b267a.patch`,
+        });
+      }),
+    );
   });
 });

packages/plugin-patch/sources/commands/patchCommit.ts

@@ -1,9 +1,9 @@
-import {BaseCommand, WorkspaceRequiredError}       from '@yarnpkg/cli';
-import {Configuration, Project, structUtils}       from '@yarnpkg/core';
-import {npath, xfs, ppath, PortablePath, Filename} from '@yarnpkg/fslib';
-import {Command, Option, Usage, UsageError}        from 'clipanion';
+import {BaseCommand, WorkspaceRequiredError}                                        from '@yarnpkg/cli';
+import {Configuration, Descriptor, DescriptorHash, Project, structUtils, Workspace} from '@yarnpkg/core';
+import {npath, xfs, ppath, PortablePath, Filename}                                  from '@yarnpkg/fslib';
+import {Command, Option, Usage, UsageError}                                         from 'clipanion';
 
-import * as patchUtils                             from '../patchUtils';
+import * as patchUtils                                                              from '../patchUtils';
 
 // eslint-disable-next-line arca/no-default-export
 export default class PatchCommitCommand extends BaseCommand {
@@ -63,12 +63,71 @@ export default class PatchCommitCommand extends BaseCommand {
     await xfs.mkdirPromise(patchFolder, {recursive: true});
     await xfs.writeFilePromise(patchPath, diff);
 
-    const relPath = ppath.relative(project.cwd, patchPath);
+    const workspaceDependents: Array<Workspace> = [];
+    const transitiveDependencies = new Map<DescriptorHash, Descriptor>();
 
-    project.topLevelWorkspace.manifest.resolutions.push({
-      pattern: {descriptor: {fullName: structUtils.stringifyIdent(locator), description: meta.version}},
-      reference: `patch:${structUtils.stringifyLocator(locator)}#${relPath}`,
-    });
+    for (const pkg of project.storedPackages.values()) {
+      if (structUtils.isVirtualLocator(pkg))
+        continue;
+
+      const descriptor = pkg.dependencies.get(locator.identHash);
+      if (!descriptor)
+        continue;
+
+      const devirtualizedDescriptor = structUtils.ensureDevirtualizedDescriptor(descriptor);
+      if (!devirtualizedDescriptor)
+        continue;
+
+      const resolution = project.storedResolutions.get(devirtualizedDescriptor.descriptorHash);
+      if (!resolution)
+        throw new Error(`Assertion failed: Expected the resolution to have been registered`);
+
+      const dependency = project.storedPackages.get(resolution);
+      if (!dependency)
+        throw new Error(`Assertion failed: Expected the package to have been registered`);
+
+      const workspace = project.tryWorkspaceByLocator(pkg);
+      if (workspace) {
+        workspaceDependents.push(workspace);
+      } else {
+        const originalPkg = project.originalPackages.get(pkg.locatorHash);
+        if (!originalPkg)
+          throw new Error(`Assertion failed: Expected the original package to have been registered`);
+
+        const originalDependency = originalPkg.dependencies.get(descriptor.identHash);
+        if (!originalDependency)
+          throw new Error(`Assertion failed: Expected the original dependency to have been registered`);
+
+        transitiveDependencies.set(originalDependency.descriptorHash, originalDependency);
+      }
+    }
+
+    for (const workspace of workspaceDependents) {
+      const originalDescriptor = workspace.manifest.dependencies.get(locator.identHash);
+      if (!originalDescriptor)
+        throw new Error(`Assertion failed: Expected the package to have been registered`);
+
+      const newDescriptor = patchUtils.makeDescriptor(originalDescriptor, {
+        parentLocator: null,
+        sourceDescriptor: originalDescriptor,
+        patchPaths: [ppath.relative(workspace.cwd, patchPath)],
+      });
+
+      workspace.manifest.dependencies.set(originalDescriptor.identHash, newDescriptor);
+    }
+
+    for (const originalDescriptor of transitiveDependencies.values()) {
+      const newDescriptor = patchUtils.makeDescriptor(originalDescriptor, {
+        parentLocator: null,
+        sourceDescriptor: originalDescriptor,
+        patchPaths: [ppath.relative(workspace.cwd, patchPath)],
+      });
+
+      project.topLevelWorkspace.manifest.resolutions.push({
+        pattern: {descriptor: {fullName: structUtils.stringifyIdent(newDescriptor), description: originalDescriptor.range}},
+        reference: newDescriptor.range,
+      });
+    }
 
     await project.persist();
   }

packages/plugin-patch/sources/patchUtils.ts

@@ -65,8 +65,8 @@ function makeSpec<T>({parentLocator, sourceItem, patchPaths, sourceVersion, patc
   });
 }
 
-export function makeDescriptor(ident: Ident, {parentLocator, sourceDescriptor, patchPaths}: ReturnType<typeof parseDescriptor>) {
-  return structUtils.makeLocator(ident, makeSpec({parentLocator, sourceItem: sourceDescriptor, patchPaths}, structUtils.stringifyDescriptor));
+export function makeDescriptor(ident: Ident, {parentLocator, sourceDescriptor, patchPaths}: Pick<ReturnType<typeof parseDescriptor>, `parentLocator` | `sourceDescriptor` | `patchPaths`>) {
+  return structUtils.makeDescriptor(ident, makeSpec({parentLocator, sourceItem: sourceDescriptor, patchPaths}, structUtils.stringifyDescriptor));
 }
 
 export function makeLocator(ident: Ident, {parentLocator, sourcePackage, patchPaths, patchHash}: Omit<ReturnType<typeof parseLocator>, 'sourceLocator' | 'sourceVersion'> & {sourcePackage: Package, patchHash: string}) {

packages/yarnpkg-core/sources/structUtils.ts

@@ -207,6 +207,27 @@ export function devirtualizeLocator(locator: Locator): Locator {
   return makeLocator(locator, locator.reference.replace(/^[^#]*#/, ``));
 }
 
+/**
+ * Returns a descriptor guaranteed to be devirtualized
+ */
+export function ensureDevirtualizedDescriptor(descriptor: Descriptor): Descriptor {
+  if (!isVirtualDescriptor(descriptor))
+    return descriptor;
+
+  return makeDescriptor(descriptor, descriptor.range.replace(/^[^#]*#/, ``));
+}
+
+/**
+ * Returns a locator guaranteed to be devirtualized
+ * @param locator the locator
+ */
+export function ensureDevirtualizedLocator(locator: Locator): Locator {
+  if (!isVirtualLocator(locator))
+    return locator;
+
+  return makeLocator(locator, locator.reference.replace(/^[^#]*#/, ``));
+}
+
 /**
  * Some descriptors only make sense when bound with some internal state. For
  * instance that would be the case for the `file:` ranges, which require to