Skip to content

Commit f64911c

Browse files
authored
refactor!: remove modules options (oxc-project#484)
The only value for this option in the ecosystem is node_modules. This option also slightly impacts performance.
1 parent 74de9cf commit f64911c

13 files changed

Lines changed: 224 additions & 333 deletions

File tree

README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -179,7 +179,6 @@ See [index.d.ts](https://github.com/oxc-project/oxc-resolver/blob/main/napi/inde
179179
| fullySpecified | false | Request passed to resolve is already fully specified and extensions or main files are not resolved for it (they are still resolved for internal requests) |
180180
| mainFields | ["main"] | A list of main fields in description files |
181181
| mainFiles | ["index"] | A list of main files in directories |
182-
| modules | ["node_modules"] | A list of directories to resolve modules from, can be absolute path or folder name |
183182
| resolveToContext | false | Resolve to a context instead of a file |
184183
| preferRelative | false | Prefer to resolve module requests as relative request and fallback to resolving as module |
185184
| preferAbsolute | false | Prefer to resolve server-relative urls as absolute paths before falling back to resolve in roots |
@@ -191,6 +190,7 @@ See [index.d.ts](https://github.com/oxc-project/oxc-resolver/blob/main/napi/inde
191190

192191
| Field | Default | Description |
193192
| ---------------- | --------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------- |
193+
| modules | ["node_modules"] | A list of directories to resolve modules from, can be absolute path or folder name |
194194
| cachePredicate | function() { return true }; | A function which decides whether a request should be cached or not. An object is passed to the function with `path` and `request` properties. |
195195
| cacheWithContext | true | If unsafe cache is enabled, includes `request.context` in the cache key |
196196
| plugins | [] | A list of additional resolve plugins which should be applied |

napi/index.d.ts

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -131,12 +131,6 @@ export interface NapiResolveOptions {
131131
* Default `["index"]`
132132
*/
133133
mainFiles?: Array<string>
134-
/**
135-
* A list of directories to resolve modules from, can be absolute path or folder name.
136-
*
137-
* Default `["node_modules"]`
138-
*/
139-
modules?: string | string[]
140134
/**
141135
* Resolve to a context instead of a file.
142136
*

napi/src/lib.rs

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -198,7 +198,6 @@ impl ResolverFactory {
198198
.map(|o| StrOrStrList(o).into())
199199
.unwrap_or(default.main_fields),
200200
main_files: op.main_files.unwrap_or(default.main_files),
201-
modules: op.modules.map(|o| StrOrStrList(o).into()).unwrap_or(default.modules),
202201
resolve_to_context: op.resolve_to_context.unwrap_or(default.resolve_to_context),
203202
prefer_relative: op.prefer_relative.unwrap_or(default.prefer_relative),
204203
prefer_absolute: op.prefer_absolute.unwrap_or(default.prefer_absolute),

napi/src/options.rs

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -105,12 +105,6 @@ pub struct NapiResolveOptions {
105105
/// Default `["index"]`
106106
pub main_files: Option<Vec<String>>,
107107

108-
/// A list of directories to resolve modules from, can be absolute path or folder name.
109-
///
110-
/// Default `["node_modules"]`
111-
#[napi(ts_type = "string | string[]")]
112-
pub modules: Option<StrOrStrListType>,
113-
114108
/// Resolve to a context instead of a file.
115109
///
116110
/// Default `false`

src/cache.rs

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -62,17 +62,6 @@ pub trait CachedPath: Sized {
6262

6363
fn parent(&self) -> Option<&Self>;
6464

65-
fn node_modules(&self) -> Option<&Self>;
66-
67-
fn module_directory<C: Cache<Cp = Self>>(
68-
&self,
69-
module_name: &str,
70-
cache: &C,
71-
ctx: &mut Ctx,
72-
) -> Option<Self>;
73-
74-
fn cached_node_modules<C: Cache<Cp = Self>>(&self, cache: &C, ctx: &mut Ctx) -> Option<Self>;
75-
7665
/// Find package.json of a path by traversing parent directories.
7766
#[allow(clippy::type_complexity)]
7867
fn find_package_json<C: Cache<Cp = Self>>(

src/fs_cache.rs

Lines changed: 0 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -265,7 +265,6 @@ pub struct CachedPathImpl {
265265
meta: OnceLock<Option<FileMetadata>>,
266266
canonicalized: OnceLock<Result<FsCachedPath, ResolveError>>,
267267
canonicalizing: AtomicU64,
268-
node_modules: OnceLock<Option<FsCachedPath>>,
269268
package_json: OnceLock<Option<(FsCachedPath, Arc<PackageJsonSerde>)>>,
270269
}
271270

@@ -278,7 +277,6 @@ impl CachedPathImpl {
278277
meta: OnceLock::new(),
279278
canonicalized: OnceLock::new(),
280279
canonicalizing: AtomicU64::new(0),
281-
node_modules: OnceLock::new(),
282280
package_json: OnceLock::new(),
283281
}
284282
}
@@ -305,24 +303,6 @@ impl CachedPath for FsCachedPath {
305303
self.0.parent.as_ref()
306304
}
307305

308-
fn module_directory<C: Cache<Cp = Self>>(
309-
&self,
310-
module_name: &str,
311-
cache: &C,
312-
ctx: &mut Ctx,
313-
) -> Option<Self> {
314-
let cached_path = cache.value(&self.path.join(module_name));
315-
cache.is_dir(&cached_path, ctx).then_some(cached_path)
316-
}
317-
318-
fn node_modules(&self) -> Option<&Self> {
319-
self.node_modules.get().and_then(|o| o.as_ref())
320-
}
321-
322-
fn cached_node_modules<C: Cache<Cp = Self>>(&self, cache: &C, ctx: &mut Ctx) -> Option<Self> {
323-
self.node_modules.get_or_init(|| self.module_directory("node_modules", cache, ctx)).clone()
324-
}
325-
326306
/// Find package.json of a path by traversing parent directories.
327307
///
328308
/// # Errors

src/lib.rs

Lines changed: 86 additions & 111 deletions
Original file line numberDiff line numberDiff line change
@@ -770,73 +770,67 @@ impl<C: Cache> ResolverGeneric<C> {
770770

771771
// 1. let DIRS = NODE_MODULES_PATHS(START)
772772
// 2. for each DIR in DIRS:
773-
for module_name in &self.options.modules {
774-
for cached_path in std::iter::successors(Some(cached_path), |p| p.parent()) {
775-
// Skip if /path/to/node_modules does not exist
776-
if !self.cache.is_dir(cached_path, ctx) {
777-
continue;
778-
}
779-
780-
let Some(cached_path) = self.get_module_directory(cached_path, module_name, ctx)
781-
else {
782-
continue;
783-
};
784-
// Optimize node_modules lookup by inspecting whether the package exists
785-
// From LOAD_PACKAGE_EXPORTS(X, DIR)
786-
// 1. Try to interpret X as a combination of NAME and SUBPATH where the name
787-
// may have a @scope/ prefix and the subpath begins with a slash (`/`).
788-
if !package_name.is_empty() {
789-
let cached_path = cached_path.normalize_with(package_name, self.cache.as_ref());
790-
// Try foo/node_modules/package_name
791-
if self.cache.is_dir(&cached_path, ctx) {
792-
// a. LOAD_PACKAGE_EXPORTS(X, DIR)
793-
if let Some(path) =
794-
self.load_package_exports(specifier, subpath, &cached_path, ctx)?
795-
{
796-
return Ok(Some(path));
797-
}
798-
} else {
799-
// foo/node_modules/package_name is not a directory, so useless to check inside it
800-
if !subpath.is_empty() {
801-
continue;
802-
}
803-
// Skip if the directory lead to the scope package does not exist
804-
// i.e. `foo/node_modules/@scope` is not a directory for `foo/node_modules/@scope/package`
805-
if package_name.starts_with('@') {
806-
if let Some(path) = cached_path.parent() {
807-
if !self.cache.is_dir(path, ctx) {
808-
continue;
809-
}
773+
for cached_path in std::iter::successors(Some(cached_path), |p| p.parent()) {
774+
// Skip if /path/to/node_modules does not exist
775+
let cached_path = cached_path.normalize_with("node_modules", self.cache.as_ref());
776+
if !self.cache.is_dir(&cached_path, ctx) {
777+
continue;
778+
}
779+
// Optimize node_modules lookup by inspecting whether the package exists
780+
// From LOAD_PACKAGE_EXPORTS(X, DIR)
781+
// 1. Try to interpret X as a combination of NAME and SUBPATH where the name
782+
// may have a @scope/ prefix and the subpath begins with a slash (`/`).
783+
if !package_name.is_empty() {
784+
let cached_path = cached_path.normalize_with(package_name, self.cache.as_ref());
785+
// Try foo/node_modules/package_name
786+
if self.cache.is_dir(&cached_path, ctx) {
787+
// a. LOAD_PACKAGE_EXPORTS(X, DIR)
788+
if let Some(path) =
789+
self.load_package_exports(specifier, subpath, &cached_path, ctx)?
790+
{
791+
return Ok(Some(path));
792+
}
793+
} else {
794+
// foo/node_modules/package_name is not a directory, so useless to check inside it
795+
if !subpath.is_empty() {
796+
continue;
797+
}
798+
// Skip if the directory lead to the scope package does not exist
799+
// i.e. `foo/node_modules/@scope` is not a directory for `foo/node_modules/@scope/package`
800+
if package_name.starts_with('@') {
801+
if let Some(path) = cached_path.parent() {
802+
if !self.cache.is_dir(path, ctx) {
803+
continue;
810804
}
811805
}
812806
}
813807
}
808+
}
814809

815-
// Try as file or directory for all other cases
816-
// b. LOAD_AS_FILE(DIR/X)
817-
// c. LOAD_AS_DIRECTORY(DIR/X)
810+
// Try as file or directory for all other cases
811+
// b. LOAD_AS_FILE(DIR/X)
812+
// c. LOAD_AS_DIRECTORY(DIR/X)
818813

819-
let cached_path = cached_path.normalize_with(specifier, self.cache.as_ref());
814+
let cached_path = cached_path.normalize_with(specifier, self.cache.as_ref());
820815

821-
// Perf: try the directory first for package specifiers.
822-
if self.options.resolve_to_context {
823-
return Ok(self.cache.is_dir(&cached_path, ctx).then(|| cached_path.clone()));
824-
}
825-
if self.cache.is_dir(&cached_path, ctx) {
826-
if let Some(path) = self.load_browser_field_or_alias(&cached_path, ctx)? {
827-
return Ok(Some(path));
828-
}
829-
if let Some(path) = self.load_as_directory(&cached_path, ctx)? {
830-
return Ok(Some(path));
831-
}
832-
}
833-
if let Some(path) = self.load_as_file(&cached_path, ctx)? {
816+
// Perf: try the directory first for package specifiers.
817+
if self.options.resolve_to_context {
818+
return Ok(self.cache.is_dir(&cached_path, ctx).then(|| cached_path.clone()));
819+
}
820+
if self.cache.is_dir(&cached_path, ctx) {
821+
if let Some(path) = self.load_browser_field_or_alias(&cached_path, ctx)? {
834822
return Ok(Some(path));
835823
}
836824
if let Some(path) = self.load_as_directory(&cached_path, ctx)? {
837825
return Ok(Some(path));
838826
}
839827
}
828+
if let Some(path) = self.load_as_file(&cached_path, ctx)? {
829+
return Ok(Some(path));
830+
}
831+
if let Some(path) = self.load_as_directory(&cached_path, ctx)? {
832+
return Ok(Some(path));
833+
}
840834
}
841835
Ok(None)
842836
}
@@ -880,23 +874,6 @@ impl<C: Cache> ResolverGeneric<C> {
880874
}
881875
}
882876

883-
fn get_module_directory(
884-
&self,
885-
cached_path: &C::Cp,
886-
module_name: &str,
887-
ctx: &mut Ctx,
888-
) -> Option<C::Cp> {
889-
if module_name == "node_modules" {
890-
cached_path.cached_node_modules(self.cache.as_ref(), ctx)
891-
} else if cached_path.path().components().next_back()
892-
== Some(Component::Normal(OsStr::new(module_name)))
893-
{
894-
Some(cached_path.clone())
895-
} else {
896-
cached_path.module_directory(module_name, self.cache.as_ref(), ctx)
897-
}
898-
}
899-
900877
fn load_package_exports(
901878
&self,
902879
specifier: &str,
@@ -1339,51 +1316,49 @@ impl<C: Cache> ResolverGeneric<C> {
13391316
self.require_core(package_name)?;
13401317

13411318
// 11. While parentURL is not the file system root,
1342-
for module_name in &self.options.modules {
1343-
for cached_path in std::iter::successors(Some(cached_path), |p| p.parent()) {
1344-
// 1. Let packageURL be the URL resolution of "node_modules/" concatenated with packageSpecifier, relative to parentURL.
1345-
let Some(cached_path) = self.get_module_directory(cached_path, module_name, ctx)
1346-
else {
1347-
continue;
1348-
};
1349-
// 2. Set parentURL to the parent folder URL of parentURL.
1350-
let cached_path = cached_path.normalize_with(package_name, self.cache.as_ref());
1351-
// 3. If the folder at packageURL does not exist, then
1352-
// 1. Continue the next loop iteration.
1353-
if self.cache.is_dir(&cached_path, ctx) {
1354-
// 4. Let pjson be the result of READ_PACKAGE_JSON(packageURL).
1355-
if let Some((_, package_json)) =
1356-
self.cache.get_package_json(&cached_path, &self.options, ctx)?
1357-
{
1358-
// 5. If pjson is not null and pjson.exports is not null or undefined, then
1359-
// 1. Return the result of PACKAGE_EXPORTS_RESOLVE(packageURL, packageSubpath, pjson.exports, defaultConditions).
1360-
for exports in package_json.exports_fields(&self.options.exports_fields) {
1361-
if let Some(path) = self.package_exports_resolve(
1362-
&cached_path,
1363-
&format!(".{subpath}"),
1364-
&exports,
1365-
ctx,
1366-
)? {
1367-
return Ok(Some(path));
1368-
}
1319+
for cached_path in std::iter::successors(Some(cached_path), |p| p.parent()) {
1320+
// 1. Let packageURL be the URL resolution of "node_modules/" concatenated with packageSpecifier, relative to parentURL.
1321+
let cached_path = cached_path.normalize_with("node_modules", self.cache.as_ref());
1322+
if !self.cache.is_dir(&cached_path, ctx) {
1323+
continue;
1324+
}
1325+
// 2. Set parentURL to the parent folder URL of parentURL.
1326+
let cached_path = cached_path.normalize_with(package_name, self.cache.as_ref());
1327+
// 3. If the folder at packageURL does not exist, then
1328+
// 1. Continue the next loop iteration.
1329+
if self.cache.is_dir(&cached_path, ctx) {
1330+
// 4. Let pjson be the result of READ_PACKAGE_JSON(packageURL).
1331+
if let Some((_, package_json)) =
1332+
self.cache.get_package_json(&cached_path, &self.options, ctx)?
1333+
{
1334+
// 5. If pjson is not null and pjson.exports is not null or undefined, then
1335+
// 1. Return the result of PACKAGE_EXPORTS_RESOLVE(packageURL, packageSubpath, pjson.exports, defaultConditions).
1336+
for exports in package_json.exports_fields(&self.options.exports_fields) {
1337+
if let Some(path) = self.package_exports_resolve(
1338+
&cached_path,
1339+
&format!(".{subpath}"),
1340+
&exports,
1341+
ctx,
1342+
)? {
1343+
return Ok(Some(path));
13691344
}
1370-
// 6. Otherwise, if packageSubpath is equal to ".", then
1371-
if subpath == "." {
1372-
// 1. If pjson.main is a string, then
1373-
for main_field in package_json.main_fields(&self.options.main_fields) {
1374-
// 1. Return the URL resolution of main in packageURL.
1375-
let cached_path =
1376-
cached_path.normalize_with(main_field, self.cache.as_ref());
1377-
if self.cache.is_file(&cached_path, ctx) {
1378-
return Ok(Some(cached_path));
1379-
}
1345+
}
1346+
// 6. Otherwise, if packageSubpath is equal to ".", then
1347+
if subpath == "." {
1348+
// 1. If pjson.main is a string, then
1349+
for main_field in package_json.main_fields(&self.options.main_fields) {
1350+
// 1. Return the URL resolution of main in packageURL.
1351+
let cached_path =
1352+
cached_path.normalize_with(main_field, self.cache.as_ref());
1353+
if self.cache.is_file(&cached_path, ctx) {
1354+
return Ok(Some(cached_path));
13801355
}
13811356
}
13821357
}
1383-
let subpath = format!(".{subpath}");
1384-
ctx.with_fully_specified(false);
1385-
return self.require(&cached_path, &subpath, ctx).map(Some);
13861358
}
1359+
let subpath = format!(".{subpath}");
1360+
ctx.with_fully_specified(false);
1361+
return self.require(&cached_path, &subpath, ctx).map(Some);
13871362
}
13881363
}
13891364

0 commit comments

Comments
 (0)