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.

Sign up for GitHub

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

Jump to bottom

fix(check): attempt to resolve types from pkg before @types pkg #24152

Merged
merged 1 commit into from
Jun 9, 2024
Merged

fix(check): attempt to resolve types from pkg before @types pkg #24152

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
19 changes: 1 addition & 18 deletions cli/npm/byonm.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 12,6 @@ use deno_core::error::AnyError;
use deno_core::serde_json;
use deno_runtime::deno_fs::FileSystem;
use deno_runtime::deno_node::NodePermissions;
use deno_runtime::deno_node::NodeResolutionMode;
use deno_runtime::deno_node::NpmResolver;
use deno_runtime::deno_node::PackageJson;
use deno_semver::package::PackageReq;
Expand All @@ -23,7 22,6 @@ use crate::args::NpmProcessStateKind;
use crate::util::fs::canonicalize_path_maybe_not_exists_with_fs;
use deno_runtime::fs_util::specifier_to_file_path;

use super::common::types_package_name;
use super::CliNpmResolver;
use super::InnerCliNpmResolverRef;

Expand Down Expand Up @@ -97,20 95,13 @@ impl NpmResolver for ByonmCliNpmResolver {
&self,
name: &str,
referrer: &ModuleSpecifier,
mode: NodeResolutionMode,
) -> Result<PathBuf, AnyError> {
fn inner(
fs: &dyn FileSystem,
name: &str,
referrer: &ModuleSpecifier,
mode: NodeResolutionMode,
) -> Result<PathBuf, AnyError> {
let referrer_file = specifier_to_file_path(referrer)?;
let types_pkg_name = if mode.is_types() && !name.starts_with("@types/") {
Some(types_package_name(name))
} else {
None
};
let mut current_folder = referrer_file.parent().unwrap();
loop {
let node_modules_folder = if current_folder.ends_with("node_modules") {
Expand All @@ -119,14 110,6 @@ impl NpmResolver for ByonmCliNpmResolver {
Cow::Owned(current_folder.join("node_modules"))
};

// attempt to resolve the types package first, then fallback to the regular package
if let Some(types_pkg_name) = &types_pkg_name {
let sub_dir = join_package_name(&node_modules_folder, types_pkg_name);
if fs.is_dir_sync(&sub_dir) {
return Ok(sub_dir);
}
}

let sub_dir = join_package_name(&node_modules_folder, name);
if fs.is_dir_sync(&sub_dir) {
return Ok(sub_dir);
Expand All @@ -146,7 129,7 @@ impl NpmResolver for ByonmCliNpmResolver {
);
}

let path = inner(&*self.fs, name, referrer, mode)?;
let path = inner(&*self.fs, name, referrer)?;
Ok(self.fs.realpath_sync(&path)?)
}

Expand Down
22 changes: 0 additions & 22 deletions cli/npm/common.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 3,6 @@
use deno_npm::npm_rc::RegistryConfig;
use reqwest::header;

/// Gets the corresponding @types package for the provided package name.
pub fn types_package_name(package_name: &str) -> String {
debug_assert!(!package_name.starts_with("@types/"));
// Scoped packages will get two underscores for each slash
// https://github.com/DefinitelyTyped/DefinitelyTyped/tree/15f1ece08f7b498f4b9a2147c2a46e94416ca777#what-about-scoped-packages
format!("@types/{}", package_name.replace('/', "__"))
}

// TODO(bartlomieju): support more auth methods besides token and basic auth
pub fn maybe_auth_header_for_npm_registry(
registry_config: &RegistryConfig,
Expand All @@ -31,17 23,3 @@ pub fn maybe_auth_header_for_npm_registry(

None
}

#[cfg(test)]
mod test {
use super::types_package_name;

#[test]
fn test_types_package_name() {
assert_eq!(types_package_name("name"), "@types/name");
assert_eq!(
types_package_name("@scoped/package"),
"@types/@scoped__package"
);
}
}
4 changes: 1 addition & 3 deletions cli/npm/managed/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 23,6 @@ use deno_npm::NpmResolutionPackage;
use deno_npm::NpmSystemInfo;
use deno_runtime::deno_fs::FileSystem;
use deno_runtime::deno_node::NodePermissions;
use deno_runtime::deno_node::NodeResolutionMode;
use deno_runtime::deno_node::NpmResolver;
use deno_semver::package::PackageNv;
use deno_semver::package::PackageReq;
Expand Down Expand Up @@ -531,11 530,10 @@ impl NpmResolver for ManagedCliNpmResolver {
&self,
name: &str,
referrer: &ModuleSpecifier,
mode: NodeResolutionMode,
) -> Result<PathBuf, AnyError> {
let path = self
.fs_resolver
.resolve_package_folder_from_package(name, referrer, mode)?;
.resolve_package_folder_from_package(name, referrer)?;
let path =
canonicalize_path_maybe_not_exists_with_fs(&path, self.fs.as_ref())?;
log::debug!("Resolved {} from {} to {}", name, referrer, path.display());
Expand Down
2 changes: 0 additions & 2 deletions cli/npm/managed/resolvers/common.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 19,6 @@ use deno_npm::NpmPackageId;
use deno_npm::NpmResolutionPackage;
use deno_runtime::deno_fs::FileSystem;
use deno_runtime::deno_node::NodePermissions;
use deno_runtime::deno_node::NodeResolutionMode;

use crate::npm::managed::cache::TarballCache;

Expand All @@ -41,7 40,6 @@ pub trait NpmPackageFsResolver: Send Sync {
&self,
name: &str,
referrer: &ModuleSpecifier,
mode: NodeResolutionMode,
) -> Result<PathBuf, AnyError>;

fn resolve_package_cache_folder_id_from_specifier(
Expand Down
32 changes: 3 additions & 29 deletions cli/npm/managed/resolvers/global.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 11,12 @@ use deno_ast::ModuleSpecifier;
use deno_core::anyhow::bail;
use deno_core::error::AnyError;
use deno_core::url::Url;
use deno_npm::resolution::PackageNotFoundFromReferrerError;
use deno_npm::NpmPackageCacheFolderId;
use deno_npm::NpmPackageId;
use deno_npm::NpmResolutionPackage;
use deno_npm::NpmSystemInfo;
use deno_runtime::deno_fs::FileSystem;
use deno_runtime::deno_node::NodePermissions;
use deno_runtime::deno_node::NodeResolutionMode;

use super::super::super::common::types_package_name;
use super::super::cache::NpmCache;
use super::super::cache::TarballCache;
use super::super::resolution::NpmResolution;
Expand Down Expand Up @@ -57,17 53,6 @@ impl GlobalNpmPackageResolver {
system_info,
}
}

fn resolve_types_package(
&self,
package_name: &str,
referrer_pkg_id: &NpmPackageCacheFolderId,
) -> Result<NpmResolutionPackage, Box<PackageNotFoundFromReferrerError>> {
let types_name = types_package_name(package_name);
self
.resolution
.resolve_package_from_package(&types_name, referrer_pkg_id)
}
}

#[async_trait(?Send)]
Expand All @@ -92,27 77,16 @@ impl NpmPackageFsResolver for GlobalNpmPackageResolver {
&self,
name: &str,
referrer: &ModuleSpecifier,
mode: NodeResolutionMode,
) -> Result<PathBuf, AnyError> {
let Some(referrer_pkg_id) = self
.cache
.resolve_package_folder_id_from_specifier(referrer)
else {
bail!("could not find npm package for '{}'", referrer);
};
let pkg = if mode.is_types() && !name.starts_with("@types/") {
// attempt to resolve the types package first, then fallback to the regular package
match self.resolve_types_package(name, &referrer_pkg_id) {
Ok(pkg) => pkg,
Err(_) => self
.resolution
.resolve_package_from_package(name, &referrer_pkg_id)?,
}
} else {
self
.resolution
.resolve_package_from_package(name, &referrer_pkg_id)?
};
let pkg = self
.resolution
.resolve_package_from_package(name, &referrer_pkg_id)?;
self.package_folder(&pkg.id)
}

Expand Down
12 changes: 0 additions & 12 deletions cli/npm/managed/resolvers/local.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 39,12 @@ use deno_npm::NpmResolutionPackage;
use deno_npm::NpmSystemInfo;
use deno_runtime::deno_fs;
use deno_runtime::deno_node::NodePermissions;
use deno_runtime::deno_node::NodeResolutionMode;
use deno_semver::package::PackageNv;
use serde::Deserialize;
use serde::Serialize;

use crate::npm::cache_dir::mixed_case_package_name_encode;

use super::super::super::common::types_package_name;
use super::super::cache::NpmCache;
use super::super::cache::TarballCache;
use super::super::resolution::NpmResolution;
Expand Down Expand Up @@ -175,7 173,6 @@ impl NpmPackageFsResolver for LocalNpmPackageResolver {
&self,
name: &str,
referrer: &ModuleSpecifier,
mode: NodeResolutionMode,
) -> Result<PathBuf, AnyError> {
let Some(local_path) = self.resolve_folder_for_specifier(referrer)? else {
bail!("could not find npm package for '{}'", referrer);
Expand All @@ -190,15 187,6 @@ impl NpmPackageFsResolver for LocalNpmPackageResolver {
Cow::Owned(current_folder.join("node_modules"))
};

// attempt to resolve the types package first, then fallback to the regular package
if mode.is_types() && !name.starts_with("@types/") {
let sub_dir =
join_package_name(&node_modules_folder, &types_package_name(name));
if self.fs.is_dir_sync(&sub_dir) {
return Ok(sub_dir);
}
}

let sub_dir = join_package_name(&node_modules_folder, name);
if self.fs.is_dir_sync(&sub_dir) {
return Ok(sub_dir);
Expand Down
12 changes: 2 additions & 10 deletions cli/resolver.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,14 509,11 @@ impl CliGraphResolver {
&self,
specifier: &str,
referrer: &ModuleSpecifier,
mode: NodeResolutionMode,
original_err: AnyError,
resolver: &ByonmCliNpmResolver,
) -> Result<(), AnyError> {
if let Ok((pkg_name, _, _)) = parse_npm_pkg_name(specifier, referrer) {
match resolver
.resolve_package_folder_from_package(&pkg_name, referrer, mode)
{
match resolver.resolve_package_folder_from_package(&pkg_name, referrer) {
Ok(_) => {
return Err(original_err);
}
Expand Down Expand Up @@ -645,7 642,6 @@ impl Resolver for CliGraphResolver {
.check_surface_byonm_node_error(
specifier,
referrer,
to_node_mode(mode),
anyhow!("Cannot find \"{}\"", specifier),
resolver,
)
Expand All @@ -654,11 650,7 @@ impl Resolver for CliGraphResolver {
Err(err) => {
self
.check_surface_byonm_node_error(
specifier,
referrer,
to_node_mode(mode),
err,
resolver,
specifier, referrer, err, resolver,
)
.map_err(ResolveError::Other)?;
}
Expand Down
1 change: 0 additions & 1 deletion ext/node/analyze.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 307,6 @@ impl<TCjsCodeAnalyzer: CjsCodeAnalyzer> NodeCodeTranslator<TCjsCodeAnalyzer> {
let module_dir = self.npm_resolver.resolve_package_folder_from_package(
package_specifier.as_str(),
referrer,
mode,
)?;

let package_json_path = module_dir.join("package.json");
Expand Down
1 change: 0 additions & 1 deletion ext/node/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 162,6 @@ pub trait NpmResolver: std::fmt::Debug MaybeSend MaybeSync {
&self,
specifier: &str,
referrer: &ModuleSpecifier,
mode: NodeResolutionMode,
) -> Result<PathBuf, AnyError>;

fn in_npm_package(&self, specifier: &ModuleSpecifier) -> bool;
Expand Down
1 change: 0 additions & 1 deletion ext/node/ops/require.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 198,6 @@ pub fn op_require_resolve_deno_dir(
&ModuleSpecifier::from_file_path(&parent_filename).unwrap_or_else(|_| {
panic!("Url::from_file_path: [{:?}]", parent_filename)
}),
NodeResolutionMode::Execution,
)
.ok()
.map(|p| p.to_string_lossy().to_string())
Expand Down
57 changes: 55 additions & 2 deletions ext/node/resolution.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1037,9 1037,45 @@ impl NodeResolver {
}
}

let result = self.resolve_package_subpath_for_package(
&package_name,
&package_subpath,
referrer,
referrer_kind,
conditions,
mode,
);
if mode.is_types() && !matches!(result, Ok(Some(_))) {
// try to resolve with the @types/node package
let package_name = types_package_name(&package_name);
if let Ok(Some(result)) = self.resolve_package_subpath_for_package(
&package_name,
&package_subpath,
referrer,
referrer_kind,
conditions,
mode,
) {
return Ok(Some(result));
}
}

result
}

#[allow(clippy::too_many_arguments)]
fn resolve_package_subpath_for_package(
&self,
package_name: &str,
package_subpath: &str,
referrer: &ModuleSpecifier,
referrer_kind: NodeModuleKind,
conditions: &[&str],
mode: NodeResolutionMode,
) -> Result<Option<ModuleSpecifier>, AnyError> {
let package_dir_path = self
.npm_resolver
.resolve_package_folder_from_package(&package_name, referrer, mode)?;
.resolve_package_folder_from_package(package_name, referrer)?;
let package_json_path = package_dir_path.join("package.json");

// todo: error with this instead when can't find package
Expand All @@ -1060,7 1096,7 @@ impl NodeResolver {
.load_package_json(&mut AllowAllNodePermissions, package_json_path)?;
self.resolve_package_subpath(
&package_json,
&package_subpath,
package_subpath,
referrer,
referrer_kind,
conditions,
Expand Down Expand Up @@ -1600,6 1636,14 @@ fn pattern_key_compare(a: &str, b: &str) -> i32 {
0
}

/// Gets the corresponding @types package for the provided package name.
fn types_package_name(package_name: &str) -> String {
debug_assert!(!package_name.starts_with("@types/"));
// Scoped packages will get two underscores for each slash
// https://github.com/DefinitelyTyped/DefinitelyTyped/tree/15f1ece08f7b498f4b9a2147c2a46e94416ca777#what-about-scoped-packages
format!("@types/{}", package_name.replace('/', "__"))
}

#[cfg(test)]
mod tests {
use deno_core::serde_json::json;
Expand Down Expand Up @@ -1780,4 1824,13 @@ mod tests {
assert_eq!(actual.to_string_lossy(), *expected);
}
}

#[test]
fn test_types_package_name() {
assert_eq!(types_package_name("name"), "@types/name");
assert_eq!(
types_package_name("@scoped/package"),
"@types/@scoped__package"
);
}
}
7 changes: 7 additions & 0 deletions tests/specs/npm/check_prefers_non_types_node_pkg/__test__.jsonc
View file
Open in desktop
Copy link
Member Author

Choose a reason for hiding this comment

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

Main:

---- specs::npm::check_prefers_non_types_node_pkg ----
command V:\deno\target\debug\deno.exe check --quiet main.ts
command cwd V:\deno\tests\specs\npm\check_prefers_non_types_node_pkg

panicked at tests\specs/mod.rs:393:12:
assertion failed: `(left == right)`

Diff < left / right > :
<error: Failed resolving types. Cannot find "lz-string"
<    at file:///V:/deno/tests/specs/npm/check_prefers_non_types_node_pkg/main.ts:1:47
<

Original file line number Diff line number Diff line change
@@ -0,0 1,7 @@
{
"envs": {
"DENO_FUTURE": "1"
},
"args": "check --quiet main.ts",
"output": ""
}
3 changes: 3 additions & 0 deletions tests/specs/npm/check_prefers_non_types_node_pkg/main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 1,3 @@
import { compressToEncodedURIComponent } from "lz-string";

console.log(compressToEncodedURIComponent("Hello, World!"));
Loading