fix package loop breaking, do some renaming

Author: ds300

integration-tests/apply-multiple-patches/__snapshots__/apply-multiple-patches.test.ts.snap

@@ -9,7 +9,7 @@ exports[`Test apply-multiple-patches: patch-package fails when a patch in the se
 
   This error was caused because patch-package cannot apply the following patch file:
 
-    patches/left-pad+1.3.0+03+broken.patch
+    patches/left-pad+1.3.0+02+broken.patch
 
   Try removing node_modules and trying again. If that doesn't work, maybe there was
   an accidental change made to the patch file? Try recreating it by manually
@@ -33,6 +33,14 @@ exports[`Test apply-multiple-patches: patch-package happily applies both good pa
 patch-package 0.0.0
 Applying patches...
 [email protected] (1 hello) ✔
-[email protected] (2 world) ✔
+[email protected] (3 world) ✔
+END SNAPSHOT"
+`;
+
+exports[`Test apply-multiple-patches: patch-package only applies the first patch if the second of three is invalid 1`] = `
+"SNAPSHOT: patch-package only applies the first patch if the second of three is invalid
+patch-package 0.0.0
+Applying patches...
+[email protected] (1 hello) ✔
 END SNAPSHOT"
 `;

integration-tests/apply-multiple-patches/apply-multiple-patches.sh

@@ -20,3 +20,10 @@ then
   exit 1
 fi
 (>&2 echo "END SNAPSHOT")
+
+echo "SNAPSHOT: patch-package only applies the first patch if the second of three is invalid"
+if patch-package
+then
+  exit 1
+fi
+echo "END SNAPSHOT"

integration-tests/apply-multiple-patches/left-pad+1.3.0+03+broken.patch renamed to integration-tests/apply-multiple-patches/left-pad+1.3.0+02+broken.patch

@@ -121,7 +121,7 @@ export function applyPatchesForApp({
     details.sort((a, b) => {
       return (a.sequenceNumber ?? 0) - (b.sequenceNumber ?? 0)
     })
-    packageLoop: for (const packageDetails of details) {
+    packageLoop: for (const patchDetails of details) {
       try {
         const {
           name,
@@ -130,7 +130,7 @@ export function applyPatchesForApp({
           pathSpecifier,
           isDevOnly,
           patchFilename,
-        } = packageDetails
+        } = patchDetails
 
         const installedPackageVersion = getInstalledPackageVersion({
           appPath,
@@ -140,7 +140,10 @@ export function applyPatchesForApp({
             isDevOnly ||
             // check for direct-dependents in prod
             (process.env.NODE_ENV === "production" &&
-              packageIsDevDependency({ appPath, packageDetails })),
+              packageIsDevDependency({
+                appPath,
+                patchDetails,
+              })),
           patchFilename,
         })
         if (!installedPackageVersion) {
@@ -157,7 +160,7 @@ export function applyPatchesForApp({
           applyPatch({
             patchFilePath: resolve(patchesDirectory, patchFilename) as string,
             reverse,
-            packageDetails,
+            patchDetails,
             patchDir,
           })
         ) {
@@ -175,10 +178,10 @@ export function applyPatchesForApp({
             )
           }
           const sequenceString =
-            packageDetails.sequenceNumber != null
-              ? ` (${packageDetails.sequenceNumber}${
-                  packageDetails.sequenceName
-                    ? " " + packageDetails.sequenceName
+            patchDetails.sequenceNumber != null
+              ? ` (${patchDetails.sequenceNumber}${
+                  patchDetails.sequenceName
+                    ? " " + patchDetails.sequenceName
                     : ""
                 })`
               : ""
@@ -198,6 +201,9 @@ export function applyPatchesForApp({
               path,
             }),
           )
+          // in case the package has multiple patches, we need to break out of this inner loop
+          // because we don't want to apply more patches on top of the broken state
+          break packageLoop
         } else {
           errors.push(
             createPatchApplicationFailureError({
@@ -209,21 +215,24 @@ export function applyPatchesForApp({
               pathSpecifier,
             }),
           )
+          // in case the package has multiple patches, we need to break out of this inner loop
+          // because we don't want to apply more patches on top of the broken state
+          break packageLoop
         }
       } catch (error) {
         if (error instanceof PatchApplicationError) {
           errors.push(error.message)
         } else {
           errors.push(
             createUnexpectedError({
-              filename: packageDetails.patchFilename,
+              filename: patchDetails.patchFilename,
               error: error as Error,
             }),
           )
         }
-        if (details.length > 0) {
-          continue packageLoop
-        }
+        // in case the package has multiple patches, we need to break out of this inner loop
+        // because we don't want to apply more patches on top of the broken state
+        break packageLoop
       }
     }
   }
