-
Notifications
You must be signed in to change notification settings - Fork 471
Cache try_package_path #7890
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
Cache try_package_path #7890
Conversation
rescript
@rescript/darwin-arm64
@rescript/darwin-x64
@rescript/linux-arm64
@rescript/linux-x64
@rescript/runtime
@rescript/win32-x64
commit: |
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 |
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Ok(path) => { | ||
cache.insert(cache_key, Some(path.clone())); | ||
} | ||
Err(_) => { | ||
cache.insert(cache_key, None); |
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.
[P1] Avoid caching missing packages without invalidation
The new global cache records both successful paths and None
for failures. Once a lookup stores None
, subsequent calls immediately return the cached error without touching the filesystem. Because the cache is never cleared (reset_try_package_path_cache
is unused), running rewatch
while node_modules
are temporarily incomplete—e.g. before or during an install—causes the tool to keep failing to resolve that package until the process is restarted. Previously, the helper retried on each call and would succeed after installation. Consider either invalidating the cache when dependencies change or skipping negative caching.
Useful? React with 👍 / 👎.
Superseded by #7896 |
Fixes #7785