fix(workspace): support resolving bare specifiers to npm pkgs within a workspace (#24611)

This makes bare specifiers for npm packages work when inside a
workspace, which emulates the same behaviour as when there's a
node_modules directory. The bare specifier can be overwritten by
specifying an import map entry or package.json dependency entry.

* denoland/deno_config#88

Closes #24605

Author: dsherret

Cargo.lock

@@ -101,7 +101,7 @@ console_static_text = "=0.8.1"
 data-encoding = "2.3.3"
 data-url = "=0.3.0"
 deno_cache_dir = "=0.10.0"
-deno_config = { version = "=0.22.2", default-features = false }
+deno_config = { version = "=0.23.0", default-features = false }
 dlopen2 = "0.6.1"
 ecb = "=0.1.2"
 elliptic-curve = { version = "0.13.4", features = ["alloc", "arithmetic", "ecdh", "std", "pem"] }

Cargo.toml

@@ -1117,6 +1117,7 @@ pub struct ConfigData {
   pub import_map_from_settings: bool,
   pub package_config: Option<Arc<LspPackageConfig>>,
   pub is_workspace_root: bool,
+  pub workspace_root_dir: ModuleSpecifier,
   /// Workspace member directories. For a workspace root this will be a list of
   /// members. For a member this will be the same list, representing self and
   /// siblings. For a solitary package this will be `vec![self.scope]`. These
@@ -1532,28 +1533,37 @@ impl ConfigData {
       })
     });
 
-    let workspace = config_file
+    let workspace_config = config_file
       .as_ref()
       .and_then(|c| c.to_workspace_config().ok().flatten().map(|w| (c, w)));
