From 7b2dcad1b9649b9005ba9683d1a0e09e29ef5127 Mon Sep 17 00:00:00 2001
From: Krishnan Chandra <1229365+krishnan-chandra@users.noreply.github.com>
Date: Thu, 11 Jul 2024 14:27:15 -0400
Subject: [PATCH] (JavaScript) Throw a more descriptive error when the package
 name is missing (#21159)

Closes #20859.

While this is a simple fix and addresses the linked issue, there is an
interesting edge case to discuss:

Internal packages do not necessarily have a name field in `package.json`
(see https://github.com/wireapp/wire-desktop/issues/1692,
https://github.com/facebook/react/issues/13107 for examples). The
JavaScript backend in Pants does require that each package.json define a
name, but I'm not so sure that's necessarily the right behavior.

It's worth considering whether we should make names optional in Pants,
given that larger JavaScript monorepos may have internal packages that
are not meant to be published. Furthermore, different JS package
managers handle this situation differently -

* [Bun doesn't handle package.json files without a
name](https://github.com/oven-sh/bun/issues/6317)
* [npm assigns the parent directory name as the
name](https://github.com/npm/cli/issues/2264)
---
 docs/notes/2.23.x.md                               |  5 +++--
 .../pants/backend/javascript/package_json.py       |  6 +++++-
 .../pants/backend/javascript/package_json_test.py  | 14 +++++++++++++-
 3 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/docs/notes/2.23.x.md b/docs/notes/2.23.x.md
index c478919d9ca..a3f3b8818bf 100644
--- a/docs/notes/2.23.x.md
+++ b/docs/notes/2.23.x.md
@@ -123,8 +123,8 @@ pants to invoke `corepack`s default versions of the package managers instead.
 (npm, pnpm, yarn) without providing a corresponding `[nodejs].package_managers` version setting. The version is then
 entirely handled by `corepack`. Previously this mode caused pants to fail.
 
-The internal installation mechanism for node_modules has changed. 
-Previously, Pants installed each package separately in sandboxes and merged the results, creating a node_modules for all dependent packages in the workspace. 
+The internal installation mechanism for node_modules has changed.
+Previously, Pants installed each package separately in sandboxes and merged the results, creating a node_modules for all dependent packages in the workspace.
 Now, this is delegated to the package managers, using each package manager's support for workspaces.
 
 This fixes an issue with integrity file collisions when newer versions of package managers (e.g. the [hidden lockfiles](https://docs.npmjs.com/cli/v9/configuring-npm/package-lock-json#hidden-lockfiles) introduced in npm v7).
@@ -132,6 +132,7 @@ This fixes an issue with integrity file collisions when newer versions of packag
 `pants export --resolve=<js-resolve>` now has basic support for exporting the package manager installation artifacts
 including `node_modules`. This can be used to inspect the installation, or to enable IDE:s to discover the packages.
 
+Pants will output a more helpful error message if there is no `name` field defined in the `package.json` file, or if the `name` field is empty.
 
 #### Shell
 
diff --git a/src/python/pants/backend/javascript/package_json.py b/src/python/pants/backend/javascript/package_json.py
index a406ec2eaeb..191b6693204 100644
--- a/src/python/pants/backend/javascript/package_json.py
+++ b/src/python/pants/backend/javascript/package_json.py
@@ -683,9 +683,13 @@ async def find_owning_package(request: OwningNodePackageRequest) -> OwningNodePa
 @rule
 async def parse_package_json(content: FileContent) -> PackageJson:
     parsed_package_json = FrozenDict.deep_freeze(json.loads(content.content))
+    package_name = parsed_package_json.get("name")
+    if not package_name:
+        raise ValueError("No package name found in package.json")
+
     return PackageJson(
         content=parsed_package_json,
-        name=parsed_package_json["name"],
+        name=package_name,
         version=parsed_package_json.get("version"),
         snapshot=await Get(Snapshot, PathGlobs([content.path])),
         module=parsed_package_json.get("type"),
diff --git a/src/python/pants/backend/javascript/package_json_test.py b/src/python/pants/backend/javascript/package_json_test.py
index 88662d88a9b..2c285225cd6 100644
--- a/src/python/pants/backend/javascript/package_json_test.py
+++ b/src/python/pants/backend/javascript/package_json_test.py
@@ -27,7 +27,7 @@
 from pants.engine.internals.scheduler import ExecutionError
 from pants.engine.rules import QueryRule
 from pants.engine.target import AllTargets
-from pants.testutil.rule_runner import RuleRunner
+from pants.testutil.rule_runner import RuleRunner, engine_error
 from pants.util.frozendict import FrozenDict
 
 
@@ -86,6 +86,18 @@ def test_parses_package_jsons(rule_runner: RuleRunner) -> None:
     }
 
 
+def test_parse_package_json_without_name(rule_runner: RuleRunner) -> None:
+    rule_runner.write_files(
+        {
+            "src/js/BUILD": "package_json()",
+            # No name in package.json, should cause an error.
+            "src/js/package.json": json.dumps({"version": "0.0.1"}),
+        }
+    )
+    with engine_error(ValueError, contains="No package name found in package.json"):
+        rule_runner.request(AllPackageJson, [])
+
+
 def test_generates_third_party_node_package_targets(rule_runner: RuleRunner) -> None:
     rule_runner.write_files(
         {