- Notifications
You must be signed in to change notification settings - Fork471
Traverse upwards for single context projects#7896
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
pkg-pr-newbot commentedSep 17, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
rescript@rescript/darwin-arm64@rescript/darwin-x64@rescript/linux-arm64@rescript/linux-x64@rescript/runtime@rescript/win32-x64commit: |
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 implements upward directory traversal for package resolution in single context projects, allowing ReScript to find dependencies in parent node_modules directories when building standalone packages in workspace setups.
Key changes include:
- Added upward traversal logic that searches parent directories for node_modules when dependencies aren't found locally
- Implemented caching mechanism to optimize repeated path existence checks during traversal
- Added comprehensive test coverage for standalone package builds
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| rewatch/src/helpers.rs | Core implementation of upward traversal logic with caching for package resolution |
| rewatch/tests/compile.sh | Added test case for standalone package building via rescript command |
| rewatch/testrepo/packages/standalone/* | Test fixtures for standalone package with dependency on @testrepo/dep01 |
| rewatch/testrepo/package.json | Added standalone package to workspace and updated scripts to use rescript instead of rewatch |
| CHANGELOG.md | Documented the new upward traversal feature |
Tip: Customize your code reviews with copilot-instructions.md.Create the file orlearn how to get started.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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.
👍 LGTM, but would be great if someone else could review, too.
@codex review |
Codex Review: Didn't find any major issues. Keep them coming! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. |
rewatch/src/helpers.rs Outdated
| // Cache existence checks for candidate node_modules paths during upward traversal. | ||
| // Keyed by the absolute candidate path; value is the existence boolean. | ||
| staticNODE_MODULES_EXIST_CACHE:LazyLock<RwLock<ahash::AHashMap<PathBuf,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.
can we not put this in the build state instead of a static value?
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.
It is used beforeBuildState is created,packages::make already needs it.
I was able to move it to ProjectContext:
Key point:
ProjectContextexists beforeBuildState. We resolve packages (e.g.helpers::try_package_path) during config/dependency reading, which happens pre-BuildState, and those call sites already have a&ProjectContext.Correct scope/lifetime: The cache models workspace layout (node_modules existence), not per-build artifacts.
BuildStatemay be recreated in watch cycles;ProjectContextpersists, so the cache isn’t thrown away.Concurrency and isolation: A per-
ProjectContextRwLockallows concurrent reads and avoids process-wide statics and cross-project cache bleed.Lower coupling/less churn: Callers already pass
ProjectContext; using it avoids threadingBuildStatethroughconfig.rsandbuild/packages.rsand reduces borrow/contention with the highly mutableBuildState.
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.
Great!
I'm also incorporating the changes of#7890 as it is a similar cache. |
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
Copilot reviewed 10 out of 11 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md.Create the file orlearn how to get started.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
84e45c5 intorescript-lang:masterUh oh!
There was an error while loading.Please reload this page.
This resolves the upwards traversal as discussed in#7117 (comment)