Don't panic when dev files are not found during clean. #7622
Conversation
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/win32-x64
commit: |
| pub enum DevDeps { | ||
| Build, | ||
| DontBuild, | ||
| Clean, |
There was a problem hiding this comment.
I don't understand this variant. We want to know if we need to build dev deps or not. Cleaning is a separate task, not a property of we need to build dev deps or not. Whether to clean can be derived from if we want to build or not (if we build we also need to clean). Or am I misunderstanding this?
| _ => match read_folders(filter, package_dir, path_dir, recurse) { | ||
| Ok(files) => map.extend(files), | ||
|
|
||
| Err(_e) => log::error!( | ||
| Err(_e) if !is_clean => log::error!( |
There was a problem hiding this comment.
why would this ever happen, dev_deps is by definition DontBuild here, and is_clean is defined as dev_deps being DevDeps::Clean
There was a problem hiding this comment.
Right now, clean is trying to remove files that are listed as type: "dev".
This happens for third-party packages inside the node_modules.
These files can be absent when published to npm.
Test files are a good example of that: https://github.com/rescript-lang/experimental-rescript-webapi/blob/d1645e31cd4f46e1b3a1bed0156e7b6930f8ea5d/rescript.json#L10-L14
|
I don't see the part where it panics, I only see a error log not being printed - but it looks like it's never printed anymore so I don't think that makes a lot of sense |
| @@ -346,7 +347,7 @@ pub fn clean( | |||
| show_progress, | |||
| // Build the package tree with dev dependencies. | |||
| // They should always be cleaned if they are there. | |||
| true, | |||
| DevDeps::Clean, | |||
There was a problem hiding this comment.
This used to be true, so dev files are included.
Yet, these files never existed in the first place because they were never shipped as part of npm package.
| ) -> Result<AHashMap<String, Package>> { | ||
| let map = read_packages(root_folder, workspace_root, show_progress)?; | ||
|
|
||
| /* Once we have the deduplicated packages, we can add the source files for each - to minimize | ||
| * the IO */ | ||
| let result = extend_with_children(filter, map, build_dev_deps); | ||
| let result = extend_with_children(filter, map, dev_deps); |
There was a problem hiding this comment.
Cleaning and building share this code.
|
@jfrolich steps to reproduce: bun create rescript-app@next
cd your-project
bun i @rescript/webapi@experimentaladd Run bunx rescript cleanAgain, folder https://github.com/rescript-lang/experimental-rescript-webapi/tree/main/tests not part of published npm package, https://github.com/rescript-lang/experimental-rescript-webapi/blob/d1645e31cd4f46e1b3a1bed0156e7b6930f8ea5d/rescript.json#L10-L14
rescript/rewatch/src/build/clean.rs Lines 342 to 349 in 6deee63
rescript/rewatch/src/build/packages.rs Line 520 in 91a322b |
This fixes #7617.
I do still need the change of #7604 for my project.As for this PR, this is beginner Rustacean code, so let me know what can be improved.