diff --git a/CHANGELOG.md b/CHANGELOG.md index 7655158757..7c4b6b0f52 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ #### :bug: Bug fix +- Rewatch: rebuild source dependencies when the consuming project's package output settings change. https://github.com/rescript-lang/rescript/pull/8540 - Preserve multibyte characters when wrapping long source lines in compiler code frames. https://github.com/rescript-lang/rescript/pull/8520 #### :memo: Documentation diff --git a/rewatch/src/build.rs b/rewatch/src/build.rs index 1c02d28da6..f3e6186e24 100644 --- a/rewatch/src/build.rs +++ b/rewatch/src/build.rs @@ -11,7 +11,9 @@ pub mod read_compile_state; use self::parse::parser_args; use crate::build::compile::{mark_modules_with_deleted_deps_dirty, mark_modules_with_expired_deps_dirty}; -use crate::build::compiler_info::{CompilerCheckResult, verify_compiler_info, write_compiler_info}; +use crate::build::compiler_info::{ + CompilerCheckResult, get_package_output_specs, verify_compiler_info, write_compiler_info, +}; use crate::config::SourceMapCommand; use crate::helpers::emojis::*; use crate::helpers::{self}; @@ -191,7 +193,8 @@ pub fn initialize_build( let source_map_args = project_context .get_root_config() .get_source_map_args(source_map_command); - let compiler_check = verify_compiler_info(&packages, &compiler, &source_map_args); + let package_output_specs = get_package_output_specs(project_context.get_root_config()); + let compiler_check = verify_compiler_info(&packages, &compiler, &source_map_args, &package_output_specs); if !packages::validate_packages_dependencies(&packages) { return Err(anyhow!("Failed to validate package dependencies")); diff --git a/rewatch/src/build/clean.rs b/rewatch/src/build/clean.rs index 7fe006c728..041f756b42 100644 --- a/rewatch/src/build/clean.rs +++ b/rewatch/src/build/clean.rs @@ -33,7 +33,7 @@ fn remove_iast(package: &packages::Package, source_file: &Path) { )); } -fn remove_mjs_file(source_file: &Path, suffix: &str) { +pub(crate) fn remove_js_file(source_file: &Path, suffix: &str) { let js_file = source_file.with_extension( // suffix.to_string includes the ., so we need to remove it &suffix[1..], @@ -102,7 +102,7 @@ fn clean_source_files(build_state: &BuildState, root_config: &Config) { rescript_file_locations .par_iter() - .for_each(|(rescript_file_location, suffix)| remove_mjs_file(rescript_file_location, suffix)); + .for_each(|(rescript_file_location, suffix)| remove_js_file(rescript_file_location, suffix)); } // TODO: change to scan_previous_build => CompileAssetsState @@ -145,7 +145,7 @@ pub fn cleanup_previous_build( .get(package_name) .expect("Could not find package"); remove_compile_assets(package, res_file_location); - remove_mjs_file(res_file_location, suffix); + remove_js_file(res_file_location, suffix); remove_iast(package, res_file_location); remove_ast(package, res_file_location); match helpers::get_extension(ast_file_path).as_str() { diff --git a/rewatch/src/build/compiler_info.rs b/rewatch/src/build/compiler_info.rs index dddacc8ee1..233cfabfe3 100644 --- a/rewatch/src/build/compiler_info.rs +++ b/rewatch/src/build/compiler_info.rs @@ -9,6 +9,25 @@ use serde::{Deserialize, Serialize}; use std::fs::File; use std::io::Write; +#[derive(Serialize, Deserialize, Debug, PartialEq)] +pub(crate) struct PackageOutputSpec { + module: String, + in_source: bool, + suffix: String, +} + +pub(crate) fn get_package_output_specs(config: &crate::config::Config) -> Vec { + config + .get_package_specs() + .iter() + .map(|spec| PackageOutputSpec { + module: spec.module.as_str().to_string(), + in_source: spec.in_source, + suffix: config.get_suffix(spec), + }) + .collect() +} + // In order to have a loose coupling with the compiler, we don't want to have a hard dependency on the compiler's structs // We can use this struct to parse the compiler-info.json file // If something is not there, that is fine, we will treat it as a mismatch @@ -19,6 +38,7 @@ struct CompilerInfoFile { bsc_hash: String, rescript_config_hash: String, source_map_args: Vec, + package_output_specs: Vec, runtime_path: String, generated_at: String, } @@ -28,29 +48,61 @@ pub enum CompilerCheckResult { CleanedPackagesDueToCompiler, } +fn remove_package_outputs(package: &packages::Package, output_specs: &[PackageOutputSpec]) { + let Some(source_files) = &package.source_files else { + return; + }; + + for output_spec in output_specs { + let output_dir = match output_spec.module.as_str() { + "commonjs" => package.get_js_path(), + "esmodule" => package.get_esmodule_path(), + _ => continue, + }; + + source_files + .keys() + .filter(|source_file| { + source_file + .extension() + .and_then(|extension| extension.to_str()) + .is_some_and(helpers::is_implementation_file) + }) + .for_each(|source_file| { + let source_file = if output_spec.in_source { + package.path.join(source_file) + } else { + output_dir.join(source_file) + }; + clean::remove_js_file(&source_file, &output_spec.suffix); + }); + } +} + fn get_rescript_config_hash(package: &packages::Package) -> Option { helpers::compute_file_hash(&package.config.path).map(|hash| hash.to_hex().to_string()) } -pub fn verify_compiler_info( +pub(crate) fn verify_compiler_info( packages: &AHashMap, compiler: &CompilerInfo, source_map_args: &[String], + package_output_specs: &[PackageOutputSpec], ) -> CompilerCheckResult { let mismatched_packages = packages .values() - .filter(|package| { + .filter_map(|package| { let info_path = package.get_compiler_info_path(); let Ok(contents) = std::fs::read_to_string(&info_path) else { // Can't read the compiler-info.json file, maybe there is no current build. // We check if the ocaml build folder exists, if not, we assume the compiler is not installed - return logs::does_ocaml_build_compiler_log_exist(package); + return logs::does_ocaml_build_compiler_log_exist(package).then_some((package, None)); }; let parsed: Result = serde_json::from_str(&contents); let parsed = match parsed { Ok(p) => p, - Err(_) => return true, // unknown or invalid format -> treat as mismatch + Err(_) => return Some((package, None)), // unknown or invalid format -> treat as mismatch }; let current_bsc_path_str = compiler.bsc_path.to_string_lossy(); @@ -58,7 +110,7 @@ pub fn verify_compiler_info( let current_runtime_path_str = compiler.runtime_path.to_string_lossy(); let current_rescript_config_hash = match get_rescript_config_hash(package) { Some(hash) => hash, - None => return true, // can't compute hash -> treat as mismatch + None => return Some((package, None)), // can't compute hash -> treat as mismatch }; let mut mismatch = false; @@ -107,16 +159,35 @@ pub fn verify_compiler_info( ); mismatch = true; } + let package_output_specs_changed = parsed.package_output_specs != package_output_specs; + if package_output_specs_changed { + log::debug!( + "compiler-info mismatch for {}: package_output_specs changed (stored={:?}, current={:?})", + package.name, + parsed.package_output_specs, + package_output_specs + ); + mismatch = true; + } - mismatch + mismatch.then(|| { + let previous_output_specs = + package_output_specs_changed.then_some(parsed.package_output_specs); + (package, previous_output_specs) + }) }) .collect::>(); let cleaned_count = mismatched_packages.len(); - mismatched_packages.par_iter().for_each(|package| { - // suppress progress printing during init to avoid breaking step output - clean::clean_package(false, true, package); - }); + mismatched_packages + .into_par_iter() + .for_each(|(package, previous_output_specs)| { + if let Some(previous_output_specs) = previous_output_specs { + remove_package_outputs(package, &previous_output_specs); + } + // suppress progress printing during init to avoid breaking step output + clean::clean_package(false, true, package); + }); if cleaned_count == 0 { CompilerCheckResult::SameCompilerAsLastRun } else { @@ -124,7 +195,7 @@ pub fn verify_compiler_info( } } -pub fn write_compiler_info(build_state: &BuildCommandState) { +pub(crate) fn write_compiler_info(build_state: &BuildCommandState) { let bsc_path = build_state.compiler_info.bsc_path.to_string_lossy().to_string(); let bsc_hash = build_state.compiler_info.bsc_hash.to_hex().to_string(); let runtime_path = build_state @@ -135,6 +206,7 @@ pub fn write_compiler_info(build_state: &BuildCommandState) { let source_map_args = build_state .get_root_config() .get_source_map_args(build_state.source_map_command); + let package_output_specs = get_package_output_specs(build_state.get_root_config()); // derive version from the crate version let version = env!("CARGO_PKG_VERSION").to_string(); let generated_at = crate::helpers::get_system_time().to_string(); @@ -147,6 +219,7 @@ pub fn write_compiler_info(build_state: &BuildCommandState) { bsc_hash: &'a str, rescript_config_hash: String, source_map_args: &'a [String], + package_output_specs: &'a [PackageOutputSpec], runtime_path: &'a str, generated_at: &'a str, } @@ -159,6 +232,7 @@ pub fn write_compiler_info(build_state: &BuildCommandState) { bsc_hash: &bsc_hash, rescript_config_hash: rescript_config_hash.to_hex().to_string(), source_map_args: &source_map_args, + package_output_specs: &package_output_specs, runtime_path: &runtime_path, generated_at: &generated_at, }; @@ -225,12 +299,13 @@ pub fn write_compiler_info(build_state: &BuildCommandState) { #[cfg(test)] mod tests { use super::*; - use crate::build::packages::{Namespace, Package}; + use crate::build::packages::{Namespace, Package, SourceFileMeta}; use crate::config; use ahash::{AHashMap, AHashSet}; use serde_json::json; use std::fs; use std::path::Path; + use std::time::SystemTime; use tempfile::TempDir; fn test_compiler(root: &Path) -> CompilerInfo { @@ -268,7 +343,12 @@ mod tests { } } - fn write_test_compiler_info(package: &Package, compiler: &CompilerInfo, source_map_args: Vec<&str>) { + fn write_test_compiler_info( + package: &Package, + compiler: &CompilerInfo, + source_map_args: Vec<&str>, + package_output_specs: &[PackageOutputSpec], + ) { fs::create_dir_all(package.get_build_path()).expect("build directory should be created"); fs::create_dir_all(package.get_ocaml_build_path()).expect("ocaml build directory should be created"); @@ -280,6 +360,7 @@ mod tests { "bsc_hash": compiler.bsc_hash.to_hex().to_string(), "rescript_config_hash": rescript_config_hash, "source_map_args": source_map_args, + "package_output_specs": package_output_specs, "runtime_path": compiler.runtime_path.to_string_lossy().to_string(), "generated_at": "test", }); @@ -304,9 +385,20 @@ mod tests { let package = test_package(temp_dir.path(), "dep"); let build_path = package.get_build_path(); let source_map_args = vec!["-bs-source-map".to_string(), "linked".to_string()]; - write_test_compiler_info(&package, &compiler, vec!["-bs-source-map", "linked"]); + let package_output_specs = get_package_output_specs(&package.config); + write_test_compiler_info( + &package, + &compiler, + vec!["-bs-source-map", "linked"], + &package_output_specs, + ); - let result = verify_compiler_info(&packages_map(package), &compiler, &source_map_args); + let result = verify_compiler_info( + &packages_map(package), + &compiler, + &source_map_args, + &package_output_specs, + ); assert!(matches!(result, CompilerCheckResult::SameCompilerAsLastRun)); assert!(build_path.exists()); @@ -319,14 +411,70 @@ mod tests { let package = test_package(temp_dir.path(), "dep"); let build_path = package.get_build_path(); let source_map_args = vec!["-bs-source-map".to_string(), "linked".to_string()]; - write_test_compiler_info(&package, &compiler, vec!["-bs-source-map", "false"]); + let package_output_specs = get_package_output_specs(&package.config); + write_test_compiler_info( + &package, + &compiler, + vec!["-bs-source-map", "false"], + &package_output_specs, + ); + + let result = verify_compiler_info( + &packages_map(package), + &compiler, + &source_map_args, + &package_output_specs, + ); + + assert!(matches!( + result, + CompilerCheckResult::CleanedPackagesDueToCompiler + )); + assert!(!build_path.exists()); + } + + #[test] + fn verify_compiler_info_cleans_package_when_package_output_specs_change() { + let temp_dir = TempDir::new().expect("temp dir should be created"); + let compiler = test_compiler(temp_dir.path()); + let mut package = test_package(temp_dir.path(), "dep"); + let build_path = package.get_build_path(); + let source_file = Path::new("src/Dep.res").to_path_buf(); + package.source_files = Some(AHashMap::from_iter([( + source_file.clone(), + SourceFileMeta { + modified: SystemTime::now(), + is_type_dev: false, + }, + )])); + let previous_output = package.get_js_path().join(source_file).with_extension("cjs"); + fs::create_dir_all(previous_output.parent().expect("output should have a parent")) + .expect("output directory should be created"); + fs::write(&previous_output, "generated output").expect("previous output should be written"); + fs::write(previous_output.with_extension("cjs.map"), "source map") + .expect("previous source map should be written"); + let source_map_args = Vec::new(); + let package_output_specs = get_package_output_specs(&package.config); + let previous_package_output_specs = vec![PackageOutputSpec { + module: "commonjs".to_string(), + in_source: false, + suffix: ".cjs".to_string(), + }]; + write_test_compiler_info(&package, &compiler, vec![], &previous_package_output_specs); - let result = verify_compiler_info(&packages_map(package), &compiler, &source_map_args); + let result = verify_compiler_info( + &packages_map(package), + &compiler, + &source_map_args, + &package_output_specs, + ); assert!(matches!( result, CompilerCheckResult::CleanedPackagesDueToCompiler )); assert!(!build_path.exists()); + assert!(!previous_output.exists()); + assert!(!previous_output.with_extension("cjs.map").exists()); } } diff --git a/rewatch/tests/compile/20-rebuild-dependency-for-package-specs.sh b/rewatch/tests/compile/20-rebuild-dependency-for-package-specs.sh new file mode 100755 index 0000000000..10efb66eed --- /dev/null +++ b/rewatch/tests/compile/20-rebuild-dependency-for-package-specs.sh @@ -0,0 +1,83 @@ +#!/bin/bash +# Verifies that a source dependency is rebuilt when the consuming project's +# package output settings change. + +cd $(dirname $0) +source "../utils.sh" + +bold "Test: Rebuild dependencies when package specs change" + +fixture=$(mktemp -d 2>/dev/null || mktemp -d -t rewatch-package-specs) +trap "rm -rf '$fixture'" EXIT + +mkdir -p "$fixture/src" +mkdir -p "$fixture/node_modules/shared-dep/src" + +cat > "$fixture/package.json" <<'EOF' +{ + "name": "host", + "version": "0.0.1" +} +EOF + +cat > "$fixture/rescript.json" <<'EOF' +{ + "name": "host", + "sources": { "dir": "src" }, + "dependencies": ["shared-dep"], + "package-specs": { "module": "commonjs", "in-source": false, "suffix": ".cjs" } +} +EOF + +cat > "$fixture/src/Main.res" <<'EOF' +let value = SharedDep.value +EOF + +cat > "$fixture/node_modules/shared-dep/package.json" <<'EOF' +{ + "name": "shared-dep", + "version": "0.0.1" +} +EOF + +cat > "$fixture/node_modules/shared-dep/rescript.json" <<'EOF' +{ + "name": "shared-dep", + "sources": { "dir": "src" } +} +EOF + +cat > "$fixture/node_modules/shared-dep/src/SharedDep.res" <<'EOF' +let value = 42 +EOF + +cd "$fixture" +rewatch build + +if [ ! -f "node_modules/shared-dep/lib/js/src/SharedDep.cjs" ]; then + error "Expected CommonJS dependency output" + exit 1 +fi + +cat > "$fixture/rescript.json" <<'EOF' +{ + "name": "host", + "sources": { "dir": "src" }, + "dependencies": ["shared-dep"], + "package-specs": { "module": "esmodule", "in-source": false, "suffix": ".mjs" } +} +EOF + +rewatch build + +if [ ! -f "node_modules/shared-dep/lib/es6/src/SharedDep.mjs" ]; then + error "Expected ES module dependency output after changing package specs" + exit 1 +fi + +if [ -f "node_modules/shared-dep/lib/js/src/SharedDep.cjs" ]; then + error "Expected previous CommonJS dependency output to be removed" + exit 1 +fi + +success "Source dependency was rebuilt for the new package specs" diff --git a/rewatch/tests/suite.sh b/rewatch/tests/suite.sh index 7c7eb518be..a27ef61e69 100755 --- a/rewatch/tests/suite.sh +++ b/rewatch/tests/suite.sh @@ -129,6 +129,7 @@ fi ./compile/17-prod-flag.sh && ./compile/18-external-dep-uncurried-dot.sh && ./compile/19-utf8-warning.sh && +./compile/20-rebuild-dependency-for-package-specs.sh && ./compile/14-no-testrepo-changes.sh && ./compile/15-no-new-files.sh && ./compile/16-snapshots-unchanged.sh && diff --git a/rewatch/tests/utils.sh b/rewatch/tests/utils.sh index eee37596b5..888a6ff372 100644 --- a/rewatch/tests/utils.sh +++ b/rewatch/tests/utils.sh @@ -99,3 +99,15 @@ wait_for_file_gone() { done return 1 } + +wait_for_clean_worktree() { + local path="$1"; local timeout="${2:-30}" + while [ "$timeout" -gt 0 ]; do + if git diff --quiet -- "$path" && [ -z "$(git ls-files --others --exclude-standard -- "$path")" ]; then + return 0 + fi + sleep 1 + timeout=$((timeout - 1)) + done + return 1 +} diff --git a/rewatch/tests/watch/04-watch-config-change.sh b/rewatch/tests/watch/04-watch-config-change.sh index 519f77f5e8..57e63f0dcd 100755 --- a/rewatch/tests/watch/04-watch-config-change.sh +++ b/rewatch/tests/watch/04-watch-config-change.sh @@ -51,6 +51,15 @@ else exit 1 fi +if ! wait_for_file_gone "./src/Test.mjs" 20; then + error "Previous suffix output was not removed after config change" + cat rewatch.log + replace "s/.res.mjs/.mjs/g" rescript.json + git checkout -- ./src/Test.res + exit_watcher + exit 1 +fi + # Verify the watcher is still running (didn't crash on config change) if [ -f lib/watch.lock ]; then success "Watcher still running after config change" @@ -66,12 +75,18 @@ fi replace "s/.res.mjs/.mjs/g" rescript.json git checkout -- ./src/Test.res -# Wait for rebuild with restored suffix (old .res.mjs should go away) -if wait_for_file_gone "./src/Test.res.mjs" 20; then - success "Rebuild after restore removed old suffix files" +# Wait for the restored build to finish before stopping the watcher. Merely +# waiting for the old output to disappear only observes the cleanup phase and +# can interrupt the slower Windows rebuild before tracked outputs are recreated. +if wait_for_clean_worktree . 30; then + success "Rebuild after restore removed old suffix files and restored outputs" else - # Clean up manually if the watcher didn't remove them - find . -name "*.res.mjs" -delete 2>/dev/null + error "Rebuild after restore did not finish" + cat rewatch.log + exit_watcher + git diff . + git ls-files --others --exclude-standard . + exit 1 fi exit_watcher