-    let is_workspace_root = workspace.is_some();
-    let workspace_members = if let Some((config, workspace)) = workspace {
-      Arc::new(
-        workspace
-          .members
-          .iter()
-          .flat_map(|p| {
-            let dir_specifier = config.specifier.join(p).ok()?;
-            let dir_path = specifier_to_file_path(&dir_specifier).ok()?;
-            Url::from_directory_path(normalize_path(dir_path)).ok()
-          })
-          .collect(),
-      )
-    } else if let Some(workspace_data) = workspace_root {
-      workspace_data.workspace_members.clone()
-    } else if config_file.as_ref().is_some_and(|c| c.json.name.is_some()) {
-      Arc::new(vec![scope.clone()])
+    let is_workspace_root = workspace_config.is_some();
+    let workspace_members =
+      if let Some((config, workspace_config)) = workspace_config {
+        Arc::new(
+          workspace_config
+            .members
+            .iter()
+            .flat_map(|p| {
+              let dir_specifier = config.specifier.join(p).ok()?;
+              let dir_path = specifier_to_file_path(&dir_specifier).ok()?;
+              Url::from_directory_path(normalize_path(dir_path)).ok()
+            })
+            .collect(),
+        )
+      } else if let Some(workspace_data) = workspace_root {
+        workspace_data.workspace_members.clone()
+      } else if config_file.as_ref().is_some_and(|c| c.json.name.is_some()) {
+        Arc::new(vec![scope.clone()])
+      } else {
+        Arc::new(vec![])
+      };
+    let workspace_root_dir = if is_workspace_root {
+      scope.clone()
     } else {
-      Arc::new(vec![])
+      workspace_root
+        .as_ref()
+        .map(|r| r.scope.clone())
+        .unwrap_or_else(|| scope.clone())
     };
 
     ConfigData {
@@ -1574,6 +1584,7 @@ impl ConfigData {
       import_map_from_settings,
       package_config: package_config.map(Arc::new),
       is_workspace_root,
+      workspace_root_dir,
       workspace_members,
       watched_files,
     }

cli/lsp/config.rs

@@ -529,6 +529,13 @@ fn create_graph_resolver(
     node_resolver: node_resolver.cloned(),
     npm_resolver: npm_resolver.cloned(),
     workspace_resolver: Arc::new(WorkspaceResolver::new_raw(
+      Arc::new(
+        config_data
+          .map(|d| d.workspace_root_dir.clone())
+          // this is fine because this value is only used to filter bare
+          // specifier resolution to workspace npm packages when in a workspace
+          .unwrap_or_else(|| ModuleSpecifier::parse("file:///").unwrap()),
+      ),
       config_data.and_then(|d| d.import_map.as_ref().map(|i| (**i).clone())),
       config_data
         .and_then(|d| d.package_json.clone())

cli/lsp/resolver.rs

@@ -536,6 +536,22 @@ impl Resolver for CliGraphResolver {
       Ok(resolution) => match resolution {
         MappedResolution::Normal(specifier)
         | MappedResolution::ImportMap(specifier) => Ok(specifier),
+        MappedResolution::WorkspaceNpmPackage {
+          target_pkg_json: pkg_json,
+          sub_path,
+          ..
+        } => self
+          .node_resolver
+          .as_ref()
+          .unwrap()
+          .resolve_package_sub_path_from_deno_module(
+            pkg_json.dir_path(),
+            sub_path.as_deref(),
+            Some(referrer),
+            to_node_mode(mode),
+          )
+          .map_err(ResolveError::Other)
+          .map(|res| res.into_url()),
         // todo(dsherret): for byonm it should do resolution solely based on
         // the referrer and not the package.json
         MappedResolution::PackageJson {

cli/resolver.rs

@@ -88,7 +88,7 @@ struct WorkspaceEszipModule {
 
 struct WorkspaceEszip {
   eszip: eszip::EszipV2,
-  root_dir_url: ModuleSpecifier,
+  root_dir_url: Arc<ModuleSpecifier>,
 }
 
 impl WorkspaceEszip {
@@ -166,6 +166,22 @@ impl ModuleLoader for EmbeddedModuleLoader {
       self.shared.workspace_resolver.resolve(specifier, &referrer);
 
     match mapped_resolution {
+      Ok(MappedResolution::WorkspaceNpmPackage {
+        target_pkg_json: pkg_json,
+        sub_path,
+        ..
+      }) => Ok(
+        self
+          .shared
+          .node_resolver
+          .resolve_package_sub_path_from_deno_module(
+            pkg_json.dir_path(),
+            sub_path.as_deref(),
+            Some(&referrer),
+            NodeResolutionMode::Execution,
+          )?
+          .into_url(),
+      ),
       Ok(MappedResolution::PackageJson {
         dep_result,
         sub_path,
@@ -427,7 +443,8 @@ pub async fn run(
   let npm_registry_url = ModuleSpecifier::parse("https://localhost/").unwrap();
   let root_path =
     std::env::temp_dir().join(format!("deno-compile-{}", current_exe_name));
-  let root_dir_url = ModuleSpecifier::from_directory_path(&root_path).unwrap();
+  let root_dir_url =
+    Arc::new(ModuleSpecifier::from_directory_path(&root_path).unwrap());
   let main_module = root_dir_url.join(&metadata.entrypoint_key).unwrap();
   let root_node_modules_path = root_path.join("node_modules");
   let npm_cache_dir = NpmCacheDir::new(
@@ -579,6 +596,7 @@ pub async fn run(
       })
       .collect();
     WorkspaceResolver::new_raw(
+      root_dir_url.clone(),
       import_map,
       pkg_jsons,
       metadata.workspace_resolver.pkg_json_resolution,

cli/standalone/mod.rs

@@ -75,26 +75,59 @@ impl SpecifierUnfurler {
       match resolved {
         MappedResolution::Normal(specifier)
         | MappedResolution::ImportMap(specifier) => Some(specifier),
+        MappedResolution::WorkspaceNpmPackage {
+          target_pkg_json: pkg_json,
+          pkg_name,
+          sub_path,
+        } => {
+          // todo(#24612): consider warning or error when this is also a jsr package?
+          ModuleSpecifier::parse(&format!(
+            "npm:{}{}{}",
+            pkg_name,
+            pkg_json
+              .version
+              .as_ref()
+              .map(|v| format!("@^{}", v))
+              .unwrap_or_default(),
+            sub_path
+              .as_ref()
+              .map(|s| format!("/{}", s))
+              .unwrap_or_default()
+          ))
+          .ok()
+        }
         MappedResolution::PackageJson {
+          alias,
           sub_path,
           dep_result,
           ..
         } => match dep_result {
           Ok(dep) => match dep {
-            PackageJsonDepValue::Req(req) => ModuleSpecifier::parse(&format!(
-              "npm:{}{}",
-              req,
-              sub_path
-                .as_ref()
-                .map(|s| format!("/{}", s))
-                .unwrap_or_default()
-            ))
-            .ok(),
-            PackageJsonDepValue::Workspace(_) => {
-              log::warn!(
-                "package.json workspace entries are not implemented yet for publishing."
-              );
-              None
+            PackageJsonDepValue::Req(pkg_req) => {
+              // todo(#24612): consider warning or error when this is an npm workspace
+              // member that's also a jsr package?
+              ModuleSpecifier::parse(&format!(
+                "npm:{}{}",
+                pkg_req,
+                sub_path
+                  .as_ref()
+                  .map(|s| format!("/{}", s))
+                  .unwrap_or_default()
+              ))
+              .ok()
+            }
+            PackageJsonDepValue::Workspace(version_req) => {
+              // todo(#24612): consider warning or error when this is also a jsr package?
+              ModuleSpecifier::parse(&format!(
+                "npm:{}@{}{}",
+                alias,
+                version_req,
+                sub_path
+                  .as_ref()
+                  .map(|s| format!("/{}", s))
+                  .unwrap_or_default()
+              ))
+              .ok()
             }
           },
           Err(err) => {
@@ -401,6 +434,7 @@ mod tests {
       }),
     );
     let workspace_resolver = WorkspaceResolver::new_raw(
+      Arc::new(ModuleSpecifier::from_directory_path(&cwd).unwrap()),
       Some(import_map),
       vec![Arc::new(package_json)],
       deno_config::workspace::PackageJsonDepResolution::Enabled,

cli/tools/registry/unfurl.rs

@@ -234,6 +234,7 @@ impl VendorTestBuilder {
     let loader = self.loader.clone();
     let parsed_source_cache = ParsedSourceCache::default();
     let resolver = Arc::new(build_resolver(
+      output_dir.parent().unwrap(),
       self.jsx_import_source_config.clone(),
       self.maybe_original_import_map.clone(),
     ));
@@ -287,6 +288,7 @@ impl VendorTestBuilder {
 }
 
 fn build_resolver(
+  root_dir: &Path,
   maybe_jsx_import_source_config: Option<JsxImportSourceConfig>,
   maybe_original_import_map: Option<ImportMap>,
 ) -> CliGraphResolver {
@@ -295,6 +297,7 @@ fn build_resolver(
     npm_resolver: None,
     sloppy_imports_resolver: None,
     workspace_resolver: Arc::new(WorkspaceResolver::new_raw(
+      Arc::new(ModuleSpecifier::from_directory_path(root_dir).unwrap()),
       maybe_original_import_map,
       Vec::new(),
       deno_config::workspace::PackageJsonDepResolution::Enabled,

cli/tools/vendor/test.rs

@@ -5,6 +5,10 @@
       "args": "run --node-modules-dir=false b/main.ts",
       "output": "b/main_global_cache.out"
     },
+    "global_cache_bare_specifier_not_in_pkg": {
+      "args": "run --node-modules-dir=false main.ts",
+      "output": "main.out"
+    },
     "node_modules_dir": {
       "args": "run --node-modules-dir=true b/main.ts",
       "output": "b/main_node_modules_dir.out"

tests/specs/npm/workspace_basic/__test__.jsonc

@@ -1,9 +1,7 @@
 import * as a1 from "@denotest/a";
 import * as a2 from "npm:@denotest/a@1";
-import * as a3 from "npm:@denotest/a@workspace";
 import * as c from "@denotest/c";
 
 a1.sayHello();
 a2.sayHello();
-a3.sayHello();
 c.sayHello();