Reuse package resolution for ppx #7776
Conversation
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/win32-x64
commit: |
isn't this covered by the testrepo |
No, yarn installed sury at the root level. And you can't force it to install at the package level. You really need resolution problem before that can happen. |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors and reuses package resolution logic for ppx (preprocessor extension) handling in monorepos. Previously, dependency resolution and ppx resolution used different logic, which caused issues when packages existed at different node_modules levels in a workspace.
- Extracted common package resolution logic into a reusable
try_package_pathfunction - Updated ppx flag processing to use the same resolution strategy as regular dependencies
- Changed error handling from strings to proper
anyhow::Resulttypes throughout the parsing pipeline
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| rewatch/src/helpers.rs | Added try_package_path function that implements multi-level package resolution logic |
| rewatch/src/config.rs | Updated flatten_ppx_flags to use new resolution logic and proper error handling |
| rewatch/src/build/parse.rs | Changed function signatures to return anyhow::Result and updated error handling |
| rewatch/src/build/packages.rs | Replaced inline resolution logic with call to new try_package_path function |
| rewatch/src/build.rs | Updated to handle new error-returning signature |
| CHANGELOG.md | Added entry documenting the ppx resolution fix |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| let path = helpers::try_package_path( | ||
| package_config, | ||
| project_context, | ||
| format!("{}{}{}", &package_config.name, MAIN_SEPARATOR, y).as_str(), |
There was a problem hiding this comment.
The string formatting with MAIN_SEPARATOR could be simplified using Path::join() which handles path separators correctly across platforms.
| format!("{}{}{}", &package_config.name, MAIN_SEPARATOR, y).as_str(), | |
| Path::new(&package_config.name).join(y).to_str().unwrap(), |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
I disagree with this really, making a Path and calling unwrap later seems worse.
There was a problem hiding this comment.
Why not use path concat, I think this is the way to constructs paths, and it works on every platform. Generally in the codebase I use to_string_lossy and then you don't need unwrap.
There was a problem hiding this comment.
btw the function try_package_path should recieve a Path
There was a problem hiding this comment.
so it receives a package name or a path relative to the current package I think? Perhaps we can pass in a variant of Name(String) | Path({package_name, path}) or something like that?
There was a problem hiding this comment.
btw the function try_package_path should recieve a Path
@jfrolich you mean pass package_name as Path?
| Some('.') => helpers::try_package_path( | ||
| package_config, | ||
| project_context, | ||
| format!("{}{}{}", package_config.name, MAIN_SEPARATOR, &ys[0]).as_str(), |
There was a problem hiding this comment.
Similar to the previous occurrence, using Path::join() would be more idiomatic and handle cross-platform path separators automatically.
| format!("{}{}{}", package_config.name, MAIN_SEPARATOR, &ys[0]).as_str(), | |
| Path::new(&package_config.name).join(&ys[0]).to_string_lossy().as_ref(), |
Copilot uses AI. Check for mistakes.
|
Ok I think it did at some point and that's why the test used to fail, but well, yarn works in mysterious ways haha. Let's merge this, I think this is the last preventing me from using rewatch everywhere without workarounds. |
|
|
||
| // root folder + node_modules + package_name | ||
| let path_from_root = package_path(project_context.get_root_path(), package_name); | ||
| if path_from_current_package.exists() { |
There was a problem hiding this comment.
the exists are quite expensive for every ppx run (== every file), can we not have one resolution per situation?
Like ./my_ppx/bla always resolves to the the current package, @my_package/ppx always resolves to the location in the package (which we scanned so we know).
There was a problem hiding this comment.
This of course specifically for the ppx resolution, and I know this is also used for the package scanning.
For package scanning, I think we can make path_from_current_config and path_from_current_package more efficient because they are the same in 99% of the cases, if they are the same we don't have to check both.
There was a problem hiding this comment.
I guess I am saying that for ppx resolution we can probably create a new function that can look up the paths for the package based on the scanning that was done earlier instead of scanning again (and special casing the relative path).
This is another attempt to solve ppx resolution in monorepos.
The gist is that some workspaces can have
node_moduleson the root and the package level.We take this into account for finding dependencies but didn't for ppx.
In this PR, I've refactored the same resolution code and reused it when looking for ppx.
I didn't add a test as it is very difficult to reproduce this in a yarn workspace.
It is my understanding that yarn will only show this behaviour when it fails to resolve a single version for the entire workspace.
@tsnobip could you give this one a go?