Reuse package resolution for ppx (#7776)

* Reuse package resolution for ppx

* Add changelog

Author: nojaf

CHANGELOG.md

@@ -26,6 +26,7 @@
 
 - Preserve `@as(...)` decorator on record fields when creating interface. https://github.com/rescript-lang/rescript/pull/7779
 - Fix parse error with nested record types and attributes on the field name that has the nested record type. https://github.com/rescript-lang/rescript/pull/7781
+- Fix ppx resolution with package inside monorepo. https://github.com/rescript-lang/rescript/pull/7776
 
 #### :memo: Documentation
 

rewatch/src/build.rs

@@ -80,7 +80,7 @@ pub fn get_compiler_args(rescript_file_path: &Path) -> Result<String> {
         &project_context.current_config,
         relative_filename,
         &contents,
-    );
+    )?;
     let is_interface = filename.to_string_lossy().ends_with('i');
     let has_interface = if is_interface {
         true

rewatch/src/build/packages.rs

@@ -255,48 +255,7 @@ pub fn read_dependency(
     package_config: &Config,
     project_context: &ProjectContext,
 ) -> Result<PathBuf> {
-    // package folder + node_modules + package_name
-    // This can happen in the following scenario:
-    // The ProjectContext has a MonoRepoContext::MonorepoRoot.
-    // We are reading a dependency from the root package.
-    // And that local dependency has a hoisted dependency.
-    let path_from_current_package = package_config
-        .path
-        .parent()
-        .ok_or_else(|| {
-            anyhow!(
-                "Expected {} to have a parent folder",
-                package_config.path.to_string_lossy()
-            )
-        })
-        .map(|parent_path| helpers::package_path(parent_path, package_name))?;
-
-    // current folder + node_modules + package_name
-    let path_from_current_config = project_context
-        .current_config
-        .path
-        .parent()
-        .ok_or_else(|| {
-            anyhow!(
-                "Expected {} to have a parent folder",
-                project_context.current_config.path.to_string_lossy()
-            )
-        })
-        .map(|parent_path| helpers::package_path(parent_path, package_name))?;
-
-    // root folder + node_modules + package_name
-    let path_from_root = helpers::package_path(project_context.get_root_path(), package_name);
-    let path = (if path_from_current_package.exists() {
-        Ok(path_from_current_package)
-    } else if path_from_current_config.exists() {
-        Ok(path_from_current_config)
-    } else if path_from_root.exists() {
-        Ok(path_from_root)
-    } else {
-        Err(anyhow!(
-            "The package \"{package_name}\" is not found (are node_modules up-to-date?)..."
-        ))
-    })?;
+    let path = helpers::try_package_path(package_config, project_context, package_name)?;
 
     let canonical_path = match path
         .canonicalize()

rewatch/src/build/parse.rs

@@ -7,6 +7,7 @@ use crate::config::{Config, OneOrMore};
 use crate::helpers;
 use crate::project_context::ProjectContext;
 use ahash::AHashSet;
+use anyhow::anyhow;
 use log::debug;
 use rayon::prelude::*;
 use std::path::{Path, PathBuf};
@@ -51,11 +52,13 @@ pub fn generate_asts(
                             package.to_owned(),
                             &source_file.implementation.path.to_owned(),
                             build_state,
-                        );
+                        )
+                        .map_err(|e| e.to_string());
 
                         let iast_result = match source_file.interface.as_ref().map(|i| i.path.to_owned()) {
                             Some(interface_file_path) => {
                                 generate_ast(package.to_owned(), &interface_file_path.to_owned(), build_state)
+                                    .map_err(|e| e.to_string())
                                     .map(Some)
                             }
                             _ => Ok(None),
@@ -237,16 +240,15 @@ pub fn parser_args(
     package_config: &Config,
     filename: &Path,
     contents: &str,
-) -> (PathBuf, Vec<String>) {
+) -> anyhow::Result<(PathBuf, Vec<String>)> {
     let root_config = project_context.get_root_config();
     let file = &filename;
     let ast_path = helpers::get_ast_path(file);
-    let node_modules_path = project_context.get_root_path().join("node_modules");
     let ppx_flags = config::flatten_ppx_flags(
-        node_modules_path.as_path(),
+        project_context,
+        package_config,
         &filter_ppx_flags(&package_config.ppx_flags, contents),
-        &package_config.name,
-    );
+    )?;
     let jsx_args = root_config.get_jsx_args();
     let jsx_module_args = root_config.get_jsx_module_args();
     let jsx_mode_args = root_config.get_jsx_mode_args();
@@ -255,7 +257,7 @@ pub fn parser_args(
 
     let file = PathBuf::from("..").join("..").join(file);
 
-    (
+    Ok((
         ast_path.to_owned(),
         [
             ppx_flags,
@@ -273,20 +275,20 @@ pub fn parser_args(
             ],
         ]
         .concat(),
-    )
+    ))
 }
 
 fn generate_ast(
     package: Package,
     filename: &Path,
     build_state: &BuildState,
-) -> Result<(PathBuf, Option<helpers::StdErr>), String> {
+) -> anyhow::Result<(PathBuf, Option<helpers::StdErr>)> {
     let file_path = PathBuf::from(&package.path).join(filename);
     let contents = helpers::read_file(&file_path).expect("Error reading file");
 
     let build_path_abs = package.get_build_path();
     let (ast_path, parser_args) =
-        parser_args(&build_state.project_context, &package.config, filename, &contents);
+        parser_args(&build_state.project_context, &package.config, filename, &contents)?;
 
     // generate the dir of the ast_path (it mirrors the source file dir)
     let ast_parent_path = package.get_build_path().join(ast_path.parent().unwrap());
@@ -298,7 +300,13 @@ fn generate_ast(
             .current_dir(&build_path_abs)
             .args(parser_args)
             .output()
-            .expect("Error converting .res to .ast"),
+            .map_err(|e| {
+                anyhow!(
+                    "Error running bsc for parsing {}: {}",
+                    filename.to_string_lossy(),
+                    e
+                )
+            })?,
     ) {
         Some(res_to_ast) => {
             let stderr = String::from_utf8_lossy(&res_to_ast.stderr).to_string();
@@ -307,7 +315,7 @@ fn generate_ast(
                 if res_to_ast.status.success() {
                     Ok((ast_path, Some(stderr.to_string())))
                 } else {
-                    Err(format!("Error in {}:\n{}", package.name, stderr))
+                    Err(anyhow!("Error in {}:\n{}", package.name, stderr))
                 }
             } else {
                 Ok((ast_path, None))
@@ -316,7 +324,7 @@ fn generate_ast(
         _ => {
             log::info!("Parsing file {}...", filename.display());
 
-            Err(format!(
+            Err(anyhow!(
                 "Could not find canonicalize_string_path for file {} in package {}",
                 filename.display(),
                 package.name

rewatch/src/config.rs

@@ -1,10 +1,12 @@
 use crate::build::packages;
+use crate::helpers;
 use crate::helpers::deserialize::*;
+use crate::project_context::ProjectContext;
 use anyhow::{Result, bail};
 use convert_case::{Case, Casing};
 use serde::Deserialize;
 use std::fs;
-use std::path::{Path, PathBuf};
+use std::path::{MAIN_SEPARATOR, Path, PathBuf};
 
 #[derive(Deserialize, Debug, Clone)]
 #[serde(untagged)]
@@ -296,56 +298,61 @@ pub fn flatten_flags(flags: &Option<Vec<OneOrMore<String>>>) -> Vec<String> {
 /// Since ppx-flags could be one or more, and could potentially be nested, this function takes the
 /// flags and flattens them.
 pub fn flatten_ppx_flags(
-    node_modules_dir: &Path,
+    project_context: &ProjectContext,
+    package_config: &Config,
     flags: &Option<Vec<OneOrMore<String>>>,
-    package_name: &String,
-) -> Vec<String> {
+) -> Result<Vec<String>> {
     match flags {
-        None => vec![],
-        Some(flags) => flags
-            .iter()
-            .flat_map(|x| match x {
+        None => Ok(vec![]),
+        Some(flags) => flags.iter().try_fold(Vec::new(), |mut acc, x| {
+            match x {
                 OneOrMore::Single(y) => {
                     let first_character = y.chars().next();
                     match first_character {
                         Some('.') => {
-                            vec![
-                                "-ppx".to_string(),
-                                node_modules_dir
-                                    .join(package_name)
-                                    .join(y)
-                                    .to_string_lossy()
-                                    .to_string(),
-                            ]
+                            let path = helpers::try_package_path(
+                                package_config,
+                                project_context,
+                                format!("{}{}{}", &package_config.name, MAIN_SEPARATOR, y).as_str(),
+                            )
+                            .map(|p| p.to_string_lossy().to_string())?;
+
+                            acc.push(String::from("-ppx"));
+                            acc.push(path);
+                        }
+                        _ => {
+                            acc.push(String::from("-ppx"));
+                            let path = helpers::try_package_path(package_config, project_context, y)
+                                .map(|p| p.to_string_lossy().to_string())?;
+                            acc.push(path);
                         }
-                        _ => vec![
-                            "-ppx".to_string(),
-                            node_modules_dir.join(y).to_string_lossy().to_string(),
-                        ],
                     }
                 }
-                OneOrMore::Multiple(ys) if ys.is_empty() => vec![],
+                OneOrMore::Multiple(ys) if ys.is_empty() => (),
                 OneOrMore::Multiple(ys) => {
                     let first_character = ys[0].chars().next();
                     let ppx = match first_character {
-                        Some('.') => node_modules_dir
-                            .join(package_name)
-                            .join(&ys[0])
-                            .to_string_lossy()
-                            .to_string(),
-                        _ => node_modules_dir.join(&ys[0]).to_string_lossy().to_string(),
+                        Some('.') => helpers::try_package_path(
+                            package_config,
+                            project_context,
+                            format!("{}{}{}", package_config.name, MAIN_SEPARATOR, &ys[0]).as_str(),
+                        )
+                        .map(|p| p.to_string_lossy().to_string())?,
+                        _ => helpers::try_package_path(package_config, project_context, &ys[0])
+                            .map(|p| p.to_string_lossy().to_string())?,
                     };
-                    vec![
-                        "-ppx".to_string(),
+                    acc.push(String::from("-ppx"));
+                    acc.push(
                         vec![ppx]
                             .into_iter()
                             .chain(ys[1..].to_owned())
                             .collect::<Vec<String>>()
                             .join(" "),
-                    ]
+                    );
                 }
-            })
-            .collect::<Vec<String>>(),
+            };
+            Ok(acc)
+        }),
     }
 }
 

rewatch/src/helpers.rs

@@ -1,5 +1,8 @@
 use crate::build::packages;
+use crate::config::Config;
+use crate::helpers;
 use crate::project_context::ProjectContext;
+use anyhow::anyhow;
 use std::ffi::OsString;
 use std::fs;
 use std::fs::File;
@@ -103,6 +106,60 @@ pub fn package_path(root: &Path, package_name: &str) -> PathBuf {
     root.join("node_modules").join(package_name)
 }
 
+/// Tries to find a path for input package_name.
+/// The node_modules folder may be found at different levels in the case of a monorepo.
+/// This helper tries a variety of paths.
+pub fn try_package_path(
+    package_config: &Config,
+    project_context: &ProjectContext,
+    package_name: &str,
+) -> anyhow::Result<PathBuf> {
+    // package folder + node_modules + package_name
+    // This can happen in the following scenario:
+    // The ProjectContext has a MonoRepoContext::MonorepoRoot.
+    // We are reading a dependency from the root package.
+    // And that local dependency has a hoisted dependency.
+    // Example, we need to find package_name `foo` in the following scenario:
+    // root/packages/a/node_modules/foo
+    let path_from_current_package = package_config
+        .path
+        .parent()
+        .ok_or_else(|| {
+            anyhow!(
+                "Expected {} to have a parent folder",
+                package_config.path.to_string_lossy()
+            )
+        })
+        .map(|parent_path| helpers::package_path(parent_path, package_name))?;
+
+    // current folder + node_modules + package_name
+    let path_from_current_config = project_context
+        .current_config
+        .path
+        .parent()
+        .ok_or_else(|| {
+            anyhow!(
+                "Expected {} to have a parent folder",
+                project_context.current_config.path.to_string_lossy()
+            )
+        })
+        .map(|parent_path| package_path(parent_path, package_name))?;
+
+    // root folder + node_modules + package_name
+    let path_from_root = package_path(project_context.get_root_path(), package_name);
+    if path_from_current_package.exists() {
+        Ok(path_from_current_package)
+    } else if path_from_current_config.exists() {
+        Ok(path_from_current_config)
+    } else if path_from_root.exists() {
+        Ok(path_from_root)
+    } else {
+        Err(anyhow!(
+            "The package \"{package_name}\" is not found (are node_modules up-to-date?)..."
+        ))
+    }
+}
+
 pub fn get_abs_path(path: &Path) -> PathBuf {
     let abs_path_buf = PathBuf::from(path);