refactor(ext/node): remove dependency on deno_npm and deno_semver (#20718)

This is required from BYONM (bring your own node_modules).

Part of #18967

Cargo.lock

@@ -1526,8 +1526,6 @@ dependencies = [
  "deno_fs",
  "deno_media_type",
  "deno_net",
- "deno_npm",
- "deno_semver 0.5.1",
  "digest 0.10.7",
  "dsa",
  "ecb",

Cargo.toml

@@ -48,8 +48,6 @@ deno_bench_util = { version = "0.114.0", path = "./bench_util" }
 test_util = { path = "./test_util" }
 deno_lockfile = "0.17.1"
 deno_media_type = { version = "0.1.1", features = ["module_specifier"] }
-deno_npm = "0.15.1"
-deno_semver = "0.5.0"
 
 # exts
 deno_broadcast_channel = { version = "0.114.0", path = "./ext/broadcast_channel" }

cli/Cargo.toml

@@ -54,9 +54,9 @@ deno_emit = "=0.28.0"
 deno_graph = "=0.55.0"
 deno_lint = { version = "=0.51.0", features = ["docs"] }
 deno_lockfile.workspace = true
-deno_npm.workspace = true
+deno_npm = "0.15.1"
 deno_runtime = { workspace = true, features = ["dont_create_runtime_snapshot", "exclude_runtime_main_js", "include_js_files_for_snapshotting"] }
-deno_semver.workspace = true
+deno_semver = "0.5.1"
 deno_task_shell = "=0.13.2"
 eszip = "=0.53.0"
 napi_sym.workspace = true

cli/factory.rs

@@ -623,10 +623,11 @@ impl CliFactory {
     &self,
   ) -> Result<CliMainWorkerFactory, AnyError> {
     let node_resolver = self.node_resolver().await?;
+    let npm_resolver = self.npm_resolver().await?;
     let fs = self.fs();
     Ok(CliMainWorkerFactory::new(
       StorageKeyResolver::from_options(&self.options),
-      self.npm_resolver().await?.clone(),
+      npm_resolver.clone(),
       node_resolver.clone(),
       self.blob_store().clone(),
       Box::new(CliModuleLoaderFactory::new(
@@ -641,6 +642,7 @@ impl CliFactory {
           self.node_code_translator().await?.clone(),
           fs.clone(),
           node_resolver.clone(),
+          npm_resolver.clone(),
         ),
       )),
       self.root_cert_store_provider().clone(),

cli/lsp/analysis.rs

@@ -199,9 +199,8 @@ impl<'a> TsResponseImportMapper<'a> {
     }
 
     if self.npm_resolver.in_npm_package(specifier) {
-      if let Ok(Some(pkg_id)) = self
+      if let Ok(Some(pkg_id)) =
-        .npm_resolver
+        self.npm_resolver.resolve_pkg_id_from_specifier(specifier)
-        .resolve_package_id_from_specifier(specifier)
       {
         let pkg_reqs =
           self.npm_resolution.resolve_pkg_reqs_from_pkg_id(&pkg_id);
@@ -254,7 +253,7 @@ impl<'a> TsResponseImportMapper<'a> {
     let specifier_path = specifier.to_file_path().ok()?;
     let root_folder = self
       .npm_resolver
-      .resolve_package_folder_from_specifier(specifier)
+      .resolve_pkg_folder_from_specifier(specifier)
       .ok()
       .flatten()?;
     let package_json_path = root_folder.join("package.json");

cli/lsp/diagnostics.rs

@@ -800,8 +800,8 @@ fn generate_lint_diagnostics(
       }
 
       // ignore any npm package files
-      if let Some(node_resolver) = &snapshot.maybe_node_resolver {
+      if let Some(npm) = &snapshot.npm {
-        if node_resolver.in_npm_package(document.specifier()) {
+        if npm.node_resolver.in_npm_package(document.specifier()) {
           continue;
         }
       }
@@ -1237,10 +1237,10 @@ fn diagnose_resolution(
       } else if let Ok(pkg_ref) =
         NpmPackageReqReference::from_specifier(specifier)
       {
-        if let Some(npm_resolver) = &snapshot.maybe_npm_resolver {
+        if let Some(npm) = &snapshot.npm {
           // show diagnostics for npm package references that aren't cached
           let req = pkg_ref.into_inner().req;
-          if !npm_resolver.is_pkg_req_folder_cached(&req) {
+          if !npm.npm_resolver.is_pkg_req_folder_cached(&req) {
             diagnostics
               .push(DenoDiagnostic::NoCacheNpm(req, specifier.clone()));
           }
@@ -1250,10 +1250,10 @@ fn diagnose_resolution(
         if !deno_node::is_builtin_node_module(module_name) {
           diagnostics
             .push(DenoDiagnostic::InvalidNodeSpecifier(specifier.clone()));
-        } else if let Some(npm_resolver) = &snapshot.maybe_npm_resolver {
+        } else if let Some(npm) = &snapshot.npm {
           // check that a @types/node package exists in the resolver
           let types_node_req = PackageReq::from_str("@types/node").unwrap();
-          if !npm_resolver.is_pkg_req_folder_cached(&types_node_req) {
+          if !npm.npm_resolver.is_pkg_req_folder_cached(&types_node_req) {
             diagnostics.push(DenoDiagnostic::NoCacheNpm(
               types_node_req,
               ModuleSpecifier::parse("npm:@types/node").unwrap(),
@@ -1291,8 +1291,8 @@ fn diagnose_dependency(
   dependency_key: &str,
   dependency: &deno_graph::Dependency,
 ) {
-  if let Some(npm_resolver) = &snapshot.maybe_npm_resolver {
+  if let Some(npm) = &snapshot.npm {
-    if npm_resolver.in_npm_package(referrer) {
+    if npm.npm_resolver.in_npm_package(referrer) {
       return; // ignore, surface typescript errors instead
     }
   }
@@ -1461,8 +1461,7 @@ mod tests {
         GlobalHttpCache::new(location.to_path_buf(), RealDenoCacheEnv),
       )),
       config: Default::default(),
-      maybe_node_resolver: None,
+      npm: None,
-      maybe_npm_resolver: None,
     }
   }
 

cli/lsp/documents.rs

@@ -2,6 +2,7 @@
 
 use super::cache::calculate_fs_version;
 use super::cache::calculate_fs_version_at_path;
+use super::language_server::StateNpmSnapshot;
 use super::text::LineIndex;
 use super::tsc;
 use super::tsc::AssetDocument;
@@ -41,7 +42,6 @@ use deno_graph::Resolution;
 use deno_runtime::deno_node;
 use deno_runtime::deno_node::NodeResolution;
 use deno_runtime::deno_node::NodeResolutionMode;
-use deno_runtime::deno_node::NodeResolver;
 use deno_runtime::deno_node::PackageJson;
 use deno_runtime::permissions::PermissionsContainer;
 use deno_semver::npm::NpmPackageReqReference;
@@ -1089,7 +1089,7 @@ impl Documents {
     &self,
     specifiers: Vec<String>,
     referrer_doc: &AssetOrDocument,
-    maybe_node_resolver: Option<&Arc<NodeResolver>>,
+    maybe_npm: Option<&StateNpmSnapshot>,
   ) -> Vec<Option<(ModuleSpecifier, MediaType)>> {
     let referrer = referrer_doc.specifier();
     let dependencies = match referrer_doc {
@@ -1098,11 +1098,12 @@ impl Documents {
     };
     let mut results = Vec::new();
     for specifier in specifiers {
-      if let Some(node_resolver) = maybe_node_resolver {
+      if let Some(npm) = maybe_npm {
-        if node_resolver.in_npm_package(referrer) {
+        if npm.node_resolver.in_npm_package(referrer) {
           // we're in an npm package, so use node resolution
           results.push(Some(NodeResolution::into_specifier_and_media_type(
-            node_resolver
+            npm
+              .node_resolver
               .resolve(
                 &specifier,
                 referrer,
@@ -1126,9 +1127,9 @@ impl Documents {
         dependencies.as_ref().and_then(|d| d.deps.get(&specifier))
       {
         if let Some(specifier) = dep.maybe_type.maybe_specifier() {
-          results.push(self.resolve_dependency(specifier, maybe_node_resolver));
+          results.push(self.resolve_dependency(specifier, maybe_npm));
         } else if let Some(specifier) = dep.maybe_code.maybe_specifier() {
-          results.push(self.resolve_dependency(specifier, maybe_node_resolver));
+          results.push(self.resolve_dependency(specifier, maybe_npm));
         } else {
           results.push(None);
         }
@@ -1136,12 +1137,11 @@ impl Documents {
         .resolve_imports_dependency(&specifier)
         .and_then(|r| r.maybe_specifier())
       {
-        results.push(self.resolve_dependency(specifier, maybe_node_resolver));
+        results.push(self.resolve_dependency(specifier, maybe_npm));
       } else if let Ok(npm_req_ref) =
         NpmPackageReqReference::from_str(&specifier)
       {
-        results
+        results.push(node_resolve_npm_req_ref(npm_req_ref, maybe_npm));
-          .push(node_resolve_npm_req_ref(npm_req_ref, maybe_node_resolver));
       } else {
         results.push(None);
       }
@@ -1475,7 +1475,7 @@ impl Documents {
   fn resolve_dependency(
     &self,
     specifier: &ModuleSpecifier,
-    maybe_node_resolver: Option<&Arc<NodeResolver>>,
+    maybe_npm: Option<&StateNpmSnapshot>,
   ) -> Option<(ModuleSpecifier, MediaType)> {
     if let Some(module_name) = specifier.as_str().strip_prefix("node:") {
       if deno_node::is_builtin_node_module(module_name) {
@@ -1487,7 +1487,7 @@ impl Documents {
     }
 
     if let Ok(npm_ref) = NpmPackageReqReference::from_specifier(specifier) {
-      return node_resolve_npm_req_ref(npm_ref, maybe_node_resolver);
+      return node_resolve_npm_req_ref(npm_ref, maybe_npm);
     }
     let doc = self.get(specifier)?;
     let maybe_module = doc.maybe_esm_module().and_then(|r| r.as_ref().ok());
@@ -1496,7 +1496,7 @@ impl Documents {
     if let Some(specifier) =
       maybe_types_dependency.and_then(|d| d.maybe_specifier())
     {
-      self.resolve_dependency(specifier, maybe_node_resolver)
+      self.resolve_dependency(specifier, maybe_npm)
     } else {
       let media_type = doc.media_type();
       Some((doc.specifier().clone(), media_type))
@@ -1519,18 +1519,26 @@ impl Documents {
 
 fn node_resolve_npm_req_ref(
   npm_req_ref: NpmPackageReqReference,
-  maybe_node_resolver: Option<&Arc<NodeResolver>>,
+  maybe_npm: Option<&StateNpmSnapshot>,
 ) -> Option<(ModuleSpecifier, MediaType)> {
-  maybe_node_resolver.map(|node_resolver| {
+  maybe_npm.map(|npm| {
     NodeResolution::into_specifier_and_media_type(
-      node_resolver
+      npm
-        .resolve_npm_req_reference(
+        .npm_resolver
-          &npm_req_ref,
+        .resolve_pkg_folder_from_deno_module_req(npm_req_ref.req())
-          NodeResolutionMode::Types,
-          &PermissionsContainer::allow_all(),
-        )
         .ok()
-        .flatten(),
+        .and_then(|package_folder| {
+          npm
+            .node_resolver
+            .resolve_npm_reference(
+              &package_folder,
+              npm_req_ref.sub_path(),
+              NodeResolutionMode::Types,
+              &PermissionsContainer::allow_all(),
+            )
+            .ok()
+            .flatten()
+        }),
     )
   })
 }

cli/lsp/language_server.rs

@@ -158,6 +158,12 @@ impl LspNpmConfigHash {
 #[derive(Debug, Clone)]
 pub struct LanguageServer(Arc<tokio::sync::RwLock<Inner>>);
 
+#[derive(Debug)]
+pub struct StateNpmSnapshot {
+  pub node_resolver: Arc<NodeResolver>,
+  pub npm_resolver: Arc<CliNpmResolver>,
+}
+
 /// Snapshot of the state used by TSC.
 #[derive(Debug)]
 pub struct StateSnapshot {
@@ -166,8 +172,7 @@ pub struct StateSnapshot {
   pub config: Arc<ConfigSnapshot>,
   pub documents: Documents,
   pub maybe_import_map: Option<Arc<ImportMap>>,
-  pub maybe_node_resolver: Option<Arc<NodeResolver>>,
+  pub npm: Option<StateNpmSnapshot>,
-  pub maybe_npm_resolver: Option<Arc<CliNpmResolver>>,
 }
 
 #[derive(Debug)]
@@ -819,8 +824,10 @@ impl Inner {
       config: self.config.snapshot(),
       documents: self.documents.clone(),
       maybe_import_map: self.maybe_import_map.clone(),
-      maybe_node_resolver: Some(node_resolver),
+      npm: Some(StateNpmSnapshot {
-      maybe_npm_resolver: Some(npm_resolver),
+        node_resolver,
+        npm_resolver,
+      }),
     })
   }
 

cli/lsp/tsc.rs

@@ -3293,9 +3293,9 @@ fn op_is_node_file(state: &mut OpState, #[string] path: String) -> bool {
   match ModuleSpecifier::parse(&path) {
     Ok(specifier) => state
       .state_snapshot
-      .maybe_npm_resolver
+      .npm
       .as_ref()
-      .map(|r| r.in_npm_package(&specifier))
+      .map(|n| n.npm_resolver.in_npm_package(&specifier))
       .unwrap_or(false),
     Err(_) => false,
   }
@@ -3341,7 +3341,7 @@ fn op_resolve(
       let resolved = state.state_snapshot.documents.resolve(
         args.specifiers,
         &referrer_doc,
-        state.state_snapshot.maybe_node_resolver.as_ref(),
+        state.state_snapshot.npm.as_ref(),
       );
       Ok(
         resolved
@@ -3477,8 +3477,7 @@ deno_core::extension!(deno_tsc,
         config: Default::default(),
         documents: Documents::new(options.cache.clone()),
         maybe_import_map: None,
-        maybe_node_resolver: None,
+        npm: None,
-        maybe_npm_resolver: None,
       }),
       options.performance,
     ));
@@ -4304,8 +4303,7 @@ mod tests {
       cache_metadata: CacheMetadata::new(cache),
       config: Default::default(),
       maybe_import_map: None,
-      maybe_node_resolver: None,
+      npm: None,
-      maybe_npm_resolver: None,
     }
   }
 

cli/module_loader.rs

@@ -12,6 +12,7 @@ use crate::graph_util::ModuleGraphBuilder;
 use crate::graph_util::ModuleGraphContainer;
 use crate::node;
 use crate::node::CliNodeCodeTranslator;
+use crate::npm::CliNpmResolver;
 use crate::resolver::CliGraphResolver;
 use crate::tools::check;
 use crate::tools::check::TypeChecker;
@@ -646,6 +647,7 @@ pub struct NpmModuleLoader {
   node_code_translator: Arc<CliNodeCodeTranslator>,
   fs: Arc<dyn deno_fs::FileSystem>,
   node_resolver: Arc<NodeResolver>,
+  npm_resolver: Arc<CliNpmResolver>,
 }
 
 impl NpmModuleLoader {
@@ -654,12 +656,14 @@ impl NpmModuleLoader {
     node_code_translator: Arc<CliNodeCodeTranslator>,
     fs: Arc<dyn deno_fs::FileSystem>,
     node_resolver: Arc<NodeResolver>,
+    npm_resolver: Arc<CliNpmResolver>,
   ) -> Self {
     Self {
       cjs_resolutions,
       node_code_translator,
       fs,
       node_resolver,
+      npm_resolver,
     }
   }
 
@@ -693,9 +697,13 @@ impl NpmModuleLoader {
     nv_ref: &NpmPackageNvReference,
     permissions: &PermissionsContainer,
   ) -> Result<ModuleSpecifier, AnyError> {
+    let package_folder = self
+      .npm_resolver
+      .resolve_pkg_folder_from_deno_module(nv_ref.nv())?;
     self
       .handle_node_resolve_result(self.node_resolver.resolve_npm_reference(
-        nv_ref,
+        &package_folder,
+        nv_ref.sub_path(),
         NodeResolutionMode::Execution,
         permissions,
       ))
@@ -704,16 +712,20 @@ impl NpmModuleLoader {
 
   pub fn resolve_req_reference(
     &self,
-    reference: &NpmPackageReqReference,
+    req_ref: &NpmPackageReqReference,
     permissions: &PermissionsContainer,
   ) -> Result<ModuleSpecifier, AnyError> {
+    let package_folder = self
+      .npm_resolver
+      .resolve_pkg_folder_from_deno_module_req(req_ref.req())?;
     self
-      .handle_node_resolve_result(self.node_resolver.resolve_npm_req_reference(
+      .handle_node_resolve_result(self.node_resolver.resolve_npm_reference(
-        reference,
+        &package_folder,
+        req_ref.sub_path(),
         NodeResolutionMode::Execution,
         permissions,
       ))
-      .with_context(|| format!("Could not resolve '{reference}'."))
+      .with_context(|| format!("Could not resolve '{}'.", req_ref))
   }
 
   pub fn maybe_prepare_load(

cli/npm/installer.rs

@@ -105,7 +105,7 @@ impl PackageJsonDepsInstaller {
       let (req, info) = result?;
       let result = inner
         .npm_resolution
-        .resolve_package_req_as_pending_with_info(req, &info);
+        .resolve_pkg_req_as_pending_with_info(req, &info);
       if let Err(err) = result {
         if inner.npm_registry_api.mark_force_reload() {
           log::debug!("Failed to resolve package. Retrying. Error: {err:#}");

cli/npm/resolution.rs

@@ -224,19 +224,19 @@ impl NpmResolution {
   /// Resolves a package requirement for deno graph. This should only be
   /// called by deno_graph's NpmResolver or for resolving packages in
   /// a package.json
-  pub fn resolve_package_req_as_pending(
+  pub fn resolve_pkg_req_as_pending(
     &self,
     pkg_req: &PackageReq,
   ) -> Result<PackageNv, NpmPackageVersionResolutionError> {
     // we should always have this because it should have been cached before here
     let package_info = self.api.get_cached_package_info(&pkg_req.name).unwrap();
-    self.resolve_package_req_as_pending_with_info(pkg_req, &package_info)
+    self.resolve_pkg_req_as_pending_with_info(pkg_req, &package_info)
   }
 
   /// Resolves a package requirement for deno graph. This should only be
   /// called by deno_graph's NpmResolver or for resolving packages in
   /// a package.json
-  pub fn resolve_package_req_as_pending_with_info(
+  pub fn resolve_pkg_req_as_pending_with_info(
     &self,
     pkg_req: &PackageReq,
     package_info: &NpmPackageInfo,

cli/npm/resolvers/mod.rs

@@ -23,7 +23,10 @@ use deno_runtime::deno_fs::FileSystem;
 use deno_runtime::deno_node::NodePermissions;
 use deno_runtime::deno_node::NodeResolutionMode;
 use deno_runtime::deno_node::NpmResolver;
+use deno_semver::npm::NpmPackageNvReference;
+use deno_semver::npm::NpmPackageReqReference;
 use deno_semver::package::PackageNv;
+use deno_semver::package::PackageNvReference;
 use deno_semver::package::PackageReq;
 use global::GlobalNpmPackageResolver;
 use serde::Deserialize;
@@ -98,6 +101,19 @@ impl CliNpmResolver {
       .unwrap_or(false)
   }
 
+  pub fn resolve_pkg_nv_ref_from_pkg_req_ref(
+    &self,
+    req_ref: &NpmPackageReqReference,
+  ) -> Result<NpmPackageNvReference, PackageReqNotFoundError> {
+    let pkg_nv = self
+      .resolve_pkg_id_from_pkg_req(req_ref.req())
+      .map(|id| id.nv)?;
+    Ok(NpmPackageNvReference::new(PackageNvReference {
+      nv: pkg_nv,
+      sub_path: req_ref.sub_path().map(|s| s.to_string()),
+    }))
+  }
+
   pub fn resolve_pkg_id_from_pkg_req(
     &self,
     req: &PackageReq,
@@ -127,7 +143,7 @@ impl CliNpmResolver {
   /// Resolve the root folder of the package the provided specifier is in.
   ///
   /// This will error when the provided specifier is not in an npm package.
-  pub fn resolve_package_folder_from_specifier(
+  pub fn resolve_pkg_folder_from_specifier(
     &self,
     specifier: &ModuleSpecifier,
   ) -> Result<Option<PathBuf>, AnyError> {
@@ -145,8 +161,24 @@ impl CliNpmResolver {
     Ok(Some(path))
   }
 
+  pub fn resolve_pkg_folder_from_deno_module_req(
+    &self,
+    req: &PackageReq,
+  ) -> Result<PathBuf, AnyError> {
+    let pkg_id = self.resolve_pkg_id_from_pkg_req(req)?;
+    self.resolve_pkg_folder_from_pkg_id(&pkg_id)
+  }
+
+  pub fn resolve_pkg_folder_from_deno_module(
+    &self,
+    nv: &PackageNv,
+  ) -> Result<PathBuf, AnyError> {
+    let pkg_id = self.resolution.resolve_pkg_id_from_deno_module(nv)?;
+    self.resolve_pkg_folder_from_pkg_id(&pkg_id)
+  }
+
   /// Resolves the package nv from the provided specifier.
-  pub fn resolve_package_id_from_specifier(
+  pub fn resolve_pkg_id_from_specifier(
     &self,
     specifier: &ModuleSpecifier,
   ) -> Result<Option<NpmPackageId>, AnyError> {
@@ -266,22 +298,7 @@ impl NpmResolver for CliNpmResolver {
     &self,
     specifier: &ModuleSpecifier,
   ) -> Result<Option<PathBuf>, AnyError> {
-    self.resolve_package_folder_from_specifier(specifier)
+    self.resolve_pkg_folder_from_specifier(specifier)
-  }
-
-  fn resolve_package_folder_from_deno_module(
-    &self,
-    pkg_nv: &PackageNv,
-  ) -> Result<PathBuf, AnyError> {
-    let pkg_id = self.resolution.resolve_pkg_id_from_deno_module(pkg_nv)?;
-    self.resolve_pkg_folder_from_pkg_id(&pkg_id)
-  }
-
-  fn resolve_pkg_id_from_pkg_req(
-    &self,
-    req: &PackageReq,
-  ) -> Result<NpmPackageId, PackageReqNotFoundError> {
-    self.resolution.resolve_pkg_id_from_pkg_req(req)
   }
 
   fn in_npm_package(&self, specifier: &ModuleSpecifier) -> bool {

cli/resolver.rs

@@ -319,9 +319,7 @@ impl NpmResolver for CliGraphResolver {
       ));
     }
 
-    let result = self
+    let result = self.npm_resolution.resolve_pkg_req_as_pending(package_req);
-      .npm_resolution
-      .resolve_package_req_as_pending(package_req);
     match result {
       Ok(nv) => NpmPackageReqResolution::Ok(nv),
       Err(err) => {

cli/standalone/mod.rs

@@ -404,6 +404,7 @@ pub async fn run(
         node_code_translator,
         fs.clone(),
         node_resolver.clone(),
+        npm_resolver.clone(),
       )),
     }),
   };
@@ -429,7 +430,7 @@ pub async fn run(
   };
   let worker_factory = CliMainWorkerFactory::new(
     StorageKeyResolver::empty(),
-    npm_resolver.clone(),
+    npm_resolver,
     node_resolver,
     Default::default(),
     Box::new(module_loader_factory),

cli/tests/testdata/npm/deno_run_no_bin_entrypoint.out

@@ -1 +1 @@
-error: package '@denotest/esm-basic' did not have a bin property in its package.json
+error: '[WILDCARD]@denotest[WILDCARD]esm-basic[WILDCARD]package.json' did not have a bin property

cli/tests/testdata/npm/deno_run_no_bin_entrypoint_non_existent_subpath.out

@@ -1,3 +1,3 @@
-error: package '@denotest/esm-basic' did not have a bin property in its package.json
+error: '[WILDCARD]@denotest[WILDCARD]esm-basic[WILDCARD]package.json' did not have a bin property
 
 Fallback failed: Cannot find module 'file:///[WILDCARD]/non-existent.js'

cli/tools/check.rs

@@ -138,7 +138,10 @@ impl TypeChecker {
       debug,
       graph: graph.clone(),
       hash_data,
-      maybe_node_resolver: Some(self.node_resolver.clone()),
+      maybe_npm: Some(tsc::RequestNpmState {
+        node_resolver: self.node_resolver.clone(),
+        npm_resolver: self.npm_resolver.clone(),
+      }),
       maybe_tsbuildinfo,
       root_names,
       check_mode: type_check_mode,

cli/tools/task.rs

@@ -268,7 +268,9 @@ fn resolve_npm_commands(
   let mut result = HashMap::new();
   let snapshot = npm_resolver.snapshot();
   for id in snapshot.top_level_packages() {
-    let bin_commands = node_resolver.resolve_binary_commands(&id.nv)?;
+    let package_folder = npm_resolver.resolve_pkg_folder_from_pkg_id(id)?;
+    let bin_commands =
+      node_resolver.resolve_binary_commands(&package_folder)?;
     for bin_command in bin_commands {
       result.insert(
         bin_command.to_string(),

cli/tsc/mod.rs

@@ -4,6 +4,7 @@ use crate::args::TsConfig;
 use crate::args::TypeCheckMode;
 use crate::cache::FastInsecureHasher;
 use crate::node;
+use crate::npm::CliNpmResolver;
 use crate::util::checksum;
 use crate::util::path::mapped_specifier_for_tsc;
 
@@ -293,6 +294,12 @@ pub struct EmittedFile {
   pub media_type: MediaType,
 }
 
+#[derive(Debug)]
+pub struct RequestNpmState {
+  pub node_resolver: Arc<NodeResolver>,
+  pub npm_resolver: Arc<CliNpmResolver>,
+}
+
 /// A structure representing a request to be sent to the tsc runtime.
 #[derive(Debug)]
 pub struct Request {
@@ -303,7 +310,7 @@ pub struct Request {
   pub debug: bool,
   pub graph: Arc<ModuleGraph>,
   pub hash_data: u64,
-  pub maybe_node_resolver: Option<Arc<NodeResolver>>,
+  pub maybe_npm: Option<RequestNpmState>,
   pub maybe_tsbuildinfo: Option<String>,
   /// A vector of strings that represent the root/entry point modules for the
   /// program.
@@ -327,7 +334,7 @@ struct State {
   graph: Arc<ModuleGraph>,
   maybe_tsbuildinfo: Option<String>,
   maybe_response: Option<RespondArgs>,
-  maybe_node_resolver: Option<Arc<NodeResolver>>,
+  maybe_npm: Option<RequestNpmState>,
   remapped_specifiers: HashMap<String, ModuleSpecifier>,
   root_map: HashMap<String, ModuleSpecifier>,
   current_dir: PathBuf,
@@ -340,7 +347,7 @@ impl Default for State {
       graph: Arc::new(ModuleGraph::new(GraphKind::All)),
       maybe_tsbuildinfo: Default::default(),
       maybe_response: Default::default(),
-      maybe_node_resolver: Default::default(),
+      maybe_npm: Default::default(),
       remapped_specifiers: Default::default(),
       root_map: Default::default(),
       current_dir: Default::default(),
@@ -352,7 +359,7 @@ impl State {
   pub fn new(
     graph: Arc<ModuleGraph>,
     hash_data: u64,
-    maybe_node_resolver: Option<Arc<NodeResolver>>,
+    maybe_npm: Option<RequestNpmState>,
     maybe_tsbuildinfo: Option<String>,
     root_map: HashMap<String, ModuleSpecifier>,
     remapped_specifiers: HashMap<String, ModuleSpecifier>,
@@ -361,7 +368,7 @@ impl State {
     State {
       hash_data,
       graph,
-      maybe_node_resolver,
+      maybe_npm,
       maybe_tsbuildinfo,
       maybe_response: None,
       remapped_specifiers,
@@ -496,9 +503,9 @@ fn op_load(state: &mut OpState, args: Value) -> Result<Value, AnyError> {
         }
       }
     } else if state
-      .maybe_node_resolver
+      .maybe_npm
       .as_ref()
-      .map(|resolver| resolver.in_npm_package(specifier))
+      .map(|npm| npm.node_resolver.in_npm_package(specifier))
       .unwrap_or(false)
     {
       media_type = MediaType::from_specifier(specifier);
@@ -650,9 +657,13 @@ fn resolve_graph_specifier_types(
       Ok(Some((module.specifier.clone(), module.media_type)))
     }
     Some(Module::Npm(module)) => {
-      if let Some(node_resolver) = &state.maybe_node_resolver {
+      if let Some(npm) = &state.maybe_npm {
-        let maybe_resolution = node_resolver.resolve_npm_reference(
+        let package_folder = npm
-          &module.nv_reference,
+          .npm_resolver
+          .resolve_pkg_folder_from_deno_module(module.nv_reference.nv())?;
+        let maybe_resolution = npm.node_resolver.resolve_npm_reference(
+          &package_folder,
+          module.nv_reference.sub_path(),
           NodeResolutionMode::Types,
           &PermissionsContainer::allow_all(),
         )?;
@@ -665,11 +676,11 @@ fn resolve_graph_specifier_types(
     }
     Some(Module::External(module)) => {
       // we currently only use "External" for when the module is in an npm package
-      Ok(state.maybe_node_resolver.as_ref().map(|node_resolver| {
+      Ok(state.maybe_npm.as_ref().map(|npm| {
         let specifier =
           node::resolve_specifier_into_node_modules(&module.specifier);
         NodeResolution::into_specifier_and_media_type(
-          node_resolver.url_to_node_resolution(specifier).ok(),
+          npm.node_resolver.url_to_node_resolution(specifier).ok(),
         )
       }))
     }
@@ -682,10 +693,11 @@ fn resolve_non_graph_specifier_types(
   referrer: &ModuleSpecifier,
   state: &State,
 ) -> Result<Option<(ModuleSpecifier, MediaType)>, AnyError> {
-  let node_resolver = match state.maybe_node_resolver.as_ref() {
+  let npm = match state.maybe_npm.as_ref() {
-    Some(node_resolver) => node_resolver,
+    Some(npm) => npm,
     None => return Ok(None), // we only support non-graph types for npm packages
   };
+  let node_resolver = &npm.node_resolver;
   if node_resolver.in_npm_package(referrer) {
     // we're in an npm package, so use node resolution
     Ok(Some(NodeResolution::into_specifier_and_media_type(
@@ -699,13 +711,17 @@ fn resolve_non_graph_specifier_types(
         .ok()
         .flatten(),
     )))
-  } else if let Ok(npm_ref) = NpmPackageReqReference::from_str(specifier) {
+  } else if let Ok(npm_req_ref) = NpmPackageReqReference::from_str(specifier) {
     // todo(dsherret): add support for injecting this in the graph so
     // we don't need this special code here.
     // This could occur when resolving npm:@types/node when it is
     // injected and not part of the graph
-    let maybe_resolution = node_resolver.resolve_npm_req_reference(
+    let package_folder = npm
-      &npm_ref,
+      .npm_resolver
+      .resolve_pkg_folder_from_deno_module_req(npm_req_ref.req())?;
+    let maybe_resolution = node_resolver.resolve_npm_reference(
+      &package_folder,
+      npm_req_ref.sub_path(),
       NodeResolutionMode::Types,
       &PermissionsContainer::allow_all(),
     )?;
@@ -722,9 +738,9 @@ fn op_is_node_file(state: &mut OpState, #[string] path: &str) -> bool {
   let state = state.borrow::<State>();
   match ModuleSpecifier::parse(path) {
     Ok(specifier) => state
-      .maybe_node_resolver
+      .maybe_npm
       .as_ref()
-      .map(|r| r.in_npm_package(&specifier))
+      .map(|n| n.node_resolver.in_npm_package(&specifier))
       .unwrap_or(false),
     Err(_) => false,
   }
@@ -783,7 +799,7 @@ pub fn exec(request: Request) -> Result<Response, AnyError> {
       state.put(State::new(
         options.request.graph,
         options.request.hash_data,
-        options.request.maybe_node_resolver,
+        options.request.maybe_npm,
         options.request.maybe_tsbuildinfo,
         options.root_map,
         options.remapped_specifiers,
@@ -952,7 +968,7 @@ mod tests {
       debug: false,
       graph: Arc::new(graph),
       hash_data,
-      maybe_node_resolver: None,
+      maybe_npm: None,
       maybe_tsbuildinfo: None,
       root_names: vec![(specifier.clone(), MediaType::TypeScript)],
       check_mode: TypeCheckMode::All,

cli/worker.rs

@@ -39,6 +39,7 @@ use deno_runtime::worker::MainWorker;
 use deno_runtime::worker::WorkerOptions;
 use deno_runtime::BootstrapOptions;
 use deno_runtime::WorkerLogLevel;
+use deno_semver::npm::NpmPackageNvReference;
 use deno_semver::npm::NpmPackageReqReference;
 use deno_semver::package::PackageReqReference;
 
@@ -386,6 +387,9 @@ impl CliMainWorkerFactory {
         .npm_resolver
         .add_package_reqs(&[package_ref.req().clone()])
         .await?;
+      let package_ref = shared
+        .npm_resolver
+        .resolve_pkg_nv_ref_from_pkg_req_ref(&package_ref)?;
       let node_resolution =
         self.resolve_binary_entrypoint(&package_ref, &permissions)?;
       let is_main_cjs = matches!(node_resolution, NodeResolution::CommonJs(_));
@@ -511,10 +515,18 @@ impl CliMainWorkerFactory {
 
   fn resolve_binary_entrypoint(
     &self,
-    package_ref: &NpmPackageReqReference,
+    package_ref: &NpmPackageNvReference,
     permissions: &PermissionsContainer,
   ) -> Result<NodeResolution, AnyError> {
-    match self.shared.node_resolver.resolve_binary_export(package_ref) {
+    let package_folder = self
+      .shared
+      .npm_resolver
+      .resolve_pkg_folder_from_deno_module(package_ref.nv())?;
+    match self
+      .shared
+      .node_resolver
+      .resolve_binary_export(&package_folder, package_ref.sub_path())
+    {
       Ok(node_resolution) => Ok(node_resolution),
       Err(original_err) => {
         // if the binary entrypoint was not found, fallback to regular node resolution
@@ -534,7 +546,7 @@ impl CliMainWorkerFactory {
   /// resolve the binary entrypoint using regular node resolution
   fn resolve_binary_entrypoint_fallback(
     &self,
-    package_ref: &NpmPackageReqReference,
+    package_ref: &NpmPackageNvReference,
     permissions: &PermissionsContainer,
   ) -> Result<Option<NodeResolution>, AnyError> {
     // only fallback if the user specified a sub path
@@ -545,12 +557,16 @@ impl CliMainWorkerFactory {
       return Ok(None);
     }
 
-    let Some(resolution) =
+    let package_folder = self
-      self.shared.node_resolver.resolve_npm_req_reference(
+      .shared
-        package_ref,
+      .npm_resolver
-        NodeResolutionMode::Execution,
+      .resolve_pkg_folder_from_deno_module(package_ref.nv())?;
-        permissions,
+    let Some(resolution) = self.shared.node_resolver.resolve_npm_reference(
-      )?
+      &package_folder,
+      package_ref.sub_path(),
+      NodeResolutionMode::Execution,
+      permissions,
+    )?
     else {
       return Ok(None);
     };

ext/node/Cargo.toml

@@ -25,8 +25,6 @@ deno_fetch.workspace = true
 deno_fs.workspace = true
 deno_media_type.workspace = true
 deno_net.workspace = true
-deno_npm.workspace = true
-deno_semver.workspace = true
 digest = { version = "0.10.5", features = ["core-api", "std"] }
 dsa = "0.6.1"
 ecb.workspace = true

ext/node/lib.rs

@@ -16,10 +16,6 @@ use deno_core::JsRuntime;
 use deno_core::ModuleSpecifier;
 use deno_fs::sync::MaybeSend;
 use deno_fs::sync::MaybeSync;
-use deno_npm::resolution::PackageReqNotFoundError;
-use deno_npm::NpmPackageId;
-use deno_semver::package::PackageNv;
-use deno_semver::package::PackageReq;
 use once_cell::sync::Lazy;
 
 pub mod analyze;
@@ -90,17 +86,6 @@ pub trait NpmResolver: std::fmt::Debug + MaybeSend + MaybeSync {
     specifier: &ModuleSpecifier,
   ) -> Result<Option<PathBuf>, AnyError>;
 
-  /// Resolves an npm package folder path from a Deno module.
-  fn resolve_package_folder_from_deno_module(
-    &self,
-    pkg_nv: &PackageNv,
-  ) -> Result<PathBuf, AnyError>;
-
-  fn resolve_pkg_id_from_pkg_req(
-    &self,
-    req: &PackageReq,
-  ) -> Result<NpmPackageId, PackageReqNotFoundError>;
-
   fn in_npm_package(&self, specifier: &ModuleSpecifier) -> bool;
 
   fn in_npm_package_at_path(&self, path: &Path) -> bool {

ext/node/package_json.rs

@@ -106,7 +106,13 @@ impl PackageJson {
   ) -> Result<PackageJson, AnyError> {
     let package_json: Value = serde_json::from_str(&source)
       .map_err(|err| anyhow::anyhow!("malformed package.json {}", err))?;
+    Self::load_from_value(path, package_json)
+  }
 
+  pub fn load_from_value(
+    path: PathBuf,
+    package_json: serde_json::Value,
+  ) -> Result<PackageJson, AnyError> {
     let imports_val = package_json.get("imports");
     let main_val = package_json.get("main");
     let module_val = package_json.get("module");

ext/node/resolution.rs

@@ -15,10 +15,6 @@ use deno_core::url::Url;
 use deno_core::ModuleSpecifier;
 use deno_fs::FileSystemRc;
 use deno_media_type::MediaType;
-use deno_semver::npm::NpmPackageNvReference;
-use deno_semver::npm::NpmPackageReqReference;
-use deno_semver::package::PackageNv;
-use deno_semver::package::PackageNvReference;
 
 use crate::errors;
 use crate::AllowAllNodePermissions;
@@ -329,46 +325,30 @@ impl NodeResolver {
     Ok(resolved)
   }
 
-  pub fn resolve_npm_req_reference(
-    &self,
-    reference: &NpmPackageReqReference,
-    mode: NodeResolutionMode,
-    permissions: &dyn NodePermissions,
-  ) -> Result<Option<NodeResolution>, AnyError> {
-    let pkg_id = self
-      .npm_resolver
-      .resolve_pkg_id_from_pkg_req(reference.req())?;
-    let reference = NpmPackageNvReference::new(PackageNvReference {
-      nv: pkg_id.nv,
-      sub_path: reference.sub_path().map(ToOwned::to_owned),
-    });
-    self.resolve_npm_reference(&reference, mode, permissions)
-  }
-
   pub fn resolve_npm_reference(
     &self,
-    reference: &NpmPackageNvReference,
+    package_folder: &Path,
+    sub_path: Option<&str>,
     mode: NodeResolutionMode,
     permissions: &dyn NodePermissions,
   ) -> Result<Option<NodeResolution>, AnyError> {
-    let package_folder = self
-      .npm_resolver
-      .resolve_package_folder_from_deno_module(reference.nv())?;
     let node_module_kind = NodeModuleKind::Esm;
     let maybe_resolved_path = self
       .package_config_resolve(
-        &reference
+        &sub_path
-          .sub_path()
           .map(|s| format!("./{s}"))
           .unwrap_or_else(|| ".".to_string()),
-        &package_folder,
+        package_folder,
         node_module_kind,
         DEFAULT_CONDITIONS,
         mode,
         permissions,
       )
       .with_context(|| {
-        format!("Failed resolving package config for '{reference}'")
+        format!(
+          "Failed resolving package config for '{}'",
+          package_folder.join("package.json").display()
+        )
       })?;
     let resolved_path = match maybe_resolved_path {
       Some(resolved_path) => resolved_path,
@@ -392,17 +372,19 @@ impl NodeResolver {
 
   pub fn resolve_binary_commands(
     &self,
-    pkg_nv: &PackageNv,
+    package_folder: &Path,
   ) -> Result<Vec<String>, AnyError> {
-    let package_folder = self
-      .npm_resolver
-      .resolve_package_folder_from_deno_module(pkg_nv)?;
     let package_json_path = package_folder.join("package.json");
-    let package_json =
+    let package_json = self
-      self.load_package_json(&AllowAllNodePermissions, package_json_path)?;
+      .load_package_json(&AllowAllNodePermissions, package_json_path.clone())?;
 
     Ok(match package_json.bin {
-      Some(Value::String(_)) => vec![pkg_nv.name.to_string()],
+      Some(Value::String(_)) => {
+        let Some(name) = &package_json.name else {
+          bail!("'{}' did not have a name", package_json_path.display());
+        };
+        vec![name.to_string()]
+      }
       Some(Value::Object(o)) => {
         o.into_iter().map(|(key, _)| key).collect::<Vec<_>>()
       }
@@ -412,27 +394,13 @@ impl NodeResolver {
 
   pub fn resolve_binary_export(
     &self,
-    pkg_ref: &NpmPackageReqReference,
+    package_folder: &Path,
+    sub_path: Option<&str>,
   ) -> Result<NodeResolution, AnyError> {
-    let pkg_nv = self
-      .npm_resolver
-      .resolve_pkg_id_from_pkg_req(pkg_ref.req())?
-      .nv;
-    let bin_name = pkg_ref.sub_path();
-    let package_folder = self
-      .npm_resolver
-      .resolve_package_folder_from_deno_module(&pkg_nv)?;
     let package_json_path = package_folder.join("package.json");
-    let package_json =
+    let package_json = self
-      self.load_package_json(&AllowAllNodePermissions, package_json_path)?;
+      .load_package_json(&AllowAllNodePermissions, package_json_path.clone())?;
-    let bin = match &package_json.bin {
+    let bin_entry = resolve_bin_entry_value(&package_json, sub_path)?;
-      Some(bin) => bin,
-      None => bail!(
-        "package '{}' did not have a bin property in its package.json",
-        &pkg_nv.name,
-      ),
-    };
-    let bin_entry = resolve_bin_entry_value(&pkg_nv, bin_name, bin)?;
     let url =
       ModuleSpecifier::from_file_path(package_folder.join(bin_entry)).unwrap();
 
@@ -1293,13 +1261,19 @@ impl NodeResolver {
 }
 
 fn resolve_bin_entry_value<'a>(
-  pkg_nv: &PackageNv,
+  package_json: &'a PackageJson,
   bin_name: Option<&str>,
-  bin: &'a Value,
 ) -> Result<&'a str, AnyError> {
+  let bin = match &package_json.bin {
+    Some(bin) => bin,
+    None => bail!(
+      "'{}' did not have a bin property",
+      package_json.path.display(),
+    ),
+  };
   let bin_entry = match bin {
     Value::String(_) => {
-      if bin_name.is_some() && bin_name.unwrap() != pkg_nv.name {
+      if bin_name.is_some() && bin_name != package_json.name.as_deref() {
         None
       } else {
         Some(bin)
@@ -1308,29 +1282,50 @@ fn resolve_bin_entry_value<'a>(
     Value::Object(o) => {
       if let Some(bin_name) = bin_name {
         o.get(bin_name)
-      } else if o.len() == 1 || o.len() > 1 && o.values().all(|v| v == o.values().next().unwrap()) {
+      } else if o.len() == 1
+        || o.len() > 1 && o.values().all(|v| v == o.values().next().unwrap())
+      {
         o.values().next()
       } else {
-        o.get(&pkg_nv.name)
+        package_json.name.as_ref().and_then(|n| o.get(n))
       }
-    },
+    }
-    _ => bail!("package '{}' did not have a bin property with a string or object value in its package.json", pkg_nv),
+    _ => bail!(
+      "'{}' did not have a bin property with a string or object value",
+      package_json.path.display()
+    ),
   };
   let bin_entry = match bin_entry {
     Some(e) => e,
     None => {
+      let prefix = package_json
+        .name
+        .as_ref()
+        .map(|n| {
+          let mut prefix = format!("npm:{}", n);
+          if let Some(version) = &package_json.version {
+            prefix.push('@');
+            prefix.push_str(version);
+          }
+          prefix.push('/');
+          prefix
+        })
+        .unwrap_or_default();
       let keys = bin
         .as_object()
         .map(|o| {
           o.keys()
-            .map(|k| format!(" * npm:{pkg_nv}/{k}"))
+            .map(|k| format!(" * {prefix}{k}"))
             .collect::<Vec<_>>()
         })
         .unwrap_or_default();
       bail!(
-        "package '{}' did not have a bin entry for '{}' in its package.json{}",
+        "'{}' did not have a bin entry{}{}",
-        pkg_nv,
+        package_json.path.display(),
-        bin_name.unwrap_or(&pkg_nv.name),
+        bin_name
+          .or(package_json.name.as_deref())
+          .map(|name| format!(" for '{}'", name))
+          .unwrap_or_default(),
         if keys.is_empty() {
           "".to_string()
         } else {
@@ -1342,8 +1337,8 @@ fn resolve_bin_entry_value<'a>(
   match bin_entry {
     Value::String(s) => Ok(s),
     _ => bail!(
-      "package '{}' had a non-string sub property of bin in its package.json",
+      "'{}' had a non-string sub property of bin",
-      pkg_nv,
+      package_json.path.display(),
     ),
   }
 }
@@ -1595,102 +1590,141 @@ mod tests {
 
   use super::*;
 
+  fn build_package_json(json: Value) -> PackageJson {
+    PackageJson::load_from_value(PathBuf::from("/package.json"), json).unwrap()
+  }
+
   #[test]
   fn test_resolve_bin_entry_value() {
     // should resolve the specified value
-    let value = json!({
+    let pkg_json = build_package_json(json!({
-      "bin1": "./value1",
+      "name": "pkg",
-      "bin2": "./value2",
+      "version": "1.1.1",
-      "test": "./value3",
+      "bin": {
-    });
+        "bin1": "./value1",
+        "bin2": "./value2",
+        "pkg": "./value3",
+      }
+    }));
     assert_eq!(
-      resolve_bin_entry_value(
+      resolve_bin_entry_value(&pkg_json, Some("bin1")).unwrap(),
-        &PackageNv::from_str("test@1.1.1").unwrap(),
-        Some("bin1"),
-        &value
-      )
-      .unwrap(),
       "./value1"
     );
 
     // should resolve the value with the same name when not specified
     assert_eq!(
-      resolve_bin_entry_value(
+      resolve_bin_entry_value(&pkg_json, None).unwrap(),
-        &PackageNv::from_str("test@1.1.1").unwrap(),
-        None,
-        &value
-      )
-      .unwrap(),
       "./value3"
     );
 
     // should not resolve when specified value does not exist
     assert_eq!(
-      resolve_bin_entry_value(
+      resolve_bin_entry_value(&pkg_json, Some("other"),)
-        &PackageNv::from_str("test@1.1.1").unwrap(),
+        .err()
-        Some("other"),
+        .unwrap()
-        &value
+        .to_string(),
-      )
-      .err()
-      .unwrap()
-      .to_string(),
       concat!(
-        "package 'test@1.1.1' did not have a bin entry for 'other' in its package.json\n",
+        "'/package.json' did not have a bin entry for 'other'\n",
         "\n",
         "Possibilities:\n",
-        " * npm:test@1.1.1/bin1\n",
+        " * npm:pkg@1.1.1/bin1\n",
-        " * npm:test@1.1.1/bin2\n",
+        " * npm:pkg@1.1.1/bin2\n",
-        " * npm:test@1.1.1/test"
+        " * npm:pkg@1.1.1/pkg"
       )
     );
 
     // should not resolve when default value can't be determined
+    let pkg_json = build_package_json(json!({
+      "name": "pkg",
+      "version": "1.1.1",
+      "bin": {
+        "bin": "./value1",
+        "bin2": "./value2",
+      }
+    }));
     assert_eq!(
-      resolve_bin_entry_value(
+      resolve_bin_entry_value(&pkg_json, None)
-        &PackageNv::from_str("asdf@1.2.3").unwrap(),
+        .err()
-        None,
+        .unwrap()
-        &value
+        .to_string(),
-      )
-      .err()
-      .unwrap()
-      .to_string(),
       concat!(
-        "package 'asdf@1.2.3' did not have a bin entry for 'asdf' in its package.json\n",
+        "'/package.json' did not have a bin entry for 'pkg'\n",
         "\n",
         "Possibilities:\n",
-        " * npm:asdf@1.2.3/bin1\n",
+        " * npm:pkg@1.1.1/bin\n",
-        " * npm:asdf@1.2.3/bin2\n",
+        " * npm:pkg@1.1.1/bin2",
-        " * npm:asdf@1.2.3/test"
       )
     );
 
     // should resolve since all the values are the same
-    let value = json!({
+    let pkg_json = build_package_json(json!({
-      "bin1": "./value",
+      "name": "pkg",
-      "bin2": "./value",
+      "version": "1.2.3",
-    });
+      "bin": {
+        "bin1": "./value",
+        "bin2": "./value",
+      }
+    }));
     assert_eq!(
-      resolve_bin_entry_value(
+      resolve_bin_entry_value(&pkg_json, None,).unwrap(),
-        &PackageNv::from_str("test@1.2.3").unwrap(),
-        None,
-        &value
-      )
-      .unwrap(),
       "./value"
     );
 
     // should not resolve when specified and is a string
-    let value = json!("./value");
+    let pkg_json = build_package_json(json!({
+      "name": "pkg",
+      "version": "1.2.3",
+      "bin": "./value",
+    }));
     assert_eq!(
-      resolve_bin_entry_value(
+      resolve_bin_entry_value(&pkg_json, Some("path"),)
-        &PackageNv::from_str("test@1.2.3").unwrap(),
+        .err()
-        Some("path"),
+        .unwrap()
-        &value
+        .to_string(),
+      "'/package.json' did not have a bin entry for 'path'"
+    );
+
+    // no version in the package.json
+    let pkg_json = build_package_json(json!({
+      "name": "pkg",
+      "bin": {
+        "bin1": "./value1",
+        "bin2": "./value2",
+      }
+    }));
+    assert_eq!(
+      resolve_bin_entry_value(&pkg_json, None)
+        .err()
+        .unwrap()
+        .to_string(),
+      concat!(
+        "'/package.json' did not have a bin entry for 'pkg'\n",
+        "\n",
+        "Possibilities:\n",
+        " * npm:pkg/bin1\n",
+        " * npm:pkg/bin2",
+      )
+    );
+
+    // no name or version in the package.json
+    let pkg_json = build_package_json(json!({
+      "bin": {
+        "bin1": "./value1",
+        "bin2": "./value2",
+      }
+    }));
+    assert_eq!(
+      resolve_bin_entry_value(&pkg_json, None)
+        .err()
+        .unwrap()
+        .to_string(),
+      concat!(
+        "'/package.json' did not have a bin entry\n",
+        "\n",
+        "Possibilities:\n",
+        " * bin1\n",
+        " * bin2",
       )
-      .err()
-      .unwrap()
-      .to_string(),
-      "package 'test@1.2.3' did not have a bin entry for 'path' in its package.json"
     );
   }