refactor: remove npm_translate_lock(use_starlark_yaml_parser) (#1658)

e2e/npm_translate_lock/WORKSPACE

@@ -19,7 +19,6 @@ npm_translate_lock(
     npmrc = "//:.npmrc",
     pnpm_lock = "//:pnpm-lock.yaml",
     update_pnpm_lock = True,
-    use_starlark_yaml_parser = True,  # test coverage for this flag
     verify_node_modules_ignored = "//:.bazelignore",
 )
 

e2e/webpack_devserver/MODULE.bazel

@@ -12,7 +12,6 @@ npm.npm_translate_lock(
     name = "npm",
     npmrc = "//:.npmrc",
     pnpm_lock = "//:pnpm-lock.yaml",
-    use_starlark_yaml_parser = True,  # test coverage for this flag
     verify_node_modules_ignored = "//:.bazelignore",
 )
 use_repo(npm, "npm")

npm/extensions.bzl

@@ -79,21 +79,19 @@ def _npm_lock_imports_bzlmod(module_ctx, attr):
     lock_packages = {}
     lock_patched_dependencies = {}
     lock_parse_err = None
-    if attr.use_starlark_yaml_parser:
-        lock_importers, lock_packages, lock_patched_dependencies, lock_parse_err = utils.parse_pnpm_lock_yaml(module_ctx.read(attr.pnpm_lock))
+
+    is_windows = repo_utils.is_windows(module_ctx)
+    host_yq = Label("@{}_{}//:yq{}".format(attr.yq_toolchain_prefix, repo_utils.platform(module_ctx), ".exe" if is_windows else ""))
+    yq_args = [
+        str(module_ctx.path(host_yq)),
+        str(module_ctx.path(attr.pnpm_lock)),
+        "-o=json",
+    ]
+    result = module_ctx.execute(yq_args)
+    if result.return_code:
+        lock_parse_err = "failed to parse pnpm lock file with yq. '{}' exited with {}: \nSTDOUT:\n{}\nSTDERR:\n{}".format(" ".join(yq_args), result.return_code, result.stdout, result.stderr)
     else:
-        is_windows = repo_utils.is_windows(module_ctx)
-        host_yq = Label("@{}_{}//:yq{}".format(attr.yq_toolchain_prefix, repo_utils.platform(module_ctx), ".exe" if is_windows else ""))
-        yq_args = [
-            str(module_ctx.path(host_yq)),
-            str(module_ctx.path(attr.pnpm_lock)),
-            "-o=json",
-        ]
-        result = module_ctx.execute(yq_args)
-        if result.return_code:
-            lock_parse_err = "failed to parse pnpm lock file with yq. '{}' exited with {}: \nSTDOUT:\n{}\nSTDERR:\n{}".format(" ".join(yq_args), result.return_code, result.stdout, result.stderr)
-        else:
-            lock_importers, lock_packages, lock_patched_dependencies, lock_parse_err = utils.parse_pnpm_lock_json(result.stdout if result.stdout != "null" else None)  # NB: yq will return the string "null" if the yaml file is empty
+        lock_importers, lock_packages, lock_patched_dependencies, lock_parse_err = utils.parse_pnpm_lock_json(result.stdout if result.stdout != "null" else None)  # NB: yq will return the string "null" if the yaml file is empty
 
     if lock_parse_err != None:
         msg = """

npm/private/BUILD.bazel

@@ -211,7 +211,6 @@ bzl_library(
     srcs = ["utils.bzl"],
     visibility = ["//npm:__subpackages__"],
     deps = [
-        ":yaml",
         "@aspect_bazel_lib//lib:paths",
         "@aspect_bazel_lib//lib:repo_utils",
         "@aspect_bazel_lib//lib:utils",
@@ -304,9 +303,3 @@ bzl_library(
     srcs = ["versions.bzl"],
     visibility = ["//npm:__subpackages__"],
 )
-
-bzl_library(
-    name = "yaml",
-    srcs = ["yaml.bzl"],
-    visibility = ["//npm:__subpackages__"],
-)

npm/private/npm_translate_lock.bzl

@@ -76,7 +76,6 @@ _ATTRS = {
     "root_package": attr.string(default = DEFAULT_ROOT_PACKAGE),
     "update_pnpm_lock": attr.bool(),
     "use_home_npmrc": attr.bool(),
-    "use_starlark_yaml_parser": attr.bool(),
     "verify_node_modules_ignored": attr.label(),
     "verify_patches": attr.label(),
     "yarn_lock": attr.label(),
@@ -186,7 +185,6 @@ def npm_translate_lock(
         pnpm_version = LATEST_PNPM_VERSION,
         use_pnpm = None,
         npm_package_target_name = "{dirname}",
-        use_starlark_yaml_parser = False,
         **kwargs):
     """Repository macro to generate starlark code from a lock file.
 
@@ -514,31 +512,6 @@ def npm_translate_lock(
 
             Default: `{dirname}`
 
-        use_starlark_yaml_parser: Opt-out of using `yq` to parse the pnpm-lock file which was added
-            in https://github.com/aspect-build/rules_js/pull/1458 and use the legacy starlark yaml
-            parser instead.
-
-            This opt-out is a return safety in cases where yq is not able to parse the pnpm generated
-            yaml file. For example, this has been observed to happen due to a line such as the following
-            in the pnpm generated lock file:
-
-            ```
-            resolution: {tarball: https://gitpkg.vercel.app/blockprotocol/blockprotocol/packages/%40blockprotocol/type-system-web?6526c0e}
-            ```
-
-            where the `?` character in the `tarball` value causes `yq` to fail with:
-
-            ```
-            $ yq pnpm-lock.yaml -o=json
-            Error: bad file 'pnpm-lock.yaml': yaml: line 7129: did not find expected ',' or '}'
-            ```
-
-            If the tarball value is quoted or escaped then yq would accept it but as of this writing, the latest
-            version of pnpm (8.14.3) does not quote or escape such a value and the latest version of yq (4.40.5)
-            does not handle it as is.
-
-            Possibly related to https://github.com/pnpm/pnpm/issues/5414.
-
         **kwargs: Internal use only
     """
 
@@ -632,7 +605,6 @@ def npm_translate_lock(
         use_pnpm = use_pnpm,
         yq_toolchain_prefix = yq_toolchain_prefix,
         npm_package_target_name = npm_package_target_name,
-        use_starlark_yaml_parser = use_starlark_yaml_parser,
         bzlmod = bzlmod,
     )
 

npm/private/npm_translate_lock_state.bzl

@@ -518,19 +518,17 @@ def _load_lockfile(priv, rctx, label_store):
     packages = {}
     patched_dependencies = {}
     lock_parse_err = None
-    if rctx.attr.use_starlark_yaml_parser:
-        importers, packages, patched_dependencies, lock_parse_err = utils.parse_pnpm_lock_yaml(rctx.read(label_store.path("pnpm_lock")))
+
+    yq_args = [
+        str(label_store.path("host_yq")),
+        str(label_store.path("pnpm_lock")),
+        "-o=json",
+    ]
+    result = rctx.execute(yq_args)
+    if result.return_code:
+        lock_parse_err = "failed to parse pnpm lock file with yq. '{}' exited with {}: \nSTDOUT:\n{}\nSTDERR:\n{}".format(" ".join(yq_args), result.return_code, result.stdout, result.stderr)
     else:
-        yq_args = [
-            str(label_store.path("host_yq")),
-            str(label_store.path("pnpm_lock")),
-            "-o=json",
-        ]
-        result = rctx.execute(yq_args)
-        if result.return_code:
-            lock_parse_err = "failed to parse pnpm lock file with yq. '{}' exited with {}: \nSTDOUT:\n{}\nSTDERR:\n{}".format(" ".join(yq_args), result.return_code, result.stdout, result.stderr)
-        else:
-            importers, packages, patched_dependencies, lock_parse_err = utils.parse_pnpm_lock_json(result.stdout if result.stdout != "null" else None)  # NB: yq will return the string "null" if the yaml file is empty
+        importers, packages, patched_dependencies, lock_parse_err = utils.parse_pnpm_lock_json(result.stdout if result.stdout != "null" else None)  # NB: yq will return the string "null" if the yaml file is empty
 
     priv["importers"] = importers
     priv["packages"] = packages

npm/private/test/BUILD.bazel

@@ -9,7 +9,6 @@ load(":pkg_glob_tests.bzl", "pkg_glob_tests")
 load(":transitive_closure_tests.bzl", "transitive_closure_tests")
 load(":translate_lock_helpers_tests.bzl", "translate_lock_helpers_tests")
 load(":utils_tests.bzl", "utils_tests")
-load(":yaml_tests.bzl", "yaml_tests")
 
 # gazelle:exclude **/snapshots/**/*
 
@@ -26,8 +25,6 @@ transitive_closure_tests(name = "test_transitive_closure")
 
 translate_lock_helpers_tests(name = "test_translate_lock")
 
-yaml_tests(name = "test_yaml")
-
 parse_pnpm_lock_tests(name = "test_parse_pnpm_lock")
 
 generated_pkg_json_test(name = "test_generated_pkg_json")

npm/private/test/parse_pnpm_lock_tests.bzl

@@ -6,55 +6,18 @@ load("//npm/private:utils.bzl", "utils")
 def _parse_empty_lock_test_impl(ctx):
     env = unittest.begin(ctx)
 
-    parsed_yaml = utils.parse_pnpm_lock_yaml("")
     parsed_json_a = utils.parse_pnpm_lock_json("")
     parsed_json_b = utils.parse_pnpm_lock_json("{}")
     expected = ({}, {}, {}, None)
 
-    asserts.equals(env, expected, parsed_yaml)
     asserts.equals(env, expected, parsed_json_a)
     asserts.equals(env, expected, parsed_json_b)
 
     return unittest.end(env)
 
-def _parse_merge_conflict_test_impl(ctx):
-    env = unittest.begin(ctx)
-
-    parsed = utils.parse_pnpm_lock_yaml("""
-importers:
-  .:
-    dependencies:
-<<<<< HEAD""")
-    expected = ({}, {}, {}, "expected lockfileVersion key in lockfile")
-
-    asserts.equals(env, expected, parsed)
-
-    return unittest.end(env)
-
 def _parse_lockfile_v5_test_impl(ctx):
     env = unittest.begin(ctx)
 
-    parsed_yaml = utils.parse_pnpm_lock_yaml("""\
-lockfileVersion: 5.4
-
-specifiers:
-    '@aspect-test/a': 5.0.0
-
-dependencies:
-    '@aspect-test/a': 5.0.0
-
-packages:
-
-    /@aspect-test/a/5.0.0:
-        resolution: {integrity: sha512-t/lwpVXG/jmxTotGEsmjwuihC2Lvz/Iqt63o78SI3O5XallxtFp5j2WM2M6HwkFiii9I42KdlAF8B3plZMz0Fw==}
-        hasBin: true
-        dependencies:
-            '@aspect-test/b': 5.0.0
-            '@aspect-test/c': 1.0.0
-            '@aspect-test/d': 2.0.0_@aspect-test+c@1.0.0
-        dev: false
-""")
-
     parsed_json = utils.parse_pnpm_lock_json("""\
 {
   "lockfileVersion": 5.4,
@@ -109,34 +72,13 @@ packages:
         None,
     )
 
-    asserts.equals(env, expected, parsed_yaml)
     asserts.equals(env, expected, parsed_json)
 
     return unittest.end(env)
 
 def _parse_lockfile_v6_test_impl(ctx):
     env = unittest.begin(ctx)
 
-    parsed_yaml = utils.parse_pnpm_lock_yaml("""\
-lockfileVersion: '6.0'
-
-dependencies:
-  '@aspect-test/a':
-    specifier: 5.0.0
-    version: 5.0.0
-
-packages:
-
-  /@aspect-test/a@5.0.0:
-    resolution: {integrity: sha512-t/lwpVXG/jmxTotGEsmjwuihC2Lvz/Iqt63o78SI3O5XallxtFp5j2WM2M6HwkFiii9I42KdlAF8B3plZMz0Fw==}
-    hasBin: true
-    dependencies:
-      '@aspect-test/b': 5.0.0
-      '@aspect-test/c': 1.0.0
-      '@aspect-test/d': 2.0.0(@aspect-test/c@1.0.0)
-    dev: false
-""")
-
     parsed_json = utils.parse_pnpm_lock_json("""\
 {
   "lockfileVersion": "6.0",
@@ -191,21 +133,18 @@ packages:
         None,
     )
 
-    asserts.equals(env, expected, parsed_yaml)
     asserts.equals(env, expected, parsed_json)
 
     return unittest.end(env)
 
 a_test = unittest.make(_parse_empty_lock_test_impl, attrs = {})
 b_test = unittest.make(_parse_lockfile_v5_test_impl, attrs = {})
 c_test = unittest.make(_parse_lockfile_v6_test_impl, attrs = {})
-d_test = unittest.make(_parse_merge_conflict_test_impl, attrs = {})
 
 TESTS = [
     a_test,
     b_test,
     c_test,
-    d_test,
 ]
 
 def parse_pnpm_lock_tests(name):