@@ -265,15 +274,19 @@ export function applyPatchesForApp({
 export function applyPatch({
   patchFilePath,
   reverse,
-  packageDetails,
+  patchDetails,
   patchDir,
 }: {
   patchFilePath: string
   reverse: boolean
-  packageDetails: PackageDetails
+  patchDetails: PackageDetails
   patchDir: string
 }): boolean {
-  const patch = readPatch({ patchFilePath, packageDetails, patchDir })
+  const patch = readPatch({
+    patchFilePath,
+    patchDetails,
+    patchDir,
+  })
   try {
     executeEffects(reverse ? reversePatch(patch) : patch, { dryRun: false })
   } catch (e) {

integration-tests/apply-multiple-patches/patches/left-pad+1.3.0+02+world.patch renamed to integration-tests/apply-multiple-patches/patches/left-pad+1.3.0+03+world.patch

@@ -11,7 +11,7 @@ describe(packageIsDevDependency, () => {
     expect(
       packageIsDevDependency({
         appPath,
-        packageDetails: getPackageDetailsFromPatchFilename(
+        patchDetails: getPackageDetailsFromPatchFilename(
           "typescript+3.0.1.patch",
         )!,
       }),
@@ -21,9 +21,7 @@ describe(packageIsDevDependency, () => {
     expect(
       packageIsDevDependency({
         appPath,
-        packageDetails: getPackageDetailsFromPatchFilename(
-          "chalk+3.0.1.patch",
-        )!,
+        patchDetails: getPackageDetailsFromPatchFilename("chalk+3.0.1.patch")!,
       }),
     ).toBe(false)
   })
@@ -32,7 +30,7 @@ describe(packageIsDevDependency, () => {
     expect(
       packageIsDevDependency({
         appPath,
-        packageDetails: getPackageDetailsFromPatchFilename(
+        patchDetails: getPackageDetailsFromPatchFilename(
           // cosmiconfig is a transitive dep of lint-staged
           "cosmiconfig+3.0.1.patch",
         )!,

src/applyPatches.ts

@@ -4,15 +4,17 @@ import { existsSync } from "fs"
 
 export function packageIsDevDependency({
   appPath,
-  packageDetails,
+  patchDetails,
 }: {
   appPath: string
-  packageDetails: PatchedPackageDetails
+  patchDetails: PatchedPackageDetails
 }) {
   const packageJsonPath = join(appPath, "package.json")
   if (!existsSync(packageJsonPath)) {
     return false
   }
   const { devDependencies } = require(packageJsonPath)
-  return Boolean(devDependencies && devDependencies[packageDetails.packageNames[0]])
+  return Boolean(
+    devDependencies && devDependencies[patchDetails.packageNames[0]],
+  )
 }

src/packageIsDevDependency.test.ts

@@ -28,7 +28,7 @@ describe(readPatch, () => {
   it("throws an error for basic packages", () => {
     readPatch({
       patchFilePath: "/test/root/patches/test+1.2.3.patch",
-      packageDetails: getPackageDetailsFromPatchFilename("test+1.2.3.patch")!,
+      patchDetails: getPackageDetailsFromPatchFilename("test+1.2.3.patch")!,
       patchDir: "patches/",
     })
 
@@ -56,7 +56,7 @@ describe(readPatch, () => {
   it("throws an error for scoped packages", () => {
     readPatch({
       patchFilePath: "/test/root/patches/@david+test+1.2.3.patch",
-      packageDetails: getPackageDetailsFromPatchFilename(
+      patchDetails: getPackageDetailsFromPatchFilename(
         "@david+test+1.2.3.patch",
       )!,
       patchDir: "patches/",
@@ -87,7 +87,7 @@ describe(readPatch, () => {
     const patchFileName = "@david+test++react-native+1.2.3.patch"
     readPatch({
       patchFilePath: `/test/root/patches/${patchFileName}`,
-      packageDetails: getPackageDetailsFromPatchFilename(patchFileName)!,
+      patchDetails: getPackageDetailsFromPatchFilename(patchFileName)!,
       patchDir: "patches/",
     })
 
@@ -116,7 +116,7 @@ describe(readPatch, () => {
     const patchFileName = "@david+test++react-native+1.2.3.patch"
     readPatch({
       patchFilePath: `/test/root/.cruft/patches/${patchFileName}`,
-      packageDetails: getPackageDetailsFromPatchFilename(patchFileName)!,
+      patchDetails: getPackageDetailsFromPatchFilename(patchFileName)!,
       patchDir: ".cruft/patches",
     })
 
@@ -145,7 +145,7 @@ describe(readPatch, () => {
     const patchFileName = "@david+test++react-native+1.2.3.patch"
     readPatch({
       patchFilePath: `/test/root/packages/banana/patches/${patchFileName}`,
-      packageDetails: getPackageDetailsFromPatchFilename(patchFileName)!,
+      patchDetails: getPackageDetailsFromPatchFilename(patchFileName)!,
       patchDir: "patches/",
     })
 
@@ -178,7 +178,7 @@ describe(readPatch, () => {
     const patchFileName = "@david+test++react-native+1.2.3.patch"
     readPatch({
       patchFilePath: `/test/root/packages/banana/.patches/${patchFileName}`,
-      packageDetails: getPackageDetailsFromPatchFilename(patchFileName)!,
+      patchDetails: getPackageDetailsFromPatchFilename(patchFileName)!,
       patchDir: ".patches/",
     })
 

src/packageIsDevDependency.ts

@@ -7,11 +7,11 @@ import { parsePatchFile, PatchFilePart } from "./parse"
 
 export function readPatch({
   patchFilePath,
-  packageDetails,
+  patchDetails,
   patchDir,
 }: {
   patchFilePath: string
-  packageDetails: PackageDetails
+  patchDetails: PackageDetails
   patchDir: string
 }): PatchFilePart[] {
   try {
@@ -33,7 +33,7 @@ export function readPatch({
         relativePatchFilePath.indexOf(patchDir),
       )}`,
     )
-    fixupSteps.push(`npx patch-package ${packageDetails.pathSpecifier}`)
+    fixupSteps.push(`npx patch-package ${patchDetails.pathSpecifier}`)
     if (patchBaseDir) {
       fixupSteps.push(
         `cd ${relative(resolve(process.cwd(), patchBaseDir), process.cwd())}`,
@@ -43,7 +43,7 @@ export function readPatch({
     console.error(`
 ${chalk.red.bold("**ERROR**")} ${chalk.red(
       `Failed to apply patch for package ${chalk.bold(
-        packageDetails.humanReadablePathSpecifier,
+        patchDetails.humanReadablePathSpecifier,
       )}`,
     )}