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.

* https://github.com/denoland/deno_config/pull/88

Closes #24605

Cargo.lock

@@ -1308,9 +1308,9 @@ dependencies = [
 
 [[package]]
 name = "deno_config"
-version = "0.22.2"
+version = "0.23.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "83df0c14d89f4e6e7ff91bfea0b4d5a0a33b4385c517ff4d8b4236d9834561e3"
+checksum = "90684d387a893a3318569b8bb548e2f9291f86f2909f5349dd9d2b97c83fdb18"
 dependencies = [
  "anyhow",
  "deno_semver",

Cargo.toml

@@ -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"] }

cli/lsp/config.rs

@@ -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,13 +1533,14 @@ 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 is_workspace_root = workspace_config.is_some();
-    let workspace_members = if let Some((config, workspace)) = workspace {
+    let workspace_members =
+      if let Some((config, workspace_config)) = workspace_config {
         Arc::new(
-        workspace
+          workspace_config
             .members
             .iter()
             .flat_map(|p| {
@@ -1555,6 +1557,14 @@ impl ConfigData {
       } else {
         Arc::new(vec![])
       };
+    let workspace_root_dir = if is_workspace_root {
+      scope.clone()
+    } else {
+      workspace_root
+        .as_ref()
+        .map(|r| r.scope.clone())
+        .unwrap_or_else(|| scope.clone())
+    };
 
     ConfigData {
       scope: scope.clone(),
@@ -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/resolver.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/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/standalone/mod.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/tools/registry/unfurl.rs

@@ -75,26 +75,59 @@ impl SpecifierUnfurler {
       match resolved {
         MappedResolution::Normal(specifier)
         | MappedResolution::ImportMap(specifier) => Some(specifier),
-        MappedResolution::PackageJson {
+        MappedResolution::WorkspaceNpmPackage {
+          target_pkg_json: pkg_json,
+          pkg_name,
           sub_path,
-          dep_result,
+        } => {
-          ..
+          // todo(#24612): consider warning or error when this is also a jsr package?
-        } => match dep_result {
+          ModuleSpecifier::parse(&format!(
-          Ok(dep) => match dep {
+            "npm:{}{}{}",
-            PackageJsonDepValue::Req(req) => ModuleSpecifier::parse(&format!(
+            pkg_name,
-              "npm:{}{}",
+            pkg_json
-              req,
+              .version
+              .as_ref()
+              .map(|v| format!("@^{}", v))
+              .unwrap_or_default(),
             sub_path
               .as_ref()
               .map(|s| format!("/{}", s))
               .unwrap_or_default()
           ))
-            .ok(),
+          .ok()
-            PackageJsonDepValue::Workspace(_) => {
+        }
-              log::warn!(
+        MappedResolution::PackageJson {
-                "package.json workspace entries are not implemented yet for publishing."
+          alias,
-              );
+          sub_path,
-              None
+          dep_result,
+          ..
+        } => match dep_result {
+          Ok(dep) => match dep {
+            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/vendor/test.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,

tests/specs/npm/workspace_basic/__test__.jsonc

@@ -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/b/main.ts

@@ -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();

tests/specs/npm/workspace_basic/b/main_byonm.out

@@ -1,4 +1,3 @@
 Hello 5
 Hello 5
-Hello 5
 C: Hi!

tests/specs/npm/workspace_basic/b/main_global_cache.out

@@ -2,5 +2,4 @@ Download http://localhost:4260/@denotest/esm-basic
 Download http://localhost:4260/@denotest/esm-basic/1.0.0.tgz
 Hello 5
 Hello 5
-Hello 5
 C: Hi!

tests/specs/npm/workspace_basic/b/main_node_modules_dir.out

@@ -3,5 +3,4 @@ Download http://localhost:4260/@denotest/esm-basic/1.0.0.tgz
 Initialize @denotest/esm-basic@1.0.0
 Hello 5
 Hello 5
-Hello 5
 C: Hi!

tests/specs/npm/workspace_basic/main.out

@@ -0,0 +1,4 @@
+Download http://localhost:4260/@denotest/esm-basic
+Download http://localhost:4260/@denotest/esm-basic/1.0.0.tgz
+Hello 5
+C: Hi!

tests/specs/npm/workspace_basic/main.ts

@@ -0,0 +1,6 @@
+// should resolve these as bare specifiers within the workspace
+import * as a from "@denotest/a";
+import * as c from "@denotest/c";
+
+a.sayHello();
+c.sayHello();

tests/specs/publish/npm_workspace_jsr_pkg_with_npm_dep/__test__.jsonc

@@ -0,0 +1,14 @@
+{
+  "tests": {
+    "dep_and_workspace_dep": {
+      "args": "publish --dry-run --no-check --log-level=debug",
+      "cwd": "subtract",
+      "output": "publish-subtract.out"
+    },
+    "bare_specifier": {
+      "args": "publish --dry-run --no-check --log-level=debug",
+      "cwd": "subtract-2",
+      "output": "publish-subtract2.out"
+    }
+  }
+}

tests/specs/publish/npm_workspace_jsr_pkg_with_npm_dep/add/index.js

@@ -0,0 +1,3 @@
+export function add(a, b) {
+  return a + b;
+}

tests/specs/publish/npm_workspace_jsr_pkg_with_npm_dep/add/package.json

@@ -0,0 +1,6 @@
+{
+  "name": "add",
+  "type": "module",
+  "version": "1.0.0",
+  "exports": "./index.js"
+}

tests/specs/publish/npm_workspace_jsr_pkg_with_npm_dep/package.json

@@ -0,0 +1,3 @@
+{
+  "workspaces": ["./add", "./subtract", "./subtract-2"]
+}

tests/specs/publish/npm_workspace_jsr_pkg_with_npm_dep/publish-subtract.out

@@ -0,0 +1,3 @@
+[WILDCARD]Unfurled specifier: add-dep from file:///[WILDLINE]/subtract/index.ts -> npm:add@1.0.0
+[WILDCARD]Unfurled specifier: add from file:///[WILDLINE]/subtract/index.ts -> npm:add@^1.0.0
+[WILDCARD]

tests/specs/publish/npm_workspace_jsr_pkg_with_npm_dep/publish-subtract2.out

@@ -0,0 +1,2 @@
+[WILDCARD]Unfurled specifier: add from file:///[WILDLINE]/subtract-2/index.ts -> npm:add@^1.0.0
+[WILDCARD]

tests/specs/publish/npm_workspace_jsr_pkg_with_npm_dep/subtract-2/deno.json

@@ -0,0 +1,5 @@
+{
+  "name": "@scope/subtract2",
+  "version": "1.0.0",
+  "exports": "./index.ts"
+}

tests/specs/publish/npm_workspace_jsr_pkg_with_npm_dep/subtract-2/index.ts

@@ -0,0 +1,6 @@
+// test using a bare specifier to a pkg.json dep
+import { add } from "add";
+
+export function subtract(a: number, b: number): number {
+  return add(a, -b);
+}

tests/specs/publish/npm_workspace_jsr_pkg_with_npm_dep/subtract-2/package.json

@@ -0,0 +1,4 @@
+{
+  "name": "subtract2",
+  "version": "1.0.0"
+}

tests/specs/publish/npm_workspace_jsr_pkg_with_npm_dep/subtract/deno.json

@@ -0,0 +1,5 @@
+{
+  "name": "@scope/subtract",
+  "version": "1.0.0",
+  "exports": "./index.ts"
+}

tests/specs/publish/npm_workspace_jsr_pkg_with_npm_dep/subtract/index.ts

@@ -0,0 +1,7 @@
+// test using a pkg.json dep and a workspace dep
+import * as addDep from "add-dep";
+import * as addWorkspaceDep from "add";
+
+export function subtract(a: number, b: number): number {
+  return addWorkspaceDep.add(addDep.add(a, -b), 1 - 1);
+}

tests/specs/publish/npm_workspace_jsr_pkg_with_npm_dep/subtract/package.json

@@ -0,0 +1,8 @@
+{
+  "name": "subtract",
+  "version": "1.0.0",
+  "dependencies": {
+    "add-dep": "npm:add@1.0.0",
+    "add": "workspace:^1.0.0"
+  }
+}