diff options
| -rw-r--r-- | cli/npm/registry.rs | 6 | ||||
| -rw-r--r-- | cli/npm/resolution/graph.rs | 285 | ||||
| -rw-r--r-- | cli/npm/semver/mod.rs | 148 | ||||
| -rw-r--r-- | cli/npm/semver/specifier.rs | 7 |
4 files changed, 335 insertions, 111 deletions
diff --git a/cli/npm/registry.rs b/cli/npm/registry.rs index daefec04c..e6af92fe8 100644 --- a/cli/npm/registry.rs +++ b/cli/npm/registry.rs @@ -518,6 +518,12 @@ impl TestNpmRegistryApi { .insert(package_to.0.to_string(), package_to.1.to_string()); } + pub fn add_dist_tag(&self, package_name: &str, tag: &str, version: &str) { + let mut infos = self.package_infos.lock(); + let info = infos.get_mut(package_name).unwrap(); + info.dist_tags.insert(tag.to_string(), version.to_string()); + } + pub fn add_peer_dependency( &self, package_from: (&str, &str), diff --git a/cli/npm/resolution/graph.rs b/cli/npm/resolution/graph.rs index a182299c7..8995b4076 100644 --- a/cli/npm/resolution/graph.rs +++ b/cli/npm/resolution/graph.rs @@ -411,19 +411,19 @@ impl<'a, TNpmRegistryApi: NpmRegistryApi> } } - fn resolve_best_package_version_and_info( + fn resolve_best_package_version_and_info<'info>( &self, name: &str, version_matcher: &impl NpmVersionMatcher, - package_info: &NpmPackageInfo, - ) -> Result<VersionAndInfo, AnyError> { + package_info: &'info NpmPackageInfo, + ) -> Result<VersionAndInfo<'info>, AnyError> { if let Some(version) = self.resolve_best_package_version(name, version_matcher) { match package_info.versions.get(&version.to_string()) { Some(version_info) => Ok(VersionAndInfo { version, - info: version_info.clone(), + info: version_info, }), None => { bail!("could not find version '{}' for '{}'", version, name) @@ -670,16 +670,21 @@ impl<'a, TNpmRegistryApi: NpmRegistryApi> ) -> Result<Option<NpmPackageId>, AnyError> { fn find_matching_child<'a>( peer_dep: &NpmDependencyEntry, + peer_package_info: &NpmPackageInfo, children: impl Iterator<Item = &'a NpmPackageId>, - ) -> Option<NpmPackageId> { + ) -> Result<Option<NpmPackageId>, AnyError> { for child_id in children { if child_id.name == peer_dep.name - && peer_dep.version_req.satisfies(&child_id.version) + && version_req_satisfies( + &peer_dep.version_req, + &child_id.version, + peer_package_info, + )? { - return Some(child_id.clone()); + return Ok(Some(child_id.clone())); } } - None + Ok(None) } // Peer dependencies are resolved based on its ancestors' siblings. @@ -712,8 +717,11 @@ impl<'a, TNpmRegistryApi: NpmRegistryApi> match &ancestor { NodeParent::Node(ancestor_node_id) => { let maybe_peer_dep_id = if ancestor_node_id.name == peer_dep.name - && peer_dep.version_req.satisfies(&ancestor_node_id.version) - { + && version_req_satisfies( + &peer_dep.version_req, + &ancestor_node_id.version, + peer_package_info, + )? { Some(ancestor_node_id.clone()) } else { let ancestor = self.graph.borrow_node(ancestor_node_id); @@ -731,7 +739,11 @@ impl<'a, TNpmRegistryApi: NpmRegistryApi> } } } - find_matching_child(peer_dep, ancestor.children.values()) + find_matching_child( + peer_dep, + peer_package_info, + ancestor.children.values(), + )? }; if let Some(peer_dep_id) = maybe_peer_dep_id { if existing_dep_id == Some(&peer_dep_id) { @@ -751,9 +763,11 @@ impl<'a, TNpmRegistryApi: NpmRegistryApi> } NodeParent::Req => { // in this case, the parent is the root so the children are all the package requirements - if let Some(child_id) = - find_matching_child(peer_dep, self.graph.package_reqs.values()) - { + if let Some(child_id) = find_matching_child( + peer_dep, + peer_package_info, + self.graph.package_reqs.values(), + )? { if existing_dep_id == Some(&child_id) { return Ok(None); // do nothing, there's already an existing child dep id for this } @@ -778,7 +792,7 @@ impl<'a, TNpmRegistryApi: NpmRegistryApi> // to resolve based on the package info and will treat this just like any // other dependency when not optional if !peer_dep.kind.is_optional() - // only + // prefer the existing dep id if it exists && existing_dep_id.is_none() { self.analyze_dependency( @@ -917,18 +931,17 @@ impl<'a, TNpmRegistryApi: NpmRegistryApi> } #[derive(Clone)] -struct VersionAndInfo { +struct VersionAndInfo<'a> { version: NpmVersion, - info: NpmPackageVersionInfo, + info: &'a NpmPackageVersionInfo, } -fn get_resolved_package_version_and_info( +fn get_resolved_package_version_and_info<'a>( pkg_name: &str, version_matcher: &impl NpmVersionMatcher, - info: &NpmPackageInfo, + info: &'a NpmPackageInfo, parent: Option<&NpmPackageId>, -) -> Result<VersionAndInfo, AnyError> { - let mut maybe_best_version: Option<VersionAndInfo> = None; +) -> Result<VersionAndInfo<'a>, AnyError> { if let Some(tag) = version_matcher.tag() { // For when someone just specifies @types/node, we want to pull in a // "known good" version of @types/node that works well with Deno and @@ -946,26 +959,9 @@ fn get_resolved_package_version_and_info( ); } - if let Some(version) = info.dist_tags.get(tag) { - match info.versions.get(version) { - Some(info) => { - return Ok(VersionAndInfo { - version: NpmVersion::parse(version)?, - info: info.clone(), - }); - } - None => { - bail!( - "Could not find version '{}' referenced in dist-tag '{}'.", - version, - tag, - ) - } - } - } else { - bail!("Could not find dist-tag '{}'.", tag) - } + tag_to_version_info(info, tag) } else { + let mut maybe_best_version: Option<VersionAndInfo> = None; for version_info in info.versions.values() { let version = NpmVersion::parse(&version_info.version)?; if version_matcher.matches(&version) { @@ -976,36 +972,73 @@ fn get_resolved_package_version_and_info( if is_best_version { maybe_best_version = Some(VersionAndInfo { version, - info: version_info.clone(), + info: version_info, }); } } } - } - match maybe_best_version { - Some(v) => Ok(v), - // If the package isn't found, it likely means that the user needs to use - // `--reload` to get the latest npm package information. Although it seems - // like we could make this smart by fetching the latest information for - // this package here, we really need a full restart. There could be very - // interesting bugs that occur if this package's version was resolved by - // something previous using the old information, then now being smart here - // causes a new fetch of the package information, meaning this time the - // previous resolution of this package's version resolved to an older - // version, but next time to a different version because it has new information. - None => bail!( - concat!( - "Could not find npm package '{}' matching {}{}. ", - "Try retrieving the latest npm package information by running with --reload", + match maybe_best_version { + Some(v) => Ok(v), + // If the package isn't found, it likely means that the user needs to use + // `--reload` to get the latest npm package information. Although it seems + // like we could make this smart by fetching the latest information for + // this package here, we really need a full restart. There could be very + // interesting bugs that occur if this package's version was resolved by + // something previous using the old information, then now being smart here + // causes a new fetch of the package information, meaning this time the + // previous resolution of this package's version resolved to an older + // version, but next time to a different version because it has new information. + None => bail!( + concat!( + "Could not find npm package '{}' matching {}{}. ", + "Try retrieving the latest npm package information by running with --reload", + ), + pkg_name, + version_matcher.version_text(), + match parent { + Some(id) => format!(" as specified in {}", id.display()), + None => String::new(), + } ), - pkg_name, - version_matcher.version_text(), - match parent { - Some(id) => format!(" as specified in {}", id.display()), - None => String::new(), + } + } +} + +fn version_req_satisfies( + version_req: &NpmVersionReq, + version: &NpmVersion, + package_info: &NpmPackageInfo, +) -> Result<bool, AnyError> { + match version_req.tag() { + Some(tag) => { + let tag_version = tag_to_version_info(package_info, tag)?.version; + Ok(tag_version == *version) + } + None => Ok(version_req.matches(version)), + } +} + +fn tag_to_version_info<'a>( + info: &'a NpmPackageInfo, + tag: &str, +) -> Result<VersionAndInfo<'a>, AnyError> { + if let Some(version) = info.dist_tags.get(tag) { + match info.versions.get(version) { + Some(info) => Ok(VersionAndInfo { + version: NpmVersion::parse(version)?, + info, + }), + None => { + bail!( + "Could not find version '{}' referenced in dist-tag '{}'.", + version, + tag, + ) } - ), + } + } else { + bail!("Could not find dist-tag '{}'.", tag) } } @@ -1022,17 +1055,18 @@ mod test { fn test_get_resolved_package_version_and_info() { // dist tag where version doesn't exist let package_ref = NpmPackageReference::from_str("npm:test").unwrap(); + let package_info = NpmPackageInfo { + name: "test".to_string(), + versions: HashMap::new(), + dist_tags: HashMap::from([( + "latest".to_string(), + "1.0.0-alpha".to_string(), + )]), + }; let result = get_resolved_package_version_and_info( "test", &package_ref.req, - &NpmPackageInfo { - name: "test".to_string(), - versions: HashMap::new(), - dist_tags: HashMap::from([( - "latest".to_string(), - "1.0.0-alpha".to_string(), - )]), - }, + &package_info, None, ); assert_eq!( @@ -1042,26 +1076,27 @@ mod test { // dist tag where version is a pre-release let package_ref = NpmPackageReference::from_str("npm:test").unwrap(); + let package_info = NpmPackageInfo { + name: "test".to_string(), + versions: HashMap::from([ + ("0.1.0".to_string(), NpmPackageVersionInfo::default()), + ( + "1.0.0-alpha".to_string(), + NpmPackageVersionInfo { + version: "0.1.0-alpha".to_string(), + ..Default::default() + }, + ), + ]), + dist_tags: HashMap::from([( + "latest".to_string(), + "1.0.0-alpha".to_string(), + )]), + }; let result = get_resolved_package_version_and_info( "test", &package_ref.req, - &NpmPackageInfo { - name: "test".to_string(), - versions: HashMap::from([ - ("0.1.0".to_string(), NpmPackageVersionInfo::default()), - ( - "1.0.0-alpha".to_string(), - NpmPackageVersionInfo { - version: "0.1.0-alpha".to_string(), - ..Default::default() - }, - ), - ]), - dist_tags: HashMap::from([( - "latest".to_string(), - "1.0.0-alpha".to_string(), - )]), - }, + &package_info, None, ); assert_eq!(result.unwrap().version.to_string(), "1.0.0-alpha"); @@ -2226,6 +2261,80 @@ mod test { ); } + #[tokio::test] + async fn resolve_dep_and_peer_dist_tag() { + let api = TestNpmRegistryApi::default(); + api.ensure_package_version("package-a", "1.0.0"); + api.ensure_package_version("package-b", "2.0.0"); + api.ensure_package_version("package-b", "3.0.0"); + api.ensure_package_version("package-c", "1.0.0"); + api.ensure_package_version("package-d", "1.0.0"); + api.add_dependency(("package-a", "1.0.0"), ("package-b", "some-tag")); + api.add_dependency(("package-a", "1.0.0"), ("package-d", "1.0.0")); + api.add_dependency(("package-a", "1.0.0"), ("package-c", "1.0.0")); + api.add_peer_dependency(("package-c", "1.0.0"), ("package-d", "other-tag")); + api.add_dist_tag("package-b", "some-tag", "2.0.0"); + api.add_dist_tag("package-d", "other-tag", "1.0.0"); + + let (packages, package_reqs) = + run_resolver_and_get_output(api, vec!["npm:package-a@1.0"]).await; + assert_eq!( + packages, + vec![ + NpmResolutionPackage { + id: NpmPackageId::from_serialized("package-a@1.0.0_package-d@1.0.0") + .unwrap(), + copy_index: 0, + dependencies: HashMap::from([ + ( + "package-b".to_string(), + NpmPackageId::from_serialized("package-b@2.0.0").unwrap(), + ), + ( + "package-c".to_string(), + NpmPackageId::from_serialized("package-c@1.0.0_package-d@1.0.0") + .unwrap(), + ), + ( + "package-d".to_string(), + NpmPackageId::from_serialized("package-d@1.0.0").unwrap(), + ), + ]), + dist: Default::default(), + }, + NpmResolutionPackage { + id: NpmPackageId::from_serialized("package-b@2.0.0").unwrap(), + copy_index: 0, + dependencies: HashMap::new(), + dist: Default::default(), + }, + NpmResolutionPackage { + id: NpmPackageId::from_serialized("package-c@1.0.0_package-d@1.0.0") + .unwrap(), + copy_index: 0, + dependencies: HashMap::from([( + "package-d".to_string(), + NpmPackageId::from_serialized("package-d@1.0.0").unwrap(), + ),]), + dist: Default::default(), + }, + NpmResolutionPackage { + id: NpmPackageId::from_serialized("package-d@1.0.0").unwrap(), + copy_index: 0, + dependencies: HashMap::new(), + dist: Default::default(), + }, + ] + ); + assert_eq!( + package_reqs, + vec![( + "package-a@1.0".to_string(), + "package-a@1.0.0_package-d@1.0.0".to_string() + ),] + ); + } + async fn run_resolver_and_get_output( api: TestNpmRegistryApi, reqs: Vec<&str>, diff --git a/cli/npm/semver/mod.rs b/cli/npm/semver/mod.rs index cd63b2a29..c8c8e64fd 100644 --- a/cli/npm/semver/mod.rs +++ b/cli/npm/semver/mod.rs @@ -25,6 +25,14 @@ mod specifier; // A lot of the below is a re-implementation of parts of https://github.com/npm/node-semver // which is Copyright (c) Isaac Z. Schlueter and Contributors (ISC License) +pub fn is_valid_npm_tag(value: &str) -> bool { + // a valid tag is anything that doesn't get url encoded + // https://github.com/npm/npm-package-arg/blob/103c0fda8ed8185733919c7c6c73937cfb2baf3a/lib/npa.js#L399-L401 + value + .chars() + .all(|c| c.is_alphanumeric() || matches!(c, '-' | '_' | '.' | '~')) +} + #[derive( Clone, Debug, PartialEq, Eq, Default, Hash, Serialize, Deserialize, )] @@ -164,20 +172,35 @@ fn parse_npm_version(input: &str) -> ParseResult<NpmVersion> { )) } +#[derive(Clone, Debug, PartialEq, Eq, Hash)] +enum NpmVersionReqInner { + RangeSet(VersionRangeSet), + Tag(String), +} + /// A version requirement found in an npm package's dependencies. #[derive(Debug, Clone, PartialEq, Eq, Hash)] pub struct NpmVersionReq { raw_text: String, - range_set: VersionRangeSet, + inner: NpmVersionReqInner, } impl NpmVersionMatcher for NpmVersionReq { fn tag(&self) -> Option<&str> { - None + match &self.inner { + NpmVersionReqInner::RangeSet(_) => None, + NpmVersionReqInner::Tag(tag) => Some(tag.as_str()), + } } fn matches(&self, version: &NpmVersion) -> bool { - self.satisfies(version) + match &self.inner { + NpmVersionReqInner::RangeSet(range_set) => range_set.satisfies(version), + NpmVersionReqInner::Tag(_) => panic!( + "programming error: cannot use matches with a tag: {}", + self.raw_text + ), + } } fn version_text(&self) -> String { @@ -197,16 +220,12 @@ impl NpmVersionReq { with_failure_handling(parse_npm_version_req)(text) .with_context(|| format!("Invalid npm version requirement '{}'.", text)) } - - pub fn satisfies(&self, version: &NpmVersion) -> bool { - self.range_set.satisfies(version) - } } fn parse_npm_version_req(input: &str) -> ParseResult<NpmVersionReq> { - map(range_set, |set| NpmVersionReq { + map(inner, |inner| NpmVersionReq { raw_text: input.to_string(), - range_set: set, + inner, })(input) } @@ -229,14 +248,97 @@ fn parse_npm_version_req(input: &str) -> ParseResult<NpmVersionReq> { // part ::= nr | [-0-9A-Za-z]+ // range-set ::= range ( logical-or range ) * -fn range_set(input: &str) -> ParseResult<VersionRangeSet> { +fn inner(input: &str) -> ParseResult<NpmVersionReqInner> { if input.is_empty() { - return Ok((input, VersionRangeSet(vec![VersionRange::all()]))); + return Ok(( + input, + NpmVersionReqInner::RangeSet(VersionRangeSet(vec![VersionRange::all()])), + )); + } + + let (input, mut ranges) = + separated_list(range_or_invalid, logical_or)(input)?; + + if ranges.len() == 1 { + match ranges.remove(0) { + RangeOrInvalid::Invalid(invalid) => { + if is_valid_npm_tag(invalid.text) { + return Ok(( + input, + NpmVersionReqInner::Tag(invalid.text.to_string()), + )); + } else { + return Err(invalid.failure); + } + } + RangeOrInvalid::Range(range) => { + // add it back + ranges.push(RangeOrInvalid::Range(range)); + } + } + } + + let ranges = ranges + .into_iter() + .filter_map(|r| r.into_range()) + .collect::<Vec<_>>(); + Ok((input, NpmVersionReqInner::RangeSet(VersionRangeSet(ranges)))) +} + +enum RangeOrInvalid<'a> { + Range(VersionRange), + Invalid(InvalidRange<'a>), +} + +impl<'a> RangeOrInvalid<'a> { + pub fn into_range(self) -> Option<VersionRange> { + match self { + RangeOrInvalid::Range(r) => { + if r.is_none() { + None + } else { + Some(r) + } + } + RangeOrInvalid::Invalid(_) => None, + } } - map(if_not_empty(separated_list(range, logical_or)), |ranges| { - // filter out the ranges that won't match anything for the tests - VersionRangeSet(ranges.into_iter().filter(|r| !r.is_none()).collect()) - })(input) +} + +struct InvalidRange<'a> { + failure: ParseError<'a>, + text: &'a str, +} + +fn range_or_invalid(input: &str) -> ParseResult<RangeOrInvalid> { + let range_result = + map_res(map(range, RangeOrInvalid::Range), |result| match result { + Ok((input, range)) => { + let is_end = input.is_empty() || logical_or(input).is_ok(); + if is_end { + Ok((input, range)) + } else { + ParseError::backtrace() + } + } + Err(err) => Err(err), + })(input); + match range_result { + Ok(result) => Ok(result), + Err(failure) => { + let (input, text) = invalid_range(input)?; + Ok(( + input, + RangeOrInvalid::Invalid(InvalidRange { failure, text }), + )) + } + } +} + +fn invalid_range(input: &str) -> ParseResult<&str> { + let end_index = input.find("||").unwrap_or(input.len()); + let text = input[..end_index].trim(); + Ok((&input[end_index..], text)) } // range ::= hyphen | simple ( ' ' simple ) * | '' @@ -505,7 +607,9 @@ mod tests { #[test] pub fn npm_version_req_ranges() { - let tester = NpmVersionReqTester::new(">= 2.1.2 < 3.0.0 || 5.x"); + let tester = NpmVersionReqTester::new( + ">= 2.1.2 < 3.0.0 || 5.x || ignored-invalid-range || $#$%^#$^#$^%@#$%SDF|||", + ); assert!(!tester.matches("2.1.1")); assert!(tester.matches("2.1.2")); assert!(tester.matches("2.9.9")); @@ -515,6 +619,12 @@ mod tests { assert!(!tester.matches("6.1.0")); } + #[test] + pub fn npm_version_req_with_tag() { + let req = NpmVersionReq::parse("latest").unwrap(); + assert_eq!(req.inner, NpmVersionReqInner::Tag("latest".to_string())); + } + macro_rules! assert_cmp { ($a:expr, $b:expr, $expected:expr) => { assert_eq!( @@ -729,7 +839,7 @@ mod tests { let range = NpmVersionReq::parse(range_text).unwrap(); let expected_range = NpmVersionReq::parse(expected).unwrap(); assert_eq!( - range.range_set, expected_range.range_set, + range.inner, expected_range.inner, "failed for {} and {}", range_text, expected ); @@ -853,7 +963,7 @@ mod tests { let req = NpmVersionReq::parse(req_text).unwrap(); let version = NpmVersion::parse(version_text).unwrap(); assert!( - req.satisfies(&version), + req.matches(&version), "Checking {} satisfies {}", req_text, version_text @@ -952,7 +1062,7 @@ mod tests { let req = NpmVersionReq::parse(req_text).unwrap(); let version = NpmVersion::parse(version_text).unwrap(); assert!( - !req.satisfies(&version), + !req.matches(&version), "Checking {} not satisfies {}", req_text, version_text diff --git a/cli/npm/semver/specifier.rs b/cli/npm/semver/specifier.rs index dc4fe1010..269846f8c 100644 --- a/cli/npm/semver/specifier.rs +++ b/cli/npm/semver/specifier.rs @@ -6,6 +6,7 @@ use monch::*; use serde::Deserialize; use serde::Serialize; +use super::is_valid_npm_tag; use super::range::Partial; use super::range::VersionRange; use super::range::XRange; @@ -55,7 +56,7 @@ fn parse_npm_specifier(input: &str) -> ParseResult<SpecifierVersionReq> { map_res(version_range, |result| { let (new_input, range_result) = match result { Ok((input, range)) => (input, Ok(range)), - // use an empty string because we'll consider the tag + // use an empty string because we'll consider it a tag Err(err) => ("", Err(err)), }; Ok(( @@ -65,9 +66,7 @@ fn parse_npm_specifier(input: &str) -> ParseResult<SpecifierVersionReq> { inner: match range_result { Ok(range) => SpecifierVersionReqInner::Range(range), Err(err) => { - // npm seems to be extremely lax on what it supports for a dist-tag (any non-valid semver range), - // so just make any error here be a dist tag unless it starts or ends with whitespace - if input.trim() != input { + if !is_valid_npm_tag(input) { return Err(err); } else { SpecifierVersionReqInner::Tag(input.to_string()) |