Skip to content

refactor(npm): move InNpmPackageChecker code to deno_resolver #27609

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

Merged
merged 1 commit into from
Jan 9, 2025
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
9 changes: 4 additions & 5 deletions cli/factory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ use deno_core::futures::FutureExt;
use deno_core::FeatureChecker;
use deno_error::JsErrorBox;
use deno_resolver::cjs::IsCjsResolutionMode;
use deno_resolver::npm::managed::ManagedInNpmPkgCheckerCreateOptions;
use deno_resolver::npm::CreateInNpmPkgCheckerOptions;
use deno_resolver::npm::NpmReqResolverOptions;
use deno_resolver::DenoResolverOptions;
use deno_resolver::NodeAndNpmReqResolver;
Expand Down Expand Up @@ -64,14 +66,11 @@ use crate::node::CliNodeCodeTranslator;
use crate::node::CliNodeResolver;
use crate::node::CliPackageJsonResolver;
use crate::npm::create_cli_npm_resolver;
use crate::npm::create_in_npm_pkg_checker;
use crate::npm::CliByonmNpmResolverCreateOptions;
use crate::npm::CliManagedInNpmPkgCheckerCreateOptions;
use crate::npm::CliManagedNpmResolverCreateOptions;
use crate::npm::CliNpmResolver;
use crate::npm::CliNpmResolverCreateOptions;
use crate::npm::CliNpmResolverManagedSnapshotOption;
use crate::npm::CreateInNpmPkgCheckerOptions;
use crate::npm::NpmRegistryReadPermissionChecker;
use crate::npm::NpmRegistryReadPermissionCheckerMode;
use crate::resolver::CjsTracker;
Expand Down Expand Up @@ -387,15 +386,15 @@ impl CliFactory {
CreateInNpmPkgCheckerOptions::Byonm
} else {
CreateInNpmPkgCheckerOptions::Managed(
CliManagedInNpmPkgCheckerCreateOptions {
ManagedInNpmPkgCheckerCreateOptions {
root_cache_dir_url: self.npm_cache_dir()?.root_dir_url(),
maybe_node_modules_path: cli_options
.node_modules_dir_path()
.map(|p| p.as_path()),
},
)
};
Ok(create_in_npm_pkg_checker(options))
Ok(deno_resolver::npm::create_in_npm_pkg_checker(options))
})
}

