From 6233c0aff0dc9e58b02dfc9499048385bbf836c6 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Thu, 23 Feb 2023 12:33:23 -0500 Subject: [PATCH] fix(npm): support bare specifiers in package.json having a path (#17903) For example `import * as test from "package/path.js"` --- cli/args/mod.rs | 4 +- cli/args/package_json.rs | 11 ++- cli/lsp/documents.rs | 3 +- cli/npm/resolvers/mod.rs | 4 +- cli/resolver.rs | 89 ++++++++++++++++++- .../testdata/npm/node_modules_import/main.out | 1 + .../testdata/npm/node_modules_import/main.ts | 7 +- .../npm/node_modules_import/main_check.out | 11 ++- 8 files changed, 110 insertions(+), 20 deletions(-) diff --git a/cli/args/mod.rs b/cli/args/mod.rs index fe6883f2c7..8347a610cc 100644 --- a/cli/args/mod.rs +++ b/cli/args/mod.rs @@ -49,7 +49,7 @@ use deno_runtime::deno_tls::webpki_roots; use deno_runtime::inspector_server::InspectorServer; use deno_runtime::permissions::PermissionsOptions; use once_cell::sync::Lazy; -use std::collections::HashMap; +use std::collections::BTreeMap; use std::env; use std::io::BufReader; use std::io::Cursor; @@ -799,7 +799,7 @@ impl CliOptions { pub fn maybe_package_json_deps( &self, - ) -> Result>, AnyError> { + ) -> Result>, AnyError> { if matches!( self.flags.subcommand, DenoSubcommand::Task(TaskFlags { task: None, .. }) diff --git a/cli/args/package_json.rs b/cli/args/package_json.rs index 73bcfb5824..667918cd1b 100644 --- a/cli/args/package_json.rs +++ b/cli/args/package_json.rs @@ -1,5 +1,6 @@ // Copyright 2018-2023 the Deno authors. All rights reserved. MIT license. +use std::collections::BTreeMap; use std::collections::HashMap; use std::path::Path; use std::path::PathBuf; @@ -36,10 +37,10 @@ pub fn parse_dep_entry_name_and_raw_version<'a>( /// entries to npm specifiers which can then be used in the resolver. pub fn get_local_package_json_version_reqs( package_json: &PackageJson, -) -> Result, AnyError> { +) -> Result, AnyError> { fn insert_deps( deps: Option<&HashMap>, - result: &mut HashMap, + result: &mut BTreeMap, ) -> Result<(), AnyError> { if let Some(deps) = deps { for (key, value) in deps { @@ -73,9 +74,7 @@ pub fn get_local_package_json_version_reqs( let deps = package_json.dependencies.as_ref(); let dev_deps = package_json.dev_dependencies.as_ref(); - let mut result = HashMap::with_capacity( - deps.map(|d| d.len()).unwrap_or(0) + dev_deps.map(|d| d.len()).unwrap_or(0), - ); + let mut result = BTreeMap::new(); // insert the dev dependencies first so the dependencies will // take priority and overwrite any collisions @@ -166,7 +165,7 @@ mod test { let result = get_local_package_json_version_reqs(&package_json).unwrap(); assert_eq!( result, - HashMap::from([ + BTreeMap::from([ ( "test".to_string(), NpmPackageReq::from_str("test@^1.2").unwrap() diff --git a/cli/lsp/documents.rs b/cli/lsp/documents.rs index 98c3b715a6..88efc80749 100644 --- a/cli/lsp/documents.rs +++ b/cli/lsp/documents.rs @@ -1170,7 +1170,7 @@ impl Documents { fn calculate_resolver_config_hash( maybe_import_map: Option<&import_map::ImportMap>, maybe_jsx_config: Option<&JsxImportSourceConfig>, - maybe_package_json_deps: Option<&HashMap>, + maybe_package_json_deps: Option<&BTreeMap>, ) -> u64 { let mut hasher = FastInsecureHasher::default(); if let Some(import_map) = maybe_import_map { @@ -1181,7 +1181,6 @@ impl Documents { hasher.write_hashable(&jsx_config); } if let Some(deps) = maybe_package_json_deps { - let deps = deps.iter().collect::>(); hasher.write_hashable(&deps); } hasher.finish() diff --git a/cli/npm/resolvers/mod.rs b/cli/npm/resolvers/mod.rs index 583a1c955b..f68eeac265 100644 --- a/cli/npm/resolvers/mod.rs +++ b/cli/npm/resolvers/mod.rs @@ -19,7 +19,7 @@ use deno_runtime::deno_node::RequireNpmResolver; use global::GlobalNpmPackageResolver; use serde::Deserialize; use serde::Serialize; -use std::collections::HashMap; +use std::collections::BTreeMap; use std::path::Path; use std::path::PathBuf; use std::sync::Arc; @@ -227,7 +227,7 @@ impl NpmPackageResolver { /// Adds the package reqs from a package.json if they exist. pub async fn add_package_json_deps( &self, - maybe_package_json_deps: Option<&HashMap>, + maybe_package_json_deps: Option<&BTreeMap>, ) -> Result<(), AnyError> { if let Some(deps) = maybe_package_json_deps { let mut package_reqs = deps.values().cloned().collect::>(); diff --git a/cli/resolver.rs b/cli/resolver.rs index ce0d5d5cf7..db6ef0c8eb 100644 --- a/cli/resolver.rs +++ b/cli/resolver.rs @@ -14,7 +14,7 @@ use deno_graph::source::UnknownBuiltInNodeModuleError; use deno_graph::source::DEFAULT_JSX_IMPORT_SOURCE_MODULE; use deno_runtime::deno_node::is_builtin_node_module; use import_map::ImportMap; -use std::collections::HashMap; +use std::collections::BTreeMap; use std::sync::Arc; use crate::args::JsxImportSourceConfig; @@ -26,7 +26,7 @@ use crate::npm::NpmResolution; #[derive(Debug, Clone)] pub struct CliGraphResolver { maybe_import_map: Option>, - maybe_package_json_deps: Option>, + maybe_package_json_deps: Option>, maybe_default_jsx_import_source: Option, maybe_jsx_import_source_module: Option, no_npm: bool, @@ -62,7 +62,7 @@ impl CliGraphResolver { no_npm: bool, npm_registry_api: NpmRegistryApi, npm_resolution: NpmResolution, - maybe_package_json_deps: Option>, + maybe_package_json_deps: Option>, ) -> Self { Self { maybe_import_map, @@ -120,6 +120,9 @@ impl Resolver for CliGraphResolver { } if let Some(deps) = self.maybe_package_json_deps.as_ref() { + if let Some(specifier) = resolve_package_json_dep(specifier, deps)? { + return Ok(specifier); + } if let Some(req) = deps.get(specifier) { return Ok(ModuleSpecifier::parse(&format!("npm:{req}")).unwrap()); } @@ -129,6 +132,25 @@ impl Resolver for CliGraphResolver { } } +fn resolve_package_json_dep( + specifier: &str, + deps: &BTreeMap, +) -> Result, deno_core::url::ParseError> { + for (bare_specifier, req) in deps { + if specifier.starts_with(bare_specifier) { + if specifier.len() == bare_specifier.len() { + return ModuleSpecifier::parse(&format!("npm:{req}")).map(Some); + } + let path = &specifier[bare_specifier.len()..]; + if path.starts_with('/') { + return ModuleSpecifier::parse(&format!("npm:/{req}{path}")).map(Some); + } + } + } + + Ok(None) +} + impl NpmResolver for CliGraphResolver { fn resolve_builtin_node_module( &self, @@ -184,3 +206,64 @@ impl NpmResolver for CliGraphResolver { .resolve_package_req_for_deno_graph(package_req) } } + +#[cfg(test)] +mod test { + use super::*; + + #[test] + fn test_resolve_package_json_dep() { + fn resolve( + specifier: &str, + deps: &BTreeMap, + ) -> Result, String> { + resolve_package_json_dep(specifier, deps) + .map(|s| s.map(|s| s.to_string())) + .map_err(|err| err.to_string()) + } + + let deps = BTreeMap::from([ + ( + "package".to_string(), + NpmPackageReq::from_str("package@1.0").unwrap(), + ), + ( + "package-alias".to_string(), + NpmPackageReq::from_str("package@^1.2").unwrap(), + ), + ( + "@deno/test".to_string(), + NpmPackageReq::from_str("@deno/test@~0.2").unwrap(), + ), + ]); + + assert_eq!( + resolve("package", &deps).unwrap(), + Some("npm:package@1.0".to_string()), + ); + assert_eq!( + resolve("package/some_path.ts", &deps).unwrap(), + Some("npm:/package@1.0/some_path.ts".to_string()), + ); + + assert_eq!( + resolve("@deno/test", &deps).unwrap(), + Some("npm:@deno/test@~0.2".to_string()), + ); + assert_eq!( + resolve("@deno/test/some_path.ts", &deps).unwrap(), + Some("npm:/@deno/test@~0.2/some_path.ts".to_string()), + ); + // matches the start, but doesn't have the same length or a path + assert_eq!(resolve("@deno/testing", &deps).unwrap(), None,); + + // alias + assert_eq!( + resolve("package-alias", &deps).unwrap(), + Some("npm:package@^1.2".to_string()), + ); + + // non-existent bare specifier + assert_eq!(resolve("non-existent", &deps).unwrap(), None); + } +} diff --git a/cli/tests/testdata/npm/node_modules_import/main.out b/cli/tests/testdata/npm/node_modules_import/main.out index 51993f072d..083edaac24 100644 --- a/cli/tests/testdata/npm/node_modules_import/main.out +++ b/cli/tests/testdata/npm/node_modules_import/main.out @@ -1,2 +1,3 @@ 2 2 +2 diff --git a/cli/tests/testdata/npm/node_modules_import/main.ts b/cli/tests/testdata/npm/node_modules_import/main.ts index ed433ed049..848ca0f81e 100644 --- a/cli/tests/testdata/npm/node_modules_import/main.ts +++ b/cli/tests/testdata/npm/node_modules_import/main.ts @@ -1,13 +1,16 @@ import * as myImport1 from "@denotest/esm-basic"; import * as myImport2 from "./node_modules/@denotest/esm-basic/main.mjs"; +import * as myImport3 from "@denotest/esm-basic/main.mjs"; myImport1.setValue(5); myImport2.setValue(2); -// these should both give type errors +// these should all give type errors const value1: string = myImport1.getValue(); const value2: string = myImport2.getValue(); +const value3: string = myImport3.getValue(); -// these should both be equal because it should be mutating the same module +// these should all be equal because it should be mutating the same module console.log(value1); console.log(value2); +console.log(value3); diff --git a/cli/tests/testdata/npm/node_modules_import/main_check.out b/cli/tests/testdata/npm/node_modules_import/main_check.out index 4442a97ba0..cf7cc110d4 100644 --- a/cli/tests/testdata/npm/node_modules_import/main_check.out +++ b/cli/tests/testdata/npm/node_modules_import/main_check.out @@ -1,11 +1,16 @@ error: TS2322 [ERROR]: Type 'number' is not assignable to type 'string'. const value1: string = myImport1.getValue(); ~~~~~~ - at file:///[WILDCARD]/npm/node_modules_import/main.ts:8:7 + at file:///[WILDCARD]/npm/node_modules_import/main.ts:9:7 TS2322 [ERROR]: Type 'number' is not assignable to type 'string'. const value2: string = myImport2.getValue(); ~~~~~~ - at file:///[WILDCARD]/npm/node_modules_import/main.ts:9:7 + at file:///[WILDCARD]/npm/node_modules_import/main.ts:10:7 -Found 2 errors. +TS2322 [ERROR]: Type 'number' is not assignable to type 'string'. +const value3: string = myImport3.getValue(); + ~~~~~~ + at file:///[WILDCARD]/npm/node_modules_import/main.ts:11:7 + +Found 3 errors.