core: test Modules::deps and handle error cases better (#2141)

2 files changed, 54 insertions, 16 deletions

diff --git a/cli/main.rs b/cli/main.rs
index b6cd49d0e..41aeca1c9 100644
--- a/cli/main.rs
+++ b/cli/main.rs
@@ -108,12 +108,18 @@ pub fn print_file_info(worker: &Worker, url: &str) {
     );
   }
 
-  let deps = worker.modules.deps(&out.module_name);
-  println!("{}{}", ansi::bold("deps:\n".to_string()), deps.name);
-  if let Some(ref depsdeps) = deps.deps {
-    for d in depsdeps {
-      println!("{}", d);
+  if let Some(deps) = worker.modules.deps(&out.module_name) {
+    println!("{}{}", ansi::bold("deps:\n".to_string()), deps.name);
+    if let Some(ref depsdeps) = deps.deps {
+      for d in depsdeps {
+        println!("{}", d);
+      }
     }
+  } else {
+    println!(
+      "{} cannot retrieve full dependency graph",
+      ansi::bold("deps:".to_string()),
+    );
   }
 }
 
diff --git a/core/modules.rs b/core/modules.rs
index 1a72bab3e..435255856 100644
--- a/core/modules.rs
+++ b/core/modules.rs
@@ -439,11 +439,15 @@ impl Modules {
     self.by_name.is_alias(name)
   }
 
-  pub fn deps(&self, url: &str) -> Deps {
+  pub fn deps(&self, url: &str) -> Option<Deps> {
     Deps::new(self, url)
   }
 }
 
+/// This is a tree structure representing the dependencies of a given module.
+/// Use Modules::deps to construct it. The 'deps' member is None if this module
+/// was already seen elsewher in the tree.
+#[derive(Debug, PartialEq)]
 pub struct Deps {
   pub name: String,
   pub deps: Option<Vec<Deps>>,
@@ -452,7 +456,7 @@ pub struct Deps {
 }
 
 impl Deps {
-  pub fn new(modules: &Modules, module_name: &str) -> Deps {
+  fn new(modules: &Modules, module_name: &str) -> Option<Deps> {
     let mut seen = HashSet::new();
     Self::helper(&mut seen, "".to_string(), true, modules, module_name)
   }
@@ -463,19 +467,19 @@ impl Deps {
     is_last: bool,
     modules: &Modules,
     name: &str, // TODO(ry) rename url
-  ) -> Deps {
+  ) -> Option<Deps> {
     if seen.contains(name) {
-      Deps {
+      Some(Deps {
         name: name.to_string(),
         prefix,
         deps: None,
         is_last,
-      }
+      })
     } else {
+      let children = modules.get_children2(name)?;
       seen.insert(name.to_string());
-      let children = modules.get_children2(name).unwrap();
-      let child_count = children.iter().count();
-      let deps = children
+      let child_count = children.len();
+      let deps: Vec<Deps> = children
         .iter()
         .enumerate()
         .map(|(index, dep_name)| {
@@ -485,13 +489,16 @@ impl Deps {
           new_prefix.push(' ');
 
           Self::helper(seen, new_prefix, new_is_last, modules, dep_name)
-        }).collect();
-      Deps {
+        })
+        // If any of the children are missing, return None.
+        .collect::<Option<_>>()?;
+
+      Some(Deps {
         name: name.to_string(),
         prefix,
         deps: Some(deps),
         is_last,
-      }
+      })
     }
   }
 }
@@ -888,4 +895,29 @@ mod tests {
     assert_eq!(either_err, JSErrorOr::Other(MockError::ResolveErr));
     assert!(recursive_load.loader.is_none());
   }
+
+  #[test]
+  fn empty_deps() {
+    let modules = Modules::new();
+    assert!(modules.deps("foo").is_none());
+  }
+
+  #[test]
+  fn deps() {
+    // "foo" -> "bar"
+    let mut modules = Modules::new();
+    modules.register(1, "foo");
+    modules.register(2, "bar");
+    modules.add_child(1, "bar");
+    let maybe_deps = modules.deps("foo");
+    assert!(maybe_deps.is_some());
+    let mut foo_deps = maybe_deps.unwrap();
+    assert_eq!(foo_deps.name, "foo");
+    assert!(foo_deps.deps.is_some());
+    let foo_children = foo_deps.deps.take().unwrap();
+    assert_eq!(foo_children.len(), 1);
+    let bar_deps = &foo_children[0];
+    assert_eq!(bar_deps.name, "bar");
+    assert_eq!(bar_deps.deps, Some(vec![]));
+  }
 }