-
Notifications
You must be signed in to change notification settings - Fork 471
Try to read dependency from the current package. #7770
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
Conversation
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.
Pull Request Overview
This PR fixes a regression introduced in #7752 by modifying the read_dependency
function to first check for dependencies in the current package's node_modules before falling back to the root node_modules directory.
- Added logic to check for dependencies in the current package's node_modules directory first
- Updated error handling to use
anyhow!
macro instead of string formatting - Changed return type from
Result<PathBuf, String>
toResult<PathBuf>
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/win32-x64
commit: |
@tsnobip this should resolve your problem I believe. |
@nojaf, the resolution of dependencies seems to work but we still have the issue of PPX resolution:
|
Okay, interesting, I have some idea about this. |
@@ -1,4 +1,4 @@ | |||
Cleaned 0/67 | |||
Cleaned 0/115 |
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 wonder why this changed so much
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 added rescript-bun
in that last test package.
I'll share it, no problem :) |
The structure of my monorepo is quite common I think, a yarn v4 monorepo, root My issue is with ppxes that are not in the root I think current ppx resolution only checks at the root. |
Yup, wanted to hear that part. This PR ensure we look at the package node_modules for dependencies but we don't do this in case of ppx. But should be the same mechanism. |
I broke this in #7752