Skip to content
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

fix: resolve "browser" field when "exports" is present #59

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
{
"exports": {
},
Comment on lines +2 to +3

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have quoted postcss to be the reason behind the change. And I agree that perhaps in their situation the browser field still should be used (not because I agree with it personally but because this is what webpack and others do). But postcss situation is somewhat different than what you have here in this test case.

I think it would make sense to:

  • do the browser-based resolution after resolving exports for relative import sources
  • always use the browser-based resolution for absolute/bare import sources

I didn't test the latter but I did test the first thing with webpack and I think it behaves like I've described. The in-package files are only resolved through the browser field (aka alias field) after a successful match is found in package.json#exports

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A useful mental model for this is that exports allow you to create arbitrary mappings - there is no real relation between the entry point and the filename. But the alias field is all about rewriting file paths - and that's kinda why it's allowed to be resolved after the actual file path gets resolve based on exports (although you could just encode everything in the exports using a browser condition and that would be way cleaner 🤷‍♂️ )

"browser": {
"./lib/ignore.js": false,
"./lib/replaced.js": "./lib/browser",
Expand Down
3 changes: 2 additions & 1 deletion fixtures/pnpm8/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
"packageManager": "pnpm@8.10.5",
"devDependencies": {
"axios": "1.6.2",
"styled-components": "6.1.1"
"styled-components": "6.1.1",
"postcss": "8.4.33"
}
}
9 changes: 6 additions & 3 deletions fixtures/pnpm8/pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion napi/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ test = false
doctest = false

[dependencies]
oxc_resolver = { path = "../../oxc_resolver" }
oxc_resolver = { path = ".." }
napi = { version = "2", default-features = false, features = ["napi3", "serde-json"] }
napi-derive = { version = "2" }

Expand Down
54 changes: 26 additions & 28 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -722,36 +722,34 @@ impl<Fs: FileSystem + Default> ResolverGeneric<Fs> {
return Ok(None);
};
// 3. If the SCOPE/package.json "exports" is null or undefined, return.
if package_json.exports.is_empty() {
return self.load_browser_field(cached_path, Some(specifier), &package_json, ctx);
}
// 4. If the SCOPE/package.json "name" is not the first segment of X, return.
let Some(subpath) = package_json
.name
.as_ref()
.and_then(|package_name| Self::strip_package_name(specifier, package_name))
else {
return Ok(None);
};
// 5. let MATCH = PACKAGE_EXPORTS_RESOLVE(pathToFileURL(SCOPE),
// "." + X.slice("name".length), `package.json` "exports", ["node", "require"])
// defined in the ESM resolver.
let package_url = package_json.directory();
// Note: The subpath is not prepended with a dot on purpose
// because `package_exports_resolve` matches subpath without the leading dot.
for exports in &package_json.exports {
if let Some(cached_path) = self.package_exports_resolve(
package_url,
subpath,
exports,
&self.options.condition_names,
ctx,
)? {
// 6. RESOLVE_ESM_MATCH(MATCH)
return self.resolve_esm_match(specifier, &cached_path, &package_json, ctx);
if !package_json.exports.is_empty() {
// 4. If the SCOPE/package.json "name" is not the first segment of X, return.
if let Some(subpath) = package_json
.name
.as_ref()
.and_then(|package_name| Self::strip_package_name(specifier, package_name))
{
// 5. let MATCH = PACKAGE_EXPORTS_RESOLVE(pathToFileURL(SCOPE),
// "." + X.slice("name".length), `package.json` "exports", ["node", "require"])
// defined in the ESM resolver.
let package_url = package_json.directory();
// Note: The subpath is not prepended with a dot on purpose
// because `package_exports_resolve` matches subpath without the leading dot.
for exports in &package_json.exports {
if let Some(cached_path) = self.package_exports_resolve(
package_url,
subpath,
exports,
&self.options.condition_names,
ctx,
)? {
// 6. RESOLVE_ESM_MATCH(MATCH)
return self.resolve_esm_match(specifier, &cached_path, &package_json, ctx);
}
}
}
}
Ok(None)
self.load_browser_field(cached_path, Some(specifier), &package_json, ctx)
}

/// RESOLVE_ESM_MATCH(MATCH)
Expand Down
21 changes: 20 additions & 1 deletion tests/resolve_test.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use std::{env, path::PathBuf};

use oxc_resolver::{ResolveOptions, Resolver};
use oxc_resolver::{ResolveError, ResolveOptions, Resolver};

fn dir() -> PathBuf {
env::current_dir().unwrap()
Expand Down Expand Up @@ -74,3 +74,22 @@ fn axios() {
let resolution = Resolver::new(options).resolve(&path, specifier);
assert_eq!(resolution.map(|r| r.into_path_buf()), Ok(module_path.join("dist/node/axios.cjs")));
}

#[test]
fn postcss() {
let dir = dir();
let path = dir.join("fixtures/pnpm8");
let module_path = path.join("node_modules/postcss");
let resolver = Resolver::new(ResolveOptions {
alias_fields: vec![vec!["browser".into()]],
..ResolveOptions::default()
});

// should ignore "path"
let resolution = resolver.resolve(&module_path, "path");
assert_eq!(resolution, Err(ResolveError::Ignored(module_path.clone())));

// should ignore "./lib/terminal-highlight"
let resolution = resolver.resolve(&module_path, "./lib/terminal-highlight");
assert_eq!(resolution, Err(ResolveError::Ignored(module_path.join("lib/terminal-highlight"))));
}