TURBOPACK: Fix subpath imports resolving to dependencies#150
Conversation
Allow package.json subpath imports targets to resolve as bare package specifiers instead of always forcing relative paths. Add a snapshot fixture that verifies an imports entry can point to a dependency in node_modules.
There was a problem hiding this comment.
Code Review
This pull request updates Turbopack's resolution logic to support subpath imports by allowing both relative and bare specifier resolutions. It also exposes the ExportImport enum internally and adds a test case for subpath imports within node_modules. Feedback was provided regarding the use of Pattern::alternatives in handle_exports_imports_field, which may introduce ambiguity or deviate from the Node.js specification. The reviewer suggested a refactor to improve readability and ensure spec compliance.
| let request = | ||
| Pattern::Concatenation(vec![Pattern::Constant(rcstr!("./")), result_path.clone()]); | ||
| let request = match kind { | ||
| ExportImport::Export => request, | ||
| ExportImport::Import => Pattern::alternatives(vec![request, result_path.clone()]), | ||
| }; | ||
| let request = *Request::parse(request).to_resolved().await?; |
There was a problem hiding this comment.
Using Pattern::alternatives to try both a relative path (prefixed with ./) and the original path for subpath imports can lead to ambiguous resolution. According to the Node.js specification, subpath import targets are either relative (starting with ./ or ../) or bare package specifiers. Trying both might cause a bare specifier like "#dep": "my-pkg" to accidentally resolve to a local file ./my-pkg if it exists, which deviates from standard Node.js behavior.
If the intention is to strictly follow the spec, result_path.clone() should be sufficient for ExportImport::Import, as Request::parse will correctly handle the relativeness based on the prefix. If backward compatibility for missing ./ prefixes is required, the current approach is a safe fallback, but the code can be simplified to avoid redundant shadowing and improve readability.
let request = match kind {
ExportImport::Export => {
Pattern::Concatenation(vec![Pattern::Constant(rcstr!("./")), result_path.clone()])
}
ExportImport::Import => Pattern::alternatives(vec![
Pattern::Concatenation(vec![
Pattern::Constant(rcstr!("./")),
result_path.clone(),
]),
result_path.clone(),
]),
};
let request = *Request::parse(request).to_resolved().await?;
Summary
package.jsonsubpath imports targets to resolve as bare package specifiers instead of always forcing relative paths.importsentry can point to a dependency innode_modules.Upstream PR vercel#93308 is addressing the same issue, but there is no clear timeline on when it will be merged.
Test plan
cargo test -p turbopack-testssnapshot coverssubpath-importsresolving to anode_modulesdependency