From 5b097fd7e5224db6de65847a604f5c60a93667a0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= <biwanczuk@gmail.com>
Date: Mon, 3 Oct 2022 19:10:53 +0200
Subject: [PATCH] fix(npm): better error is version is specified after subpath
 (#16131)

---
 cli/npm/resolution.rs                         | 12 +++++++++++
 cli/tests/integration/npm_tests.rs            |  8 +++++++
 .../npm/error_version_after_subpath/main.js   |  1 +
 .../npm/error_version_after_subpath/main.out  |  2 ++
 ext/node/errors.rs                            | 21 +++++++++++++++----
 5 files changed, 40 insertions(+), 4 deletions(-)
 create mode 100644 cli/tests/testdata/npm/error_version_after_subpath/main.js
 create mode 100644 cli/tests/testdata/npm/error_version_after_subpath/main.out

diff --git a/cli/npm/resolution.rs b/cli/npm/resolution.rs
index d56cc87bc8..497f409065 100644
--- a/cli/npm/resolution.rs
+++ b/cli/npm/resolution.rs
@@ -77,6 +77,18 @@ impl NpmPackageReference {
     } else {
       Some(parts[name_part_len..].join("/"))
     };
+
+    if let Some(sub_path) = &sub_path {
+      if let Some(at_index) = sub_path.rfind('@') {
+        let (new_sub_path, version) = sub_path.split_at(at_index);
+        let msg = format!(
+          "Invalid package specifier 'npm:{}/{}'. Did you mean to write 'npm:{}{}/{}'?",
+          name, sub_path, name, version, new_sub_path
+        );
+        return Err(generic_error(msg));
+      }
+    }
+
     Ok(NpmPackageReference {
       req: NpmPackageReq { name, version_req },
       sub_path,
diff --git a/cli/tests/integration/npm_tests.rs b/cli/tests/integration/npm_tests.rs
index 32ec126569..de3f81674f 100644
--- a/cli/tests/integration/npm_tests.rs
+++ b/cli/tests/integration/npm_tests.rs
@@ -195,6 +195,14 @@ itest!(require_json {
   http_server: true,
 });
 
+itest!(error_version_after_subpath {
+  args: "run --unstable -A --quiet npm/error_version_after_subpath/main.js",
+  output: "npm/error_version_after_subpath/main.out",
+  envs: env_vars(),
+  http_server: true,
+  exit_code: 1,
+});
+
 #[test]
 fn parallel_downloading() {
   let (out, _err) = util::run_and_collect_output_with_args(
diff --git a/cli/tests/testdata/npm/error_version_after_subpath/main.js b/cli/tests/testdata/npm/error_version_after_subpath/main.js
new file mode 100644
index 0000000000..77c7a017cc
--- /dev/null
+++ b/cli/tests/testdata/npm/error_version_after_subpath/main.js
@@ -0,0 +1 @@
+import "npm:react-dom/server@18.2.0";
diff --git a/cli/tests/testdata/npm/error_version_after_subpath/main.out b/cli/tests/testdata/npm/error_version_after_subpath/main.out
new file mode 100644
index 0000000000..0cdd1b6da0
--- /dev/null
+++ b/cli/tests/testdata/npm/error_version_after_subpath/main.out
@@ -0,0 +1,2 @@
+error: Invalid package specifier 'npm:react-dom/server@18.2.0'. Did you mean to write 'npm:react-dom@18.2.0/server'?
+    at [WILDCARD]/npm/error_version_after_subpath/main.js:1:8
diff --git a/ext/node/errors.rs b/ext/node/errors.rs
index 4b55d19822..929f51e1b3 100644
--- a/ext/node/errors.rs
+++ b/ext/node/errors.rs
@@ -81,23 +81,36 @@ pub fn err_invalid_package_target(
 }
 
 pub fn err_package_path_not_exported(
-  pkg_path: String,
+  mut pkg_path: String,
   subpath: String,
   maybe_referrer: Option<String>,
 ) -> AnyError {
   let mut msg = "[ERR_PACKAGE_PATH_NOT_EXPORTED]".to_string();
 
+  #[cfg(windows)]
+  {
+    if !pkg_path.ends_with('\\') {
+      pkg_path.push('\\');
+    }
+  }
+  #[cfg(not(windows))]
+  {
+    if !pkg_path.ends_with('/') {
+      pkg_path.push('/');
+    }
+  }
+
   if subpath == "." {
     msg = format!(
-      "{} No \"exports\" main defined in {}package.json",
+      "{} No \"exports\" main defined in '{}package.json'",
       msg, pkg_path
     );
   } else {
-    msg = format!("{} Package subpath \'{}\' is not defined by \"exports\" in {}package.json", msg, subpath, pkg_path);
+    msg = format!("{} Package subpath '{}' is not defined by \"exports\" in '{}package.json'", msg, subpath, pkg_path);
   };
 
   if let Some(referrer) = maybe_referrer {
-    msg = format!("{} imported from {}", msg, referrer);
+    msg = format!("{} imported from '{}'", msg, referrer);
   }
 
   generic_error(msg)