-
Notifications
You must be signed in to change notification settings - Fork 469
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
base: master
Are you sure you want to change the base?
Conversation
Another problem I see here is that the inheritance part of the rescript.json isn't quite respected in: rescript/rewatch/src/build/compile.rs Lines 364 to 396 in c092acd
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. |
Okay, another interesting finding is that running a top-level 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!( |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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. |
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
andpackage.json
.I also had some instances where a filename was already in use. (
Tuples.res
vsnested/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.