Fix crash on missing dev dependency (#161)

Author: Bushuo

src/build/compile.rs

@@ -364,33 +364,10 @@ pub fn compiler_args(
     packages: &Option<&AHashMap<String, packages::Package>>,
     build_dev_deps: bool,
 ) -> Vec<String> {
-    let normal_deps = config.bs_dependencies.as_ref().unwrap_or(&vec![]).to_owned();
-
     let bsc_flags = config::flatten_flags(&config.bsc_flags);
-    // don't compile dev-deps yet
-    let dev_deps = if build_dev_deps {
-        config.bs_dev_dependencies.as_ref().unwrap_or(&vec![]).to_owned()
-    } else {
-        vec![]
-    };
 
-    let deps = [dev_deps, normal_deps]
-        .concat()
-        .par_iter()
-        .map(|package_name| {
-            let canonicalized_path = if let Some(packages) = packages {
-                let package = packages.get(package_name).expect("expect package");
-                package.path.to_string()
-            } else {
-                packages::read_dependency(package_name, project_root, project_root, workspace_root)
-                    .expect("cannot find dep")
-            };
-            vec![
-                "-I".to_string(),
-                packages::get_ocaml_build_path(&canonicalized_path),
-            ]
-        })
-        .collect::<Vec<Vec<String>>>();
+    let dependency_paths =
+        get_dependency_paths(config, project_root, workspace_root, packages, build_dev_deps);
 
     let module_name = helpers::file_path_to_module_name(file_path, &config.get_namespace());
 
@@ -476,7 +453,7 @@ pub fn compiler_args(
         namespace_args,
         read_cmi_args,
         vec!["-I".to_string(), "../ocaml".to_string()],
-        deps.concat(),
+        dependency_paths.concat(),
         uncurried_args,
         bsc_flags.to_owned(),
         warning_args,
@@ -497,6 +474,83 @@ pub fn compiler_args(
     .concat()
 }
 
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+enum DependentPackage {
+    Normal(String),
+    Dev(String),
+}
+
+impl DependentPackage {
+    fn name(&self) -> &str {
+        match self {
+            Self::Normal(name) => name,
+            Self::Dev(name) => name,
+        }
+    }
+
+    fn is_dev(&self) -> bool {
+        match self {
+            Self::Normal(_) => false,
+            Self::Dev(_) => true,
+        }
+    }
+}
+
+fn get_dependency_paths(
+    config: &config::Config,
+    project_root: &str,
+    workspace_root: &Option<String>,
+    packages: &Option<&AHashMap<String, packages::Package>>,
+    build_dev_deps: bool,
+) -> Vec<Vec<String>> {
+    let normal_deps = config
+        .bs_dependencies
+        .clone()
+        .unwrap_or_default()
+        .into_iter()
+        .map(DependentPackage::Normal)
+        .collect();
+    let dev_deps = if build_dev_deps {
+        config
+            .bs_dev_dependencies
+            .clone()
+            .unwrap_or_default()
+            .into_iter()
+            .map(DependentPackage::Dev)
+            .collect()
+    } else {
+        vec![]
+    };
+
+    [dev_deps, normal_deps]
+        .concat()
+        .par_iter()
+        .filter_map(|dependent_package| {
+            let package_name = dependent_package.name();
+            let dependency_path = if let Some(packages) = packages {
+                packages.get(package_name).map(|package| package.path.to_string())
+            } else {
+                packages::read_dependency(package_name, project_root, project_root, workspace_root).ok()
+            }
+            .map(|canonicalized_path| {
+                vec![
+                    "-I".to_string(),
+                    packages::get_ocaml_build_path(&canonicalized_path),
+                ]
+            });
+
+            if !dependent_package.is_dev() && dependency_path.is_none() {
+                panic!(
+                    "Expected to find dependent package {} of {}",
+                    package_name, config.name
+                );
+            }
+
+            dependency_path
+        })
+        .collect::<Vec<Vec<String>>>()
+}
+
 fn compile_file(
     package: &packages::Package,
     root_package: &packages::Package,

src/config.rs

@@ -289,7 +289,7 @@ fn check_if_rescript11_or_higher(version: &str) -> Result<bool, String> {
 fn namespace_from_package_name(package_name: &str) -> String {
     let len = package_name.len();
     let mut buf = String::with_capacity(len);
-    
+
     fn aux(s: &str, capital: bool, buf: &mut String, off: usize) {
         if off >= s.len() {
             return;
@@ -529,7 +529,7 @@ mod tests {
 
         let config = serde_json::from_str::<Config>(json).unwrap();
         if let Some(OneOrMore::Multiple(sources)) = config.sources {
-            let src_dir = sources[0].to_qualified_without_children(None);
+            assert_eq!(sources.len(), 2);
             let test_dir = sources[1].to_qualified_without_children(None);
 
             assert_eq!(test_dir.type_, Some(String::from("dev")));

testrepo/packages/with-dev-deps/package.json

@@ -7,6 +7,7 @@
   "author": "",
   "license": "MIT",
   "dependencies": {
-    "rescript": "*"
+    "rescript": "*",
+    "@rescript/core": "*"
   }
 }

testrepo/packages/with-dev-deps/rescript.json

@@ -13,5 +13,6 @@
     "module": "es6",
     "in-source": true
   },
+  "bs-dependencies": ["@rescript/core"],
   "suffix": ".res.js"
 }

testrepo/yarn.lock

@@ -2,7 +2,12 @@
 # yarn lockfile v1
 
 
+"@rescript/core@*":
+  version "1.6.1"
+  resolved "https://registry.yarnpkg.com/@rescript/core/-/core-1.6.1.tgz#159670c94d64a2b8236f46be2bf09a007b1ece08"
+  integrity sha512-vyb5k90ck+65Fgui+5vCja/mUfzKaK3kOPT4Z6aAJdHLH1eljEi1zKhXroCiCtpNLSWp8k4ulh1bdB5WS0hvqA==
+
 rescript@*:
-  version "11.0.0"
-  resolved "https://registry.yarnpkg.com/rescript/-/rescript-11.0.0.tgz#9a0b6fc998c360543c459aba49b77a572a0306cd"
-  integrity sha512-uIUwDZZmDUb7ymGkBiiGioxMg8hXh1mze/2k/qhYQcZGgi7PrLHQIW9AksM7gb9WnpjCAvFsA8U2VgC0nA468w==
+  version "11.1.4"
+  resolved "https://registry.yarnpkg.com/rescript/-/rescript-11.1.4.tgz#9a42ebc4fc5363707e39cef5b3188160b63bee42"
+  integrity sha512-0bGU0bocihjSC6MsE3TMjHjY0EUpchyrREquLS8VsZ3ohSMD+VHUEwimEfB3kpBI1vYkw3UFZ3WD8R28guz/Vw==

tests/compile.sh

@@ -33,51 +33,57 @@ fi
 node ./packages/main/src/Main.mjs > ./packages/main/src/output.txt
 
 mv ./packages/main/src/Main.res ./packages/main/src/Main2.res
-rewatch build --no-timing=true &> ../tests/snapshots/rename-file.txt
+rewatch build &> ../tests/snapshots/rename-file.txt
 mv ./packages/main/src/Main2.res ./packages/main/src/Main.res
 
 # Rename a file with a dependent - this should trigger an error
 mv ./packages/main/src/InternalDep.res ./packages/main/src/InternalDep2.res
-rewatch build --no-timing=true &> ../tests/snapshots/rename-file-internal-dep.txt
+rewatch build &> ../tests/snapshots/rename-file-internal-dep.txt
 # replace the absolute path so the snapshot is the same on all machines
 replace "s/$(pwd | sed "s/\//\\\\\//g")//g" ../tests/snapshots/rename-file-internal-dep.txt
 mv ./packages/main/src/InternalDep2.res ./packages/main/src/InternalDep.res
 
 # Rename a file with a dependent in a namespaced package - this should trigger an error (regression)
 mv ./packages/new-namespace/src/Other_module.res ./packages/new-namespace/src/Other_module2.res
-rewatch build --no-timing=true &> ../tests/snapshots/rename-file-internal-dep-namespace.txt
+rewatch build &> ../tests/snapshots/rename-file-internal-dep-namespace.txt
 # replace the absolute path so the snapshot is the same on all machines
 replace "s/$(pwd | sed "s/\//\\\\\//g")//g" ../tests/snapshots/rename-file-internal-dep-namespace.txt
 mv ./packages/new-namespace/src/Other_module2.res ./packages/new-namespace/src/Other_module.res
 
 rewatch build &>  /dev/null
 mv ./packages/main/src/ModuleWithInterface.resi ./packages/main/src/ModuleWithInterface2.resi
-rewatch build --no-timing=true &> ../tests/snapshots/rename-interface-file.txt
+rewatch build &> ../tests/snapshots/rename-interface-file.txt
 mv ./packages/main/src/ModuleWithInterface2.resi ./packages/main/src/ModuleWithInterface.resi
 rewatch build &> /dev/null
 mv ./packages/main/src/ModuleWithInterface.res ./packages/main/src/ModuleWithInterface2.res
-rewatch build --no-timing=true &> ../tests/snapshots/rename-file-with-interface.txt
+rewatch build &> ../tests/snapshots/rename-file-with-interface.txt
 mv ./packages/main/src/ModuleWithInterface2.res ./packages/main/src/ModuleWithInterface.res
 rewatch build &> /dev/null
 
 # when deleting a file that other files depend on, the compile should fail
 rm packages/dep02/src/Dep02.res
-rewatch build --no-timing=true &> ../tests/snapshots/remove-file.txt
+rewatch build &> ../tests/snapshots/remove-file.txt
 # replace the absolute path so the snapshot is the same on all machines
 replace "s/$(pwd | sed "s/\//\\\\\//g")//g" ../tests/snapshots/remove-file.txt
 git checkout -- packages/dep02/src/Dep02.res
 rewatch build &> /dev/null
 
 # it should show an error when we have a dependency cycle
 echo 'Dep01.log()' >> packages/new-namespace/src/NS_alias.res
-rewatch build --no-timing=true &> ../tests/snapshots/dependency-cycle.txt
+rewatch build &> ../tests/snapshots/dependency-cycle.txt
 git checkout -- packages/new-namespace/src/NS_alias.res
-rewatch build &> /dev/null
 
 # it should compile dev dependencies with the --dev flag
-rewatch build --dev &> /dev/null
-file_count=$(find ./packages/with-dev-deps -name *.mjs | wc -l)
-if [ "$file_count" -eq 2 ];
+rewatch clean &> /dev/null
+rewatch build --dev &> /dev/null;
+if [ $? -ne 0 ];
+then
+  error "Failed to compile dev dependencies"
+  exit 1
+fi
+
+file_count=$(find ./packages/with-dev-deps/test -name *.mjs | wc -l)
+if [ "$file_count" -eq 1 ];
 then
   success "Compiled dev dependencies successfully"
 else

tests/snapshots/dependency-cycle.txt

@@ -1,7 +1,7 @@
 [1/7] 📦 Building package tree...[1/7] 📦 Built package tree in 0.00s
 [2/7] 👀 Finding source files...[2/7] 👀 Found source files in 0.00s
 [3/7] 📝 Reading compile state...[3/7] 📝 Read compile state 0.00s
-[4/7] 🧹 Cleaning up previous build...[4/7] 🧹 Cleaned 0/14 0.00s
+[4/7] 🧹 Cleaning up previous build...[4/7] 🧹 Cleaned 0/94 0.00s
 [5/7] 🧱 Parsed 1 source files in 0.00s
 [6/7] 🌴 Collected deps in 0.00s
 [7/7] ❌ Compiled 0 modules in 0.00s

tests/snapshots/remove-file.txt

@@ -1,7 +1,7 @@
 [1/7] 📦 Building package tree...[1/7] 📦 Built package tree in 0.00s
 [2/7] 👀 Finding source files...[2/7] 👀 Found source files in 0.00s
 [3/7] 📝 Reading compile state...[3/7] 📝 Read compile state 0.00s
-[4/7] 🧹 Cleaning up previous build...[4/7] 🧹 Cleaned 1/14 0.00s
+[4/7] 🧹 Cleaning up previous build...[4/7] 🧹 Cleaned 1/94 0.00s
 [5/7] 🧱 Parsed 0 source files in 0.00s
 [6/7] 🌴 Collected deps in 0.00s
 [7/7] ❌ Compiled 1 modules in 0.00s

tests/snapshots/rename-file-internal-dep-namespace.txt

@@ -1,7 +1,7 @@
 [1/7] 📦 Building package tree...[1/7] 📦 Built package tree in 0.00s
 [2/7] 👀 Finding source files...[2/7] 👀 Found source files in 0.00s
 [3/7] 📝 Reading compile state...[3/7] 📝 Read compile state 0.00s
-[4/7] 🧹 Cleaning up previous build...[4/7] 🧹 Cleaned 2/14 0.00s
+[4/7] 🧹 Cleaning up previous build...[4/7] 🧹 Cleaned 2/94 0.00s
 [5/7] 🧱 Parsed 2 source files in 0.00s
 [6/7] 🌴 Collected deps in 0.00s
 [7/7] ❌ Compiled 3 modules in 0.00s

tests/snapshots/rename-file-internal-dep.txt

@@ -1,7 +1,7 @@
 [1/7] 📦 Building package tree...[1/7] 📦 Built package tree in 0.00s
 [2/7] 👀 Finding source files...[2/7] 👀 Found source files in 0.00s
 [3/7] 📝 Reading compile state...[3/7] 📝 Read compile state 0.00s
-[4/7] 🧹 Cleaning up previous build...[4/7] 🧹 Cleaned 2/14 0.00s
+[4/7] 🧹 Cleaning up previous build...[4/7] 🧹 Cleaned 2/94 0.00s
 [5/7] 🧱 Parsed 2 source files in 0.00s
 [6/7] 🌴 Collected deps in 0.00s
 [7/7] ❌ Compiled 2 modules in 0.00s