Skip to content

Mention which package was not found in the build state. #7696

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

Draft
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

nojaf
Copy link
Collaborator

@nojaf nojaf commented Jul 22, 2025

In this PR, I want to highlight the problem found in #7688

When inside a monorepo, you need to specify the root folder if the cwd is not that directory.

Running rescript clean ../../.. is the right thing to do for the analysis tests.

This change snowballed a bit, it required more corrections for other packages.
Names need to align in rescript.json and package.json.

I also had some instances where a filename was already in use. (Tuples.res vs nested/Tuples.res). This change seems correct, not sure why this wasn't a problem before.

Lastly, I've added some more text in the ReScript errors so the user knows why things are failing.

@nojaf
Copy link
Collaborator Author

nojaf commented Jul 22, 2025

Another problem I see here is that the inheritance part of the rescript.json isn't quite respected in:

let bsc_flags = config::flatten_flags(&config.compiler_flags);
let dependency_paths = get_dependency_paths(config, project_root, workspace_root, packages, is_type_dev);
let module_name = helpers::file_path_to_module_name(file_path, &config.get_namespace());
let namespace_args = match &config.get_namespace() {
packages::Namespace::NamespaceWithEntry { namespace: _, entry } if &module_name == entry => {
// if the module is the entry we just want to open the namespace
vec![
"-open".to_string(),
config.get_namespace().to_suffix().unwrap().to_string(),
]
}
packages::Namespace::Namespace(_)
| packages::Namespace::NamespaceWithEntry {
namespace: _,
entry: _,
} => {
vec![
"-bs-ns".to_string(),
config.get_namespace().to_suffix().unwrap().to_string(),
]
}
packages::Namespace::NoNamespace => vec![],
};
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();
let jsx_preserve_args = root_config.get_jsx_preserve_args();
let gentype_arg = config.get_gentype_arg();
let warning_args = config.get_warning_args(is_local_dep);

Root BSC flags won't be picked up.

Jsx args need to come from the root and can't be overridden by the package config.

@nojaf
Copy link
Collaborator Author

nojaf commented Jul 22, 2025

Okay, another interesting finding is that running a top-level rescript clean will purge the runtime assets in lib/bs.

So, doing a clean for the test analysis project (with the correct root folder argument) will remove the runtime as well.

@@ -65,7 +65,12 @@ fn clean_source_files(build_state: &BuildState, root_package: &packages::Package
.values()
.filter_map(|module| match &module.source_type {
SourceType::SourceFile(source_file) => {
let package = build_state.packages.get(&module.package_name).unwrap();
let package = build_state.packages.get(&module.package_name).unwrap_or_else(|| {
panic!(
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to bubble this up as a Result instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To what end, I wonder? This is a configuration error or cli argument error. Feels fine to fail her in my opinion.
We should, of course, communicate why it failed.

@jfrolich
Copy link
Contributor

Another problem I see here is that the inheritance part of the rescript.json isn't quite respected in:

let bsc_flags = config::flatten_flags(&config.compiler_flags);
let dependency_paths = get_dependency_paths(config, project_root, workspace_root, packages, is_type_dev);
let module_name = helpers::file_path_to_module_name(file_path, &config.get_namespace());
let namespace_args = match &config.get_namespace() {
packages::Namespace::NamespaceWithEntry { namespace: _, entry } if &module_name == entry => {
// if the module is the entry we just want to open the namespace
vec![
"-open".to_string(),
config.get_namespace().to_suffix().unwrap().to_string(),
]
}
packages::Namespace::Namespace(_)
| packages::Namespace::NamespaceWithEntry {
namespace: _,
entry: _,
} => {
vec![
"-bs-ns".to_string(),
config.get_namespace().to_suffix().unwrap().to_string(),
]
}
packages::Namespace::NoNamespace => vec![],
};
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();
let jsx_preserve_args = root_config.get_jsx_preserve_args();
let gentype_arg = config.get_gentype_arg();
let warning_args = config.get_warning_args(is_local_dep);

Root BSC flags won't be picked up.

Jsx args need to come from the root and can't be overridden by the package config.

BSC flags are from the local config (I think this is correct, this is a package local config because for some packages you want to set specific BSC flags. JSX is already obtained from the root config.

@nojaf
Copy link
Collaborator Author

nojaf commented Jul 23, 2025

BSC flags are from the local config (I think this is correct, this is a package local config because for some packages you want to set specific BSC flags. JSX is already obtained from the root config.

I can see scenarios where having a top-level BSC flag applied to all my projects would be beneficial. Manually copying those flags to each project does seem cumbersome.

Regarding JSX, I can also envision a use case where I have a separate package for my React components. In that case, using the "preserve JSX" flag and changing the file extension to JSX for that specific project would be ideal.

So, I'm unsure if those BSC flags and JSX settings should be fixed at a single level. The config inheritance, as I understand it, is certainly more complex to implement correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants