feat: improve lockfile v4 to store normalized version constraints and be more terse (#25247)

Stores normalized version constraints in the lockfile, which will
improve reproducibility and will fix a bug with duplicate specifiers
ending up in the lockfile. Also, gets rid of some duplicate data in the
specifiers area of the lockfile.

7 files changed, 110 insertions, 58 deletions

diff --git a/cli/Cargo.toml b/cli/Cargo.toml
index 33a169458..616dae92e 100644
--- a/cli/Cargo.toml
+++ b/cli/Cargo.toml
@@ -72,13 +72,13 @@ deno_emit = "=0.44.0"
 deno_graph = { version = "=0.81.3" }
 deno_lint = { version = "=0.63.1", features = ["docs"] }
 deno_lockfile.workspace = true
-deno_npm = "=0.24.0"
+deno_npm = "=0.25.0"
 deno_package_json.workspace = true
 deno_runtime = { workspace = true, features = ["include_js_files_for_snapshotting"] }
-deno_semver = "=0.5.10"
+deno_semver.workspace = true
 deno_task_shell = "=0.17.0"
 deno_terminal.workspace = true
-eszip = "=0.76.0"
+eszip = "=0.77.0"
 libsui = "0.3.0"
 napi_sym.workspace = true
 node_resolver.workspace = true
@@ -115,7 +115,7 @@ http.workspace = true
 http-body.workspace = true
 http-body-util.workspace = true
 hyper-util.workspace = true
-import_map = { version = "=0.20.0", features = ["ext"] }
+import_map = { version = "=0.20.1", features = ["ext"] }
 indexmap.workspace = true
 jsonc-parser.workspace = true
 jupyter_runtime = { package = "runtimelib", version = "=0.14.0" }
diff --git a/cli/args/lockfile.rs b/cli/args/lockfile.rs
index 8817975cf..3b6bcc2e5 100644
--- a/cli/args/lockfile.rs
+++ b/cli/args/lockfile.rs
@@ -1,6 +1,6 @@
 // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license.
 
-use std::collections::BTreeSet;
+use std::collections::HashSet;
 use std::path::PathBuf;
 
 use deno_config::deno_json::ConfigFile;
@@ -12,6 +12,7 @@ use deno_core::parking_lot::MutexGuard;
 use deno_lockfile::WorkspaceMemberConfig;
 use deno_package_json::PackageJsonDepValue;
 use deno_runtime::deno_node::PackageJson;
+use deno_semver::jsr::JsrDepPackageReq;
 
 use crate::cache;
 use crate::util::fs::atomic_write_file_with_retries;
@@ -98,7 +99,9 @@ impl CliLockfile {
     flags: &Flags,
     workspace: &Workspace,
   ) -> Result<Option<CliLockfile>, AnyError> {
-    fn pkg_json_deps(maybe_pkg_json: Option<&PackageJson>) -> BTreeSet<String> {
+    fn pkg_json_deps(
+      maybe_pkg_json: Option<&PackageJson>,
+    ) -> HashSet<JsrDepPackageReq> {
       let Some(pkg_json) = maybe_pkg_json else {
         return Default::default();
       };
@@ -107,21 +110,21 @@ impl CliLockfile {
         .values()
         .filter_map(|dep| dep.as_ref().ok())
         .filter_map(|dep| match dep {
-          PackageJsonDepValue::Req(req) => Some(req),
+          PackageJsonDepValue::Req(req) => {
+            Some(JsrDepPackageReq::npm(req.clone()))
+          }
           PackageJsonDepValue::Workspace(_) => None,
         })
-        .map(|r| format!("npm:{}", r))
         .collect()
     }
 
     fn deno_json_deps(
       maybe_deno_json: Option<&ConfigFile>,
-    ) -> BTreeSet<String> {
+    ) -> HashSet<JsrDepPackageReq> {
       maybe_deno_json
         .map(|c| {
           crate::args::deno_json::deno_json_deps(c)
             .into_iter()
-            .map(|req| req.to_string())
             .collect()
         })
         .unwrap_or_default()
@@ -207,6 +210,7 @@ impl CliLockfile {
 
     Ok(Some(lockfile))
   }
+
   pub fn read_from_path(
     file_path: PathBuf,
     frozen: bool,
diff --git a/cli/graph_util.rs b/cli/graph_util.rs
index 6f0e6acd9..c8432b67e 100644
--- a/cli/graph_util.rs
+++ b/cli/graph_util.rs
@@ -44,10 +44,13 @@ use deno_graph::SpecifierError;
 use deno_runtime::deno_fs::FileSystem;
 use deno_runtime::deno_node;
 use deno_runtime::deno_permissions::PermissionsContainer;
+use deno_semver::jsr::JsrDepPackageReq;
 use deno_semver::package::PackageNv;
-use deno_semver::package::PackageReq;
+use deno_semver::Version;
 use import_map::ImportMapError;
 use std::collections::HashSet;
+use std::error::Error;
+use std::ops::Deref;
 use std::path::PathBuf;
 use std::sync::Arc;
 
@@ -110,7 +113,7 @@ pub fn graph_valid(
         ModuleGraphError::ModuleError(error) => {
           enhanced_lockfile_error_message(error)
             .or_else(|| enhanced_sloppy_imports_error_message(fs, error))
-            .unwrap_or_else(|| format!("{}", error))
+            .unwrap_or_else(|| format_deno_graph_error(error))
         }
       };
 
@@ -164,7 +167,10 @@ pub fn graph_valid(
   } else {
     // finally surface the npm resolution result
     if let Err(err) = &graph.npm_dep_graph_result {
-      return Err(custom_error(get_error_class_name(err), format!("{}", err)));
+      return Err(custom_error(
+        get_error_class_name(err),
+        format_deno_graph_error(err.as_ref().deref()),
+      ));
     }
     Ok(())
   }
@@ -463,7 +469,7 @@ impl ModuleGraphBuilder {
           .content
           .packages
           .jsr
-          .get(&package_nv.to_string())
+          .get(package_nv)
           .map(|s| LoaderChecksum::new(s.integrity.clone()))
       }
 
@@ -477,7 +483,7 @@ impl ModuleGraphBuilder {
         self
           .0
           .lock()
-          .insert_package(package_nv.to_string(), checksum.into_string());
+          .insert_package(package_nv.clone(), checksum.into_string());
       }
     }
 
@@ -556,16 +562,21 @@ impl ModuleGraphBuilder {
             }
           }
         }
-        for (key, value) in &lockfile.content.packages.specifiers {
-          if let Some(key) = key
-            .strip_prefix("jsr:")
-            .and_then(|key| PackageReq::from_str(key).ok())
-          {
-            if let Some(value) = value
-              .strip_prefix("jsr:")
-              .and_then(|value| PackageNv::from_str(value).ok())
-            {
-              graph.packages.add_nv(key, value);
+        for (req_dep, value) in &lockfile.content.packages.specifiers {
+          match req_dep.kind {
+            deno_semver::package::PackageKind::Jsr => {
+              if let Ok(version) = Version::parse_standard(value) {
+                graph.packages.add_nv(
+                  req_dep.req.clone(),
+                  PackageNv {
+                    name: req_dep.req.name.clone(),
+                    version,
+                  },
+                );
+              }
+            }
+            deno_semver::package::PackageKind::Npm => {
+              // ignore
             }
           }
         }
@@ -603,16 +614,15 @@ impl ModuleGraphBuilder {
         if has_jsr_package_mappings_changed {
           for (from, to) in graph.packages.mappings() {
             lockfile.insert_package_specifier(
-              format!("jsr:{}", from),
-              format!("jsr:{}", to),
+              JsrDepPackageReq::jsr(from.clone()),
+              to.version.to_string(),
             );
           }
         }
         // jsr packages
         if has_jsr_package_deps_changed {
-          for (name, deps) in graph.packages.packages_with_deps() {
-            lockfile
-              .add_package_deps(&name.to_string(), deps.map(|s| s.to_string()));
+          for (nv, deps) in graph.packages.packages_with_deps() {
+            lockfile.add_package_deps(nv, deps.cloned());
           }
         }
       }
@@ -726,7 +736,7 @@ pub fn error_for_any_npm_specifier(
 
 /// Adds more explanatory information to a resolution error.
 pub fn enhanced_resolution_error_message(error: &ResolutionError) -> String {
-  let mut message = format!("{error}");
+  let mut message = format_deno_graph_error(error);
 
   if let Some(specifier) = get_resolution_error_bare_node_specifier(error) {
     if !*DENO_DISABLE_PEDANTIC_NODE_WARNINGS {
@@ -1022,6 +1032,49 @@ impl deno_graph::source::JsrUrlProvider for CliJsrUrlProvider {
   }
 }
 
+// todo(dsherret): We should change ModuleError to use thiserror so that
+// we don't need to do this.
+fn format_deno_graph_error(err: &dyn Error) -> String {
+  use std::fmt::Write;
+
+  let mut message = format!("{}", err);
+  let mut maybe_source = err.source();
+
+  if maybe_source.is_some() {
+    let mut past_message = message.clone();
+    let mut count = 0;
+    let mut display_count = 0;
+    while let Some(source) = maybe_source {
+      let current_message = format!("{}", source);
+      maybe_source = source.source();
+
+      // sometimes an error might be repeated due to
+      // being boxed multiple times in another AnyError
+      if current_message != past_message {
+        write!(message, "\n    {}: ", display_count,).unwrap();
+        for (i, line) in current_message.lines().enumerate() {
+          if i > 0 {
+            write!(message, "\n       {}", line).unwrap();
+          } else {
+            write!(message, "{}", line).unwrap();
+          }
+        }
+        display_count += 1;
+      }
+
+      if count > 8 {
+        write!(message, "\n    {}: ...", count).unwrap();
+        break;
+      }
+
+      past_message = current_message;
+      count += 1;
+    }
+  }
+
+  message
+}
+
 #[cfg(test)]
 mod test {
   use std::sync::Arc;
diff --git a/cli/lsp/documents.rs b/cli/lsp/documents.rs
index 80c5a4dc5..dcfdb0d00 100644
--- a/cli/lsp/documents.rs
+++ b/cli/lsp/documents.rs
@@ -1410,11 +1410,9 @@ impl Documents {
       if let Some(lockfile) = config_data.lockfile.as_ref() {
         let reqs = npm_reqs_by_scope.entry(Some(scope.clone())).or_default();
         let lockfile = lockfile.lock();
-        for key in lockfile.content.packages.specifiers.keys() {
-          if let Some(key) = key.strip_prefix("npm:") {
-            if let Ok(req) = PackageReq::from_str(key) {
-              reqs.insert(req);
-            }
+        for dep_req in lockfile.content.packages.specifiers.keys() {
+          if dep_req.kind == deno_semver::package::PackageKind::Npm {
+            reqs.insert(dep_req.req.clone());
           }
         }
       }
diff --git a/cli/lsp/jsr.rs b/cli/lsp/jsr.rs
index c0a82383d..6c591637c 100644
--- a/cli/lsp/jsr.rs
+++ b/cli/lsp/jsr.rs
@@ -92,20 +92,23 @@ impl JsrCacheResolver {
       }
     }
     if let Some(lockfile) = config_data.and_then(|d| d.lockfile.as_ref()) {
-      for (req_url, nv_url) in &lockfile.lock().content.packages.specifiers {
-        let Some(req) = req_url.strip_prefix("jsr:") else {
-          continue;
-        };
-        let Some(nv) = nv_url.strip_prefix("jsr:") else {
-          continue;
-        };
-        let Ok(req) = PackageReq::from_str(req) else {
-          continue;
+      for (dep_req, version) in &lockfile.lock().content.packages.specifiers {
+        let req = match dep_req.kind {
+          deno_semver::package::PackageKind::Jsr => &dep_req.req,
+          deno_semver::package::PackageKind::Npm => {
+            continue;
+          }
         };
-        let Ok(nv) = PackageNv::from_str(nv) else {
+        let Ok(version) = Version::parse_standard(version) else {
           continue;
         };
-        nv_by_req.insert(req, Some(nv));
+        nv_by_req.insert(
+          req.clone(),
+          Some(PackageNv {
+            name: req.name.clone(),
+            version,
+          }),
+        );
       }
     }
     Self {
diff --git a/cli/npm/managed/mod.rs b/cli/npm/managed/mod.rs
index 8f82ddea2..a0148c648 100644
--- a/cli/npm/managed/mod.rs
+++ b/cli/npm/managed/mod.rs
@@ -406,8 +406,7 @@ impl ManagedCliNpmResolver {
       }
     }
     if result.dependencies_result.is_ok() {
-      result.dependencies_result =
-        self.cache_packages().await.map_err(AnyError::from);
+      result.dependencies_result = self.cache_packages().await;
     }
 
     result
diff --git a/cli/npm/managed/resolution.rs b/cli/npm/managed/resolution.rs
index 3d13e1735..486e87816 100644
--- a/cli/npm/managed/resolution.rs
+++ b/cli/npm/managed/resolution.rs
@@ -22,6 +22,7 @@ use deno_npm::NpmPackageCacheFolderId;
 use deno_npm::NpmPackageId;
 use deno_npm::NpmResolutionPackage;
 use deno_npm::NpmSystemInfo;
+use deno_semver::jsr::JsrDepPackageReq;
 use deno_semver::package::PackageNv;
 use deno_semver::package::PackageReq;
 use deno_semver::VersionReq;
@@ -329,16 +330,10 @@ fn populate_lockfile_from_snapshot(
 ) {
   let mut lockfile = lockfile.lock();
   for (package_req, nv) in snapshot.package_reqs() {
+    let id = &snapshot.resolve_package_from_deno_module(nv).unwrap().id;
     lockfile.insert_package_specifier(
-      format!("npm:{}", package_req),
-      format!(
-        "npm:{}",
-        snapshot
-          .resolve_package_from_deno_module(nv)
-          .unwrap()
-          .id
-          .as_serialized()
-      ),
+      JsrDepPackageReq::npm(package_req.clone()),
+      format!("{}{}", id.nv.version, id.peer_deps_serialized()),
     );
   }
   for package in snapshot.all_packages_for_every_system() {