Expand Down
8 changes: 4 additions & 4 deletions cli/lsp/resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ use deno_graph::Range;
use deno_npm::NpmSystemInfo;
use deno_path_util::url_to_file_path;
use deno_resolver::cjs::IsCjsResolutionMode;
use deno_resolver::npm::managed::ManagedInNpmPkgCheckerCreateOptions;
use deno_resolver::npm::CreateInNpmPkgCheckerOptions;
use deno_resolver::npm::NpmReqResolverOptions;
use deno_resolver::DenoResolverOptions;
use deno_resolver::NodeAndNpmReqResolver;
Expand Down Expand Up @@ -53,12 +55,10 @@ use crate::node::CliNodeResolver;
use crate::node::CliPackageJsonResolver;
use crate::npm::create_cli_npm_resolver_for_lsp;
use crate::npm::CliByonmNpmResolverCreateOptions;
use crate::npm::CliManagedInNpmPkgCheckerCreateOptions;
use crate::npm::CliManagedNpmResolverCreateOptions;
use crate::npm::CliNpmResolver;
use crate::npm::CliNpmResolverCreateOptions;
use crate::npm::CliNpmResolverManagedSnapshotOption;
use crate::npm::CreateInNpmPkgCheckerOptions;
use crate::npm::ManagedCliNpmResolver;
use crate::resolver::CliDenoResolver;
use crate::resolver::CliNpmReqResolver;
Expand Down Expand Up @@ -736,14 +736,14 @@ impl<'a> ResolverFactory<'a> {

pub fn in_npm_pkg_checker(&self) -> &Arc<dyn InNpmPackageChecker> {
self.services.in_npm_pkg_checker.get_or_init(|| {
crate::npm::create_in_npm_pkg_checker(
deno_resolver::npm::create_in_npm_pkg_checker(
match self.services.npm_resolver.as_ref().map(|r| r.as_inner()) {
Some(crate::npm::InnerCliNpmResolverRef::Byonm(_)) | None => {
CreateInNpmPkgCheckerOptions::Byonm
}
Some(crate::npm::InnerCliNpmResolverRef::Managed(m)) => {
CreateInNpmPkgCheckerOptions::Managed(
CliManagedInNpmPkgCheckerCreateOptions {
ManagedInNpmPkgCheckerCreateOptions {
root_cache_dir_url: m.global_cache_root_url(),
maybe_node_modules_path: m.maybe_node_modules_path(),
},
Expand Down
30 changes: 0 additions & 30 deletions cli/npm/managed/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ use installers::create_npm_fs_installer;
use installers::NpmPackageFsInstaller;
use node_resolver::errors::PackageFolderResolveError;
use node_resolver::errors::PackageFolderResolveIoError;
use node_resolver::InNpmPackageChecker;
use node_resolver::NpmPackageFolderResolver;

use super::CliNpmCache;
Expand Down Expand Up @@ -276,35 +275,6 @@ async fn snapshot_from_lockfile(
Ok(snapshot)
}

#[derive(Debug)]
struct ManagedInNpmPackageChecker {
root_dir: Url,
}

impl InNpmPackageChecker for ManagedInNpmPackageChecker {
fn in_npm_package(&self, specifier: &Url) -> bool {
specifier.as_ref().starts_with(self.root_dir.as_str())
}
}

pub struct CliManagedInNpmPkgCheckerCreateOptions<'a> {
pub root_cache_dir_url: &'a Url,
pub maybe_node_modules_path: Option<&'a Path>,
}

pub fn create_managed_in_npm_pkg_checker(
options: CliManagedInNpmPkgCheckerCreateOptions,
) -> Arc<dyn InNpmPackageChecker> {
let root_dir = match options.maybe_node_modules_path {
Some(node_modules_folder) => {
deno_path_util::url_from_directory_path(node_modules_folder).unwrap()
}
None => options.root_cache_dir_url.clone(),
};
debug_assert!(root_dir.as_str().ends_with('/'));
Arc::new(ManagedInNpmPackageChecker { root_dir })
}

#[derive(Debug, Clone, PartialEq, Eq)]
pub enum PackageCaching<'a> {
Only(Cow<'a, [PackageReq]>),
Expand Down
20 changes: 0 additions & 20 deletions cli/npm/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ use deno_core::url::Url;
use deno_error::JsErrorBox;
use deno_npm::npm_rc::ResolvedNpmRc;
use deno_npm::registry::NpmPackageInfo;
use deno_resolver::npm::ByonmInNpmPackageChecker;
use deno_resolver::npm::ByonmNpmResolver;
use deno_resolver::npm::CliNpmReqResolver;
use deno_resolver::npm::ResolvePkgFolderFromDenoReqError;
Expand All @@ -23,13 +22,10 @@ use deno_semver::package::PackageNv;
use deno_semver::package::PackageReq;
use http::HeaderName;
use http::HeaderValue;
use managed::create_managed_in_npm_pkg_checker;
use node_resolver::InNpmPackageChecker;
use node_resolver::NpmPackageFolderResolver;

pub use self::byonm::CliByonmNpmResolver;
pub use self::byonm::CliByonmNpmResolverCreateOptions;
pub use self::managed::CliManagedInNpmPkgCheckerCreateOptions;
pub use self::managed::CliManagedNpmResolverCreateOptions;
pub use self::managed::CliNpmResolverManagedSnapshotOption;
pub use self::managed::ManagedCliNpmResolver;
Expand Down Expand Up @@ -134,22 +130,6 @@ pub async fn create_cli_npm_resolver(
}
}

pub enum CreateInNpmPkgCheckerOptions<'a> {
Managed(CliManagedInNpmPkgCheckerCreateOptions<'a>),
Byonm,
}

pub fn create_in_npm_pkg_checker(
options: CreateInNpmPkgCheckerOptions,
) -> Arc<dyn InNpmPackageChecker> {
match options {
CreateInNpmPkgCheckerOptions::Managed(options) => {
create_managed_in_npm_pkg_checker(options)
}
CreateInNpmPkgCheckerOptions::Byonm => Arc::new(ByonmInNpmPackageChecker),
}
}

pub enum InnerCliNpmResolverRef<'a> {
Managed(&'a ManagedCliNpmResolver),
#[allow(dead_code)]
Expand Down
10 changes: 5 additions & 5 deletions cli/standalone/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,9 @@ use deno_error::JsErrorBox;
use deno_npm::npm_rc::ResolvedNpmRc;
use deno_package_json::PackageJsonDepValue;
use deno_resolver::cjs::IsCjsResolutionMode;
use deno_resolver::npm::create_in_npm_pkg_checker;
use deno_resolver::npm::managed::ManagedInNpmPkgCheckerCreateOptions;
use deno_resolver::npm::CreateInNpmPkgCheckerOptions;
use deno_resolver::npm::NpmReqResolverOptions;
use deno_runtime::deno_fs;
use deno_runtime::deno_fs::FileSystem;
Expand Down Expand Up @@ -81,14 +84,11 @@ use crate::node::CliNodeCodeTranslator;
use crate::node::CliNodeResolver;
use crate::node::CliPackageJsonResolver;
use crate::npm::create_cli_npm_resolver;
use crate::npm::create_in_npm_pkg_checker;
use crate::npm::CliByonmNpmResolverCreateOptions;
use crate::npm::CliManagedInNpmPkgCheckerCreateOptions;
use crate::npm::CliManagedNpmResolverCreateOptions;
use crate::npm::CliNpmResolver;
use crate::npm::CliNpmResolverCreateOptions;
use crate::npm::CliNpmResolverManagedSnapshotOption;
use crate::npm::CreateInNpmPkgCheckerOptions;
use crate::npm::NpmRegistryReadPermissionChecker;
use crate::npm::NpmRegistryReadPermissionCheckerMode;
use crate::resolver::CjsTracker;
Expand Down Expand Up @@ -738,7 +738,7 @@ pub async fn run(
.map(|node_modules_dir| root_path.join(node_modules_dir));
let in_npm_pkg_checker =
create_in_npm_pkg_checker(CreateInNpmPkgCheckerOptions::Managed(
CliManagedInNpmPkgCheckerCreateOptions {
ManagedInNpmPkgCheckerCreateOptions {
root_cache_dir_url: npm_cache_dir.root_dir_url(),
maybe_node_modules_path: maybe_node_modules_path.as_deref(),
},
Expand Down Expand Up @@ -796,7 +796,7 @@ pub async fn run(
));
let in_npm_pkg_checker =
create_in_npm_pkg_checker(CreateInNpmPkgCheckerOptions::Managed(
CliManagedInNpmPkgCheckerCreateOptions {
ManagedInNpmPkgCheckerCreateOptions {
root_cache_dir_url: npm_cache_dir.root_dir_url(),
maybe_node_modules_path: None,
},
Expand Down
6 changes: 3 additions & 3 deletions resolvers/deno/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ description = "Deno resolution algorithm"
path = "lib.rs"

[features]
sync = ["dashmap"]
sync = ["dashmap", "deno_package_json/sync", "node_resolver/sync"]

[dependencies]
anyhow.workspace = true
Expand All @@ -27,10 +27,10 @@ deno_config.workspace = true
deno_error.workspace = true
deno_media_type.workspace = true
deno_npm.workspace = true
deno_package_json = { workspace = true, features = ["sync"] }
deno_package_json.workspace = true
deno_path_util.workspace = true
deno_semver.workspace = true
node_resolver = { workspace = true, features = ["sync"] }
node_resolver.workspace = true
parking_lot.workspace = true
sys_traits.workspace = true
thiserror.workspace = true
Expand Down
5 changes: 4 additions & 1 deletion resolvers/deno/npm/managed/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,9 @@ use deno_npm::NpmPackageId;
use node_resolver::errors::PackageFolderResolveError;
use url::Url;

use crate::sync::MaybeSend;
use crate::sync::MaybeSync;

#[allow(clippy::disallowed_types)]
pub(super) type NpmPackageFsResolverRc =
crate::sync::MaybeArc<dyn NpmPackageFsResolver>;
Expand All @@ -20,7 +23,7 @@ pub struct NpmPackageFsResolverPackageFolderError(deno_semver::StackString);

/// Part of the resolution that interacts with the file system.
#[async_trait(?Send)]
pub trait NpmPackageFsResolver: Send + Sync {
pub trait NpmPackageFsResolver: MaybeSend + MaybeSync {
Copy link
Member Author

Choose a reason for hiding this comment

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

Also included some fixes to get this crate compiling when not using the sync feature.

Copy link
Member

Choose a reason for hiding this comment

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

So supersedes #27409?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh. Yes.

/// The local node_modules folder if it is applicable to the implementation.
fn node_modules_path(&self) -> Option<&Path>;

Expand Down
32 changes: 32 additions & 0 deletions resolvers/deno/npm/managed/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,13 @@ mod global;
mod local;
mod resolution;

use std::path::Path;
use std::path::PathBuf;

use node_resolver::InNpmPackageChecker;
use sys_traits::FsCanonicalize;
use sys_traits::FsMetadata;
use url::Url;

pub use self::common::NpmPackageFsResolver;
pub use self::common::NpmPackageFsResolverPackageFolderError;
Expand Down Expand Up @@ -43,3 +46,32 @@ pub fn create_npm_fs_resolver<
)),
}
}

#[derive(Debug)]
pub struct ManagedInNpmPackageChecker {
root_dir: Url,
}

impl InNpmPackageChecker for ManagedInNpmPackageChecker {
fn in_npm_package(&self, specifier: &Url) -> bool {
specifier.as_ref().starts_with(self.root_dir.as_str())
}
}

pub struct ManagedInNpmPkgCheckerCreateOptions<'a> {
pub root_cache_dir_url: &'a Url,
pub maybe_node_modules_path: Option<&'a Path>,
}

pub fn create_managed_in_npm_pkg_checker(
options: ManagedInNpmPkgCheckerCreateOptions,
) -> ManagedInNpmPackageChecker {
let root_dir = match options.maybe_node_modules_path {
Some(node_modules_folder) => {
deno_path_util::url_from_directory_path(node_modules_folder).unwrap()
}
None => options.root_cache_dir_url.clone(),
};
debug_assert!(root_dir.as_str().ends_with('/'));
ManagedInNpmPackageChecker { root_dir }
}
23 changes: 22 additions & 1 deletion resolvers/deno/npm/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,32 @@ pub use self::byonm::ByonmNpmResolverRc;
pub use self::byonm::ByonmResolvePkgFolderFromDenoReqError;
pub use self::local::get_package_folder_id_folder_name;
pub use self::local::normalize_pkg_name_for_node_modules_deno_folder;
use self::managed::create_managed_in_npm_pkg_checker;
use self::managed::ManagedInNpmPkgCheckerCreateOptions;
use crate::sync::new_rc;
use crate::sync::MaybeSend;
use crate::sync::MaybeSync;

mod byonm;
mod local;
pub mod managed;

pub enum CreateInNpmPkgCheckerOptions<'a> {
Managed(ManagedInNpmPkgCheckerCreateOptions<'a>),
Byonm,
}

pub fn create_in_npm_pkg_checker(
options: CreateInNpmPkgCheckerOptions,
) -> InNpmPackageCheckerRc {
match options {
CreateInNpmPkgCheckerOptions::Managed(options) => {
new_rc(create_managed_in_npm_pkg_checker(options))
}
CreateInNpmPkgCheckerOptions::Byonm => new_rc(ByonmInNpmPackageChecker),
}
}

#[derive(Debug, Error, JsError)]
#[class(generic)]
#[error("Could not resolve \"{}\", but found it in a package.json. Deno expects the node_modules/ directory to be up to date. Did you forget to run `deno install`?", specifier)]
Expand Down Expand Up @@ -99,7 +120,7 @@ pub type CliNpmReqResolverRc = crate::sync::MaybeArc<dyn CliNpmReqResolver>;

// todo(dsherret): a temporary trait until we extract
// out the CLI npm resolver into here
pub trait CliNpmReqResolver: Debug + Send + Sync {
pub trait CliNpmReqResolver: Debug + MaybeSend + MaybeSync {
fn resolve_pkg_folder_from_deno_module_req(
&self,
req: &PackageReq,
Expand Down
7 changes: 7 additions & 0 deletions resolvers/deno/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ pub use inner::*;
mod inner {
#![allow(clippy::disallowed_types)]

pub use core::marker::Send as MaybeSend;
pub use core::marker::Sync as MaybeSync;
pub use std::sync::Arc as MaybeArc;

pub use dashmap::DashMap as MaybeDashMap;
Expand All @@ -21,6 +23,11 @@ mod inner {
use std::hash::RandomState;
pub use std::rc::Rc as MaybeArc;

pub trait MaybeSync {}
impl<T> MaybeSync for T where T: ?Sized {}
pub trait MaybeSend {}
impl<T> MaybeSend for T where T: ?Sized {}

// Wrapper struct that exposes a subset of `DashMap` API.
#[derive(Debug)]
pub struct MaybeDashMap<K, V, S = RandomState>(RefCell<HashMap<K, V, S>>);
Expand Down
Loading