fix: do not support mixing pnpm.patchedDependencies and npm_translate…

…_lock(patches)

Fix #1427

MODULE.bazel

@@ -187,12 +187,6 @@ npm.npm_translate_lock(
         "unused": ["//visibility:private"],
         "@mycorp/pkg-a": ["//examples:__subpackages__"],
     },
-    patch_args = {
-        "*": ["-p1"],
-    },
-    patches = {
-        "meaning-of-life@1.0.0": ["//examples/npm_deps:patches/meaning-of-life@1.0.0-after_pnpm.patch"],
-    },
     pnpm_lock = "//:pnpm-lock.yaml",
     public_hoist_packages = {
         # Instructs the linker to hoist the ms@2.1.3 npm package to `node_modules/ms` in the `examples/npm_deps` package.

WORKSPACE

@@ -141,12 +141,6 @@ npm_translate_lock(
         "unused": ["//visibility:private"],
         "@mycorp/pkg-a": ["//examples:__subpackages__"],
     },
-    patch_args = {
-        "*": ["-p1"],
-    },
-    patches = {
-        "meaning-of-life@1.0.0": ["//examples/npm_deps:patches/meaning-of-life@1.0.0-after_pnpm.patch"],
-    },
     pnpm_lock = "//:pnpm-lock.yaml",
     # Use a version that's not vendored into rules_js by providing a (version, integrity) tuple.
     # curl --silent https://registry.npmjs.org/pnpm | jq '.versions["8.6.11"].dist.integrity'

examples/npm_deps/patched-dependencies-test.js

@@ -1,19 +1,12 @@
 const meaningOfLife = require('meaning-of-life')
 
-// meaning-of-life should have been patched twice:
-//
-// First, by the `pnpm.patchedDependencies` patch:
+// Verify meaning-of-life was patched with
 //  examples/npm_deps/patches/meaning-of-life@1.0.0-pnpm.patch
-// 42 => "forty two"
-//
-// Then by the the following patch in the `patches` attr:
-//  examples/npm_deps/patches/meaning-of-life@1.0.0-after_pnpm.patch
-// "forty two" => 32
 
 if (meaningOfLife === 42) {
     throw new Error('Patches were not applied!')
-} else if (meaningOfLife === 'forty two') {
-    throw new Error('Only `pnpm.patchedDependencies` patch was applied!')
-} else if (meaningOfLife !== 32) {
-    throw new Error('Patch in `patches` was not applied!')
+}
+
+if (meaningOfLife !== 'forty two') {
+    throw new Error('`pnpm.patchedDependencies` was not applied!')
 }

examples/npm_deps/patches/patches

@@ -1,2 +1 @@
-examples/npm_deps/patches/meaning-of-life@1.0.0-after_pnpm.patch
 examples/npm_deps/patches/meaning-of-life@1.0.0-pnpm.patch

js/private/test/image/checksum.expected

@@ -1,2 +1,2 @@
-e6a04b6344eb21154508bf246ebead12d436313739c478978cd5763262491f98  js/private/test/image/cksum_app.tar
-7990126e5961588efd2af7cdccac3846dfffb8ca2e26206840df8c7a1c34c42e  js/private/test/image/cksum_node_modules.tar
+f949b3cd67fab3be9b0b28158be2fd88ebcaa914896671207e4bfc256c8bc6c3  js/private/test/image/cksum_app.tar
+f17d1b735862ebdb8e68361a492230acdb087e1382163703febfde9253d5e196  js/private/test/image/cksum_node_modules.tar

npm/private/npm_translate_lock_helpers.bzl

@@ -326,12 +326,28 @@ def _get_npm_imports(importers, packages, patched_dependencies, root_package, rc
             unfriendly_name = None
 
         # gather patches & patch args
-        patches, patches_keys = _gather_values_from_matching_names(True, attr.patches, name, friendly_name, unfriendly_name)
+        patches = []
+        patch_args = []
+
+        translate_patches, patches_keys = _gather_values_from_matching_names(True, attr.patches, name, friendly_name, unfriendly_name)
+
+        if len(translate_patches) > 0 and pnpm_patched:
+            msg = """\
+ERROR: can not apply both `pnpm.patchedDependencies` and `npm_translate_lock(patches)` to the same package {pkg}.
+""".format(
+                pkg = name,
+            )
+            fail(msg)
 
         # Apply patch from `pnpm.patchedDependencies` first
         if pnpm_patched:
             patch_path = "//%s:%s" % (attr.pnpm_lock.package, patched_dependencies.get(friendly_name).get("path"))
-            patches.insert(0, patch_path)
+            patches.append(patch_path)
+            patch_args = ["-p1"]
+        elif len(translate_patches) > 0:
+            patches = translate_patches
+            patch_args, _ = _gather_values_from_matching_names(False, attr.patch_args, "*", name, friendly_name, unfriendly_name)
+            patches_used.extend(patches_keys)
 
         # Resolve string patch labels relative to the root respository rather than relative to rules_js.
         # https://docs.google.com/document/d/1N81qfCa8oskCk5LqTW-LNthy6EBrDot7bdUsjz6JFC4/
@@ -341,25 +357,6 @@ def _get_npm_imports(importers, packages, patched_dependencies, root_package, rc
         # that checked in repositories.bzl files don't fail diff tests when run under multiple versions of Bazel.
         patches = [("@" if patch.startswith("//") else "") + patch for patch in patches]
 
-        patch_args, _ = _gather_values_from_matching_names(False, attr.patch_args, "*", name, friendly_name, unfriendly_name)
-
-        # Patches in `pnpm.patchedDependencies` must have the -p1 format. Therefore any
-        # patches applied via `patches` must also use -p1 since we don't support different
-        # patch args for different patches.
-        if pnpm_patched and not _has_strip_prefix_arg(patch_args, 1):
-            if _has_strip_prefix_arg(patch_args):
-                msg = """\
-ERROR: patch_args for package {package} contains a strip prefix that is incompatible with a patch applied via `pnpm.patchedDependencies`.
-
-`pnpm.patchedDependencies` requires a strip prefix of `-p1`. All applied patches must use the same strip prefix.
-
-""".format(package = friendly_name)
-                fail(msg)
-            patch_args = patch_args[:]
-            patch_args.append("-p1")
-
-        patches_used.extend(patches_keys)
-
         # gather replace packages
         replace_packages, _ = _gather_values_from_matching_names(True, attr.replace_packages, name, friendly_name, unfriendly_name)
         if len(replace_packages) > 1:
@@ -496,15 +493,6 @@ def _link_package(root_package, import_path, rel_path = "."):
 def _is_url(url):
     return url.find("://") != -1
 
-################################################################################
-def _has_strip_prefix_arg(patch_args, strip_num = None):
-    if strip_num != None:
-        return "-p%d" % strip_num in patch_args or "--strip=%d" % strip_num in patch_args
-    for arg in patch_args:
-        if arg.startswith("-p") or arg.startswith("--strip="):
-            return True
-    return False
-
 ################################################################################
 def _to_apparent_repo_name(canonical_name):
     return canonical_name[canonical_name.rfind("~") + 1:]