fix: improve error messages presented to the user

This commit improves error messages presented to the user when
encountering errors parsing JSON.

This required a bit of refactoring, since we were performing too much
work up-front, and swallowing the helpful errors with a fallback (that
stops looking for a lerna manifest and starts looking for an npm 7
workspace) that happened too late.

I took this opportunity to improve the representation of package.json
manifests, which makes interaction with serde more ergonomic.

Author: EricCrosson

Cargo.lock

@@ -12,6 +12,7 @@ anyhow = "1.0.57"
 askama = "0.11.1"
 clap = { version = "3.2.4", features = ["cargo", "derive"] }
 globwalk = "0.8.1"
+indoc = "1.0.7"
 pariter = "0.5.1"
 pathdiff = "0.2.1"
 serde = { version = "1.0.137", features = ["derive"] }

Cargo.toml

@@ -9,9 +9,7 @@ pub trait ConfigurationFile<T> {
 
     /// Create an instance of this configuration file by reading
     /// the specified file from this directory on disk.
-    fn from_directory<P>(monorepo_root: P, directory: P) -> Result<T>
-    where
-        P: AsRef<Path>;
+    fn from_directory(monorepo_root: &Path, directory: &Path) -> Result<T>;
 
     /// Relative path to directory containing this configuration file,
     /// from monorepo root.

src/configuration_file.rs

@@ -1,17 +1,20 @@
+use anyhow::Context;
 use std::fs::File;
-use std::io::{BufWriter, Write};
+use std::io::{BufWriter, Read, Write};
 use std::path::Path;
 
 use anyhow::Result;
 
 use serde::{Deserialize, Serialize};
 
-#[derive(Serialize, Deserialize, Debug, PartialEq)]
+// REFACTOR: this belongs in a different file
+#[derive(Serialize, Deserialize, Debug, PartialEq, Eq)]
 pub struct TypescriptProjectReference {
     pub path: String,
 }
 
-#[derive(Serialize, PartialEq)]
+// REFACTOR: this belongs in a different file
+#[derive(Serialize, PartialEq, Eq)]
 pub struct TypescriptParentProjectReference {
     pub files: Vec<String>,
     pub references: Vec<TypescriptProjectReference>,
@@ -28,3 +31,16 @@ pub fn write_project_references<P: AsRef<Path>>(
     writer.flush()?;
     Ok(())
 }
+
+pub(crate) fn read_json_from_file<T>(filename: &Path) -> Result<T>
+where
+    for<'de> T: Deserialize<'de>,
+{
+    // Reading a file into a string before invoking Serde is faster than
+    // invoking Serde from a BufReader, see
+    // https://github.com/serde-rs/json/issues/160
+    let mut string = String::new();
+    File::open(&filename)?.read_to_string(&mut string)?;
+    serde_json::from_str(&string)
+        .with_context(|| format!("Unable to parse JSON from file '{:?}'", filename))
+}

src/io.rs

@@ -61,7 +61,7 @@ fn link_children_packages(opts: &opts::Link, lerna_manifest: &MonorepoManifest)
     let mut is_exit_success = true;
 
     lerna_manifest
-        .internal_package_manifests
+        .internal_package_manifests()?
         .iter()
         .fold(HashMap::new(), key_children_by_parent)
         .iter()
@@ -102,9 +102,7 @@ fn link_children_packages(opts: &opts::Link, lerna_manifest: &MonorepoManifest)
 }
 
 fn link_package_dependencies(opts: &opts::Link, lerna_manifest: &MonorepoManifest) -> Result<bool> {
-    let package_manifest_by_package_name = lerna_manifest
-        .package_manifests_by_package_name()
-        .expect("Unable to read all package manifests");
+    let package_manifest_by_package_name = lerna_manifest.package_manifests_by_package_name()?;
 
     let tsconfig_diffs: Vec<Option<TypescriptConfig>> = package_manifest_by_package_name
         .iter()
@@ -118,6 +116,7 @@ fn link_package_dependencies(opts: &opts::Link, lerna_manifest: &MonorepoManifes
 
                 let desired_project_references: Vec<TypescriptProjectReference> = {
                     let mut typescript_project_references: Vec<String> = internal_dependencies
+                        .into_iter()
                         .map(|dependency| {
                             diff_paths(dependency.directory(), package_manifest.directory())
                             .expect(

src/link.rs

@@ -30,7 +30,7 @@ fn most_common_dependency_version(
 fn lint_dependency_version(opts: &opts::DependencyVersion) -> Result<()> {
     let opts::DependencyVersion { root, dependencies } = opts;
 
-    let lerna_manifest = MonorepoManifest::from_directory(&root)?;
+    let lerna_manifest = MonorepoManifest::from_directory(root)?;
     let package_manifest_by_package_name = lerna_manifest.package_manifests_by_package_name()?;
 
     let mut is_exit_success = true;

src/lint.rs

@@ -1,46 +1,48 @@
 use std::collections::HashMap;
-use std::fs::File;
-use std::io::BufReader;
-use std::marker::Sync;
-use std::path::Path;
+use std::path::{Path, PathBuf};
 
-use anyhow::Result;
+use anyhow::{Context, Result};
 
 use globwalk::{FileType, GlobWalkerBuilder};
 
+use indoc::formatdoc;
 use serde::{Deserialize, Serialize};
 
 use pariter::IteratorExt as _;
 
 use crate::configuration_file::ConfigurationFile;
+use crate::io::read_json_from_file;
 use crate::package_manifest::PackageManifest;
 
+#[derive(Serialize, Deserialize, Debug)]
+struct PackageManifestGlob(String);
+
+// REFACTOR: drop the File suffix in this identifier
 #[derive(Serialize, Deserialize, Debug)]
 struct LernaManifestFile {
-    packages: Vec<String>,
+    packages: Vec<PackageManifestGlob>,
 }
 
+// REFACTOR: drop the File suffix in this identifier
 #[derive(Serialize, Deserialize, Debug)]
 struct PackageManifestFile {
-    workspaces: Vec<String>,
+    workspaces: Vec<PackageManifestGlob>,
 }
 
 #[derive(Debug)]
 pub struct MonorepoManifest {
-    pub internal_package_manifests: Vec<PackageManifest>,
+    root: PathBuf,
+    globs: Vec<PackageManifestGlob>,
 }
 
-fn get_internal_package_manifests<'a, P, I>(
-    directory: P,
-    package_globs: I,
-) -> Result<Vec<PackageManifest>>
-where
-    P: AsRef<Path> + Sync,
-    I: Iterator<Item = &'a String>,
-{
+fn get_internal_package_manifests(
+    monorepo_root: &Path,
+    package_globs: &[PackageManifestGlob],
+) -> Result<Vec<PackageManifest>> {
     let mut package_manifests: Vec<String> = package_globs
+        .iter()
         .map(|package_manifest_glob| {
-            Path::new(package_manifest_glob)
+            Path::new(&package_manifest_glob.0)
                 .join("package.json")
                 .to_str()
                 .expect("Path not valid UTF-8")
@@ -51,9 +53,10 @@ where
     // ignore paths to speed up file-system walk
     package_manifests.push(String::from("!node_modules/"));
 
-    let monorepo_root = directory.as_ref().to_owned();
+    // Take ownership so we can move this value into the parallel_map
+    let monorepo_root = monorepo_root.to_owned();
 
-    GlobWalkerBuilder::from_patterns(&directory, &package_manifests)
+    GlobWalkerBuilder::from_patterns(&monorepo_root, &package_manifests)
         .file_type(FileType::FILE)
         .min_depth(1)
         .build()
@@ -64,14 +67,13 @@ where
             |options| options.threads(32),
             move |dir_entry| {
                 PackageManifest::from_directory(
-                    monorepo_root.clone(),
+                    &monorepo_root,
                     dir_entry
                         .path()
                         .parent()
                         .expect("Unexpected package in monorepo root")
-                        .strip_prefix(monorepo_root.clone())
-                        .expect("Unexpected package in monorepo root")
-                        .to_owned(),
+                        .strip_prefix(&monorepo_root)
+                        .expect("Unexpected package in monorepo root"),
                 )
             },
         )
@@ -82,57 +84,63 @@ impl MonorepoManifest {
     const LERNA_MANIFEST_FILENAME: &'static str = "lerna.json";
     const PACKAGE_MANIFEST_FILENAME: &'static str = "package.json";
 
-    fn from_lerna_manifest<P>(root: P) -> Result<MonorepoManifest>
-    where
-        P: AsRef<Path> + Sync,
-    {
-        let file = File::open(root.as_ref().join(Self::LERNA_MANIFEST_FILENAME))?;
-        let reader = BufReader::new(file);
-        let lerna_manifest_contents: LernaManifestFile = serde_json::from_reader(reader)?;
+    fn from_lerna_manifest(root: &Path) -> Result<MonorepoManifest> {
+        let filename = root.join(Self::LERNA_MANIFEST_FILENAME);
+        let lerna_manifest: LernaManifestFile =
+            read_json_from_file(&filename).with_context(|| {
+                formatdoc!(
+                    "
+                    Unexpected contents in {:?}
+
+                    I'm trying to parse the following properties and values out
+                    of this lerna.json file:
+
+                    - packages: string[]
+                    ",
+                    filename
+                )
+            })?;
         Ok(MonorepoManifest {
-            internal_package_manifests: get_internal_package_manifests(
-                &root,
-                lerna_manifest_contents.packages.iter(),
-            )?,
+            root: root.to_owned(),
+            globs: lerna_manifest.packages,
         })
     }
 
-    fn from_package_manifest<P>(root: P) -> Result<MonorepoManifest>
-    where
-        P: AsRef<Path> + Sync,
-    {
-        let file = File::open(root.as_ref().join(Self::PACKAGE_MANIFEST_FILENAME))?;
-        let reader = BufReader::new(file);
-        let package_manifest_contents: PackageManifestFile = serde_json::from_reader(reader)?;
+    fn from_package_manifest(root: &Path) -> Result<MonorepoManifest> {
+        let filename = root.join(Self::PACKAGE_MANIFEST_FILENAME);
+        let package_manifest: PackageManifestFile =
+            read_json_from_file(&filename).with_context(|| {
+                formatdoc!(
+                    "
+                    Unexpected contents in {:?}
+
+                    I'm trying to parse the following properties and values out
+                    of this package.json file:
+
+                    - workspaces: string[]
+                    ",
+                    filename
+                )
+            })?;
         Ok(MonorepoManifest {
-            internal_package_manifests: get_internal_package_manifests(
-                &root,
-                package_manifest_contents.workspaces.iter(),
-            )?,
+            root: root.to_owned(),
+            globs: package_manifest.workspaces,
         })
     }
 
-    pub fn from_directory<P>(root: P) -> Result<MonorepoManifest>
-    where
-        P: AsRef<Path> + Sync,
-    {
-        MonorepoManifest::from_lerna_manifest(&root)
-            .or_else(|_| MonorepoManifest::from_package_manifest(&root))
+    pub fn from_directory(root: &Path) -> Result<MonorepoManifest> {
+        MonorepoManifest::from_lerna_manifest(root)
+            .or_else(|_| MonorepoManifest::from_package_manifest(root))
     }
 
-    pub fn package_manifests_by_package_name(&self) -> Result<HashMap<String, &PackageManifest>> {
-        self.internal_package_manifests
-            .iter()
-            .map(|manifest| Ok((manifest.contents.name.to_owned(), manifest)))
-            .collect()
+    pub fn package_manifests_by_package_name(&self) -> Result<HashMap<String, PackageManifest>> {
+        Ok(get_internal_package_manifests(&self.root, &self.globs)?
+            .into_iter()
+            .map(|manifest| (manifest.contents.name.to_owned(), manifest))
+            .collect())
     }
 
-    pub fn into_package_manifests_by_package_name(
-        self,
-    ) -> Result<HashMap<String, PackageManifest>> {
-        self.internal_package_manifests
-            .into_iter()
-            .map(|manifest| Ok((manifest.contents.name.clone(), manifest)))
-            .collect()
+    pub fn internal_package_manifests(&self) -> Result<Vec<PackageManifest>> {
+        get_internal_package_manifests(&self.root, &self.globs)
     }
 }