fix: align package exports fallback array resolution with Node.js spec#177
fix: align package exports fallback array resolution with Node.js spec#177stormslowly wants to merge 2 commits intomainfrom
Conversation
The condition `resolved.is_err() && i == targets.len()` was always false (off-by-one) causing InvalidPackageTarget errors to be silently dropped when all targets in a fallback array are invalid. Also implement Debug for CachedPathImpl to display its path.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug in package resolution where Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes an off-by-one bug in package_target_resolve that caused InvalidPackageTarget errors to be silently dropped when all targets in a fallback array are invalid. The addition of Debug implementations for CachedPath and CachedPathImpl is a nice improvement for developer experience, and the new test case effectively verifies the bug fix. However, I've identified a potential issue in the new error handling logic that could swallow other important errors, which I've detailed in a comment.
| match resolved { | ||
| // Only track InvalidPackageTarget for re-throwing after the loop. | ||
| Err(e @ ResolveError::InvalidPackageTarget(..)) => { | ||
| last_error = Some(e); | ||
| } | ||
| Err(_) => {} | ||
| // 2. If resolved is undefined, continue the loop. | ||
| Ok(None) => { | ||
| last_error = None; | ||
| } | ||
| // 3. Return resolved. | ||
| return Ok(Some(path)); | ||
| Ok(Some(path)) => return Ok(Some(path)), | ||
| } |
There was a problem hiding this comment.
While this change correctly handles InvalidPackageTarget errors in a fallback array, the Err(_) => {} case will silently swallow any other type of ResolveError. This could hide other resolution issues. According to the Node.js spec and enhanced-resolve's behavior, only InvalidPackageTarget errors should allow the loop to continue. Other errors should be propagated immediately.
match resolved {
// Only track InvalidPackageTarget for re-throwing after the loop.
Err(e @ ResolveError::InvalidPackageTarget(..)) => {
last_error = Some(e);
}
// Other errors should be propagated immediately.
Err(e) => return Err(e),
// 2. If resolved is undefined, continue the loop.
Ok(None) => {
last_error = None;
}
// 3. Return resolved.
Ok(Some(path)) => return Ok(Some(path)),
}There was a problem hiding this comment.
Pull request overview
Fixes exports-field fallback array resolution to correctly propagate InvalidPackageTarget when all fallback targets are invalid, aligning behavior with the Node.js resolution algorithm. Also improves debuggability of cached paths.
Changes:
- Fix fallback-array error propagation in
package_target_resolve(avoid dropping the finalInvalidPackageTarget). - Add a regression test covering “all fallback targets invalid” behavior.
- Implement/derive
DebugforCachedPathImpl/CachedPathto print the underlying path.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/lib.rs | Adjusts array-fallback logic in package_target_resolve to track and rethrow the last InvalidPackageTarget. |
| src/tests/exports_field.rs | Adds regression test ensuring invalid fallback arrays propagate InvalidPackageTarget. |
| src/cache.rs | Adds Debug output for cached paths (prints the path). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| Err(e @ ResolveError::InvalidPackageTarget(..)) => { | ||
| last_error = Some(e); | ||
| } | ||
| Err(_) => {} |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f35bd7475
ℹ️ 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Err(e @ ResolveError::InvalidPackageTarget(..)) => { | ||
| last_error = Some(e); | ||
| } | ||
| Err(_) => {} |
There was a problem hiding this comment.
This Err(_) => {} arm drops later fallback errors even though the array algorithm is only supposed to continue on InvalidPackageTarget. A config like {"./foo/": ["invalid", "./bar"]} resolving pkg/foo/x makes the second entry fail with InvalidPackageConfigDirectory in normalize_string_target, but this loop ignores that and rethrows the stale first InvalidPackageTarget from last_error. That changes the surfaced error away from the entry that actually failed and makes misconfigured fallback arrays much harder to diagnose.
Useful? React with 👍 / 👎.
Summary
package_target_resolve: the conditionresolved.is_err() && i == targets.len()was always false, causingInvalidPackageTargeterrors to be silently dropped when all targets in a fallback array are invalidDebugtrait forCachedPathImpl(displays its path) and deriveDebugforCachedPathTest plan
array_fallback_all_invalid_targets_should_return_invalid_package_targetto verify the fixcargo test --all-featurespassescargo clippy --all-featurespasses