Skip to content

Fix crash on missing dev dependency #161

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
May 6, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
106 changes: 80 additions & 26 deletions src/build/compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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());

Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down
4 changes: 2 additions & 2 deletions src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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")));
Expand Down
3 changes: 2 additions & 1 deletion testrepo/packages/with-dev-deps/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
"author": "",
"license": "MIT",
"dependencies": {
"rescript": "*"
"rescript": "*",
"@rescript/core": "*"
}
}
1 change: 1 addition & 0 deletions testrepo/packages/with-dev-deps/rescript.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,6 @@
"module": "es6",
"in-source": true
},
"bs-dependencies": ["@rescript/core"],
"suffix": ".res.js"
}
11 changes: 8 additions & 3 deletions testrepo/yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -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==
28 changes: 17 additions & 11 deletions tests/compile.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why remove no-timing?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh duplicated... i see 👍

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
Expand Down
2 changes: 1 addition & 1 deletion tests/snapshots/dependency-cycle.txt
Original file line number Diff line number Diff line change
@@ -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
Expand Down
2 changes: 1 addition & 1 deletion tests/snapshots/remove-file.txt
Original file line number Diff line number Diff line change
@@ -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
Expand Down
2 changes: 1 addition & 1 deletion tests/snapshots/rename-file-internal-dep-namespace.txt
Original file line number Diff line number Diff line change
@@ -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
Expand Down
2 changes: 1 addition & 1 deletion tests/snapshots/rename-file-internal-dep.txt
Original file line number Diff line number Diff line change
@@ -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
Expand Down
2 changes: 1 addition & 1 deletion tests/snapshots/rename-file-with-interface.txt
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
 No implementation file found for interface file (skipping): src/ModuleWithInterface.resi
[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 1 source files in 0.00s
[6/7] 🌴 Collected deps in 0.00s
[7/7] 🤺 Compiled 2 modules in 0.00s
Expand Down
2 changes: 1 addition & 1 deletion tests/snapshots/rename-file.txt
Original file line number Diff line number Diff line change
@@ -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 1 source files in 0.00s
[6/7] 🌴 Collected deps in 0.00s
[7/7] 🤺 Compiled 1 modules in 0.00s
Expand Down
2 changes: 1 addition & 1 deletion tests/snapshots/rename-interface-file.txt
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
 No implementation file found for interface file (skipping): src/ModuleWithInterface2.resi
[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 1 source files in 0.00s
[6/7] 🌴 Collected deps in 0.00s
[7/7] 🤺 Compiled 2 modules in 0.00s
Expand Down
4 changes: 2 additions & 2 deletions tests/suffix.sh
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,9 @@ else
fi

# Count files with new extension
file_count=$(find . -name *.res.js | wc -l)
file_count=$(find ./packages -name *.res.js | wc -l)

if [ "$file_count" -eq 26 ];
if [ "$file_count" -eq 24 ];
then
success "Found files with correct suffix"
else
Expand Down
2 changes: 1 addition & 1 deletion tests/utils.sh
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ overwrite() { echo -e "\r\033[1A\033[0K$@"; }
success() { echo -e "- ✅ \033[32m$1\033[0m"; }
error() { echo -e "- 🛑 \033[31m$1\033[0m"; }
bold() { echo -e "\033[1m$1\033[0m"; }
rewatch() { RUST_BACKTRACE=1 ../target/release/rewatch --no-timing=true $1; }
rewatch() { RUST_BACKTRACE=1 ../target/release/rewatch --no-timing=true $@; }

replace() {
if [[ $OSTYPE == 'darwin'* ]];
Expand Down