diff --git a/CHANGELOG.md b/CHANGELOG.md index eae7f7a28a..1bb621f541 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,8 @@ #### :bug: Bug fix +- Fix ppx resolution with package inside monorepo. https://github.com/rescript-lang/rescript/pull/7776 + #### :memo: Documentation #### :nail_care: Polish diff --git a/rewatch/src/build.rs b/rewatch/src/build.rs index c7c4cead3f..d0b89b7110 100644 --- a/rewatch/src/build.rs +++ b/rewatch/src/build.rs @@ -80,7 +80,7 @@ pub fn get_compiler_args(rescript_file_path: &Path) -> Result { &project_context.current_config, relative_filename, &contents, - ); + )?; let is_interface = filename.to_string_lossy().ends_with('i'); let has_interface = if is_interface { true diff --git a/rewatch/src/build/packages.rs b/rewatch/src/build/packages.rs index 73fa603943..8aabaa8936 100644 --- a/rewatch/src/build/packages.rs +++ b/rewatch/src/build/packages.rs @@ -255,48 +255,7 @@ pub fn read_dependency( package_config: &Config, project_context: &ProjectContext, ) -> Result { - // 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() diff --git a/rewatch/src/build/parse.rs b/rewatch/src/build/parse.rs index 375c5e9abe..29d973430c 100644 --- a/rewatch/src/build/parse.rs +++ b/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) { +) -> anyhow::Result<(PathBuf, Vec)> { 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), String> { +) -> anyhow::Result<(PathBuf, Option)> { 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 diff --git a/rewatch/src/config.rs b/rewatch/src/config.rs index 3435859e4b..43391a9f22 100644 --- a/rewatch/src/config.rs +++ b/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 { /// 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>>, - package_name: &String, -) -> Vec { +) -> Result> { 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::>() .join(" "), - ] + ); } - }) - .collect::>(), + }; + Ok(acc) + }), } } diff --git a/rewatch/src/helpers.rs b/rewatch/src/helpers.rs index b3115f528d..9a85f94e99 100644 --- a/rewatch/src/helpers.rs +++ b/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 { + // 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);