-
Notifications
You must be signed in to change notification settings - Fork 469
Read bs-dev-dependencies if --dev was passed. #7650
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
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/win32-x64
commit: |
Okay, I discovered something interesting here. The rules of https://rescript-lang.org/docs/manual/v12.0.0/build-configuration#bs-dependencies-bs-dev-dependencies are not respected in Rewatch right now. I can have something like: {
"name": "dev-dep-sample",
"sources": [
{
"dir": "src",
"subdirs": true
},
{
"dir": "test",
"subdirs": true,
"type": "dev"
}],
"package-specs": {
"module": "esmodule",
"in-source": true
},
"suffix": ".res.mjs",
"bs-dependencies": [],
"bs-dev-dependencies": [
"@rescript/webapi"
],
"bsc-flags": []
} And
where
rescript/rewatch/src/build/compile.rs Lines 351 to 364 in 556545d
type: dev and can thus use bs-dev-dependencies.
|
Hmm, this is proving to be somewhat difficult. However, for rescript/rewatch/src/build/compile.rs Lines 360 to 362 in 0c36cb5
|
rewatch/src/build.rs
Outdated
packages_map.insert(package_name.clone(), package); | ||
// We need to populate the source_files meta data. | ||
// TODO: filter with current file | ||
let packages_map = packages::extend_with_children( |
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.
This is very expensive, this would scan all the files in the package. I think it would be better to see if the current file matches any of the dev paths.
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.
The idea of this command that it's a very low-latency for the editor tooling to know the compile arguments. Reading all the files would create a lot of overhead.
rewatch/src/build/compile.rs
Outdated
@@ -583,6 +581,7 @@ fn compile_file( | |||
}?; | |||
let module_name = helpers::file_path_to_module_name(implementation_file_path, &package.namespace); | |||
let has_interface = module.get_interface().is_some(); | |||
let is_type_dev = package.is_source_file_type_dev(implementation_file_path); |
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.
this is a semi slow lookup, if we compile a file we usually construct it from the module struct, that module struct has a field if it's a local dep. Looking up the module again from the package is unneeded, we can just pass the module attribute to this function instead.
@@ -19,6 +19,7 @@ use std::time::SystemTime; | |||
#[derive(Debug, Clone)] | |||
pub struct SourceFileMeta { | |||
pub modified: SystemTime, | |||
pub is_type_dev: bool, |
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.
I don't think it's relevant to put here, I would put it on the Module struct
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.
We would still need it here if we want to put it in Module struct.
packages::parse_packages
will loop over all the source_files and by that time, we no longer know what "type" they had.
@@ -186,9 +186,6 @@ pub enum Command { | |||
/// Path to a rescript file (.res or .resi) | |||
#[command()] | |||
path: String, | |||
|
|||
#[command(flatten)] | |||
dev: DevArg, |
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.
we don't have the explicit command to build the dev deps anymore?
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.
We do, that is part of BuildArgs.
Here this is about the compile-args
which the tooling calls to figure out the arguments. Tooling does not know if a file is type:dev or not.
@jfrolich I added some code to traverse the rescript.json in case of I'm unsure how to proceed with the |
Fixes #7638, I believe.
I tested https://github.com/dsiu/rewatch-dev-deps-build-test locally.
I will add a test later, but please feel free to take a look and let me know if this logic is correct.