Skip to content

Commit 7d5ce4b

Browse files
authored
fix(npm): correct position finding for dependencies with same version (#7)
The parser was using simple `content.find()` to locate dependency names, which would find the first occurrence in the file. This caused issues when: - A package name appeared earlier in the file (e.g., in scripts section) - Multiple packages had the same version string The fix now: 1. Checks if the found name is a JSON key (followed by `:`) 2. Continues searching if not a key 3. Limits version search to 100 chars after colon to stay within the key-value pair Added regression tests for: - Scoped packages (@vitest/coverage-v8) - Package name in scripts section (vitest as script value) - Multiple packages with same version (vitest and @vitest/coverage-v8)
1 parent 218af74 commit 7d5ce4b

File tree

1 file changed

+124
-9
lines changed

1 file changed

+124
-9
lines changed

crates/deps-npm/src/parser.rs

Lines changed: 124 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -157,9 +157,8 @@ fn parse_dependency_section(
157157

158158
/// Finds the position of a dependency name and version in the source text.
159159
///
160-
/// This is a simplified implementation that searches for the first occurrence
161-
/// of the dependency name in quotes. A more robust implementation would use
162-
/// a JSON parser that preserves position information.
160+
/// Searches for the dependency as a JSON key-value pair to avoid false matches
161+
/// when the name appears elsewhere in the file (e.g., in scripts).
163162
fn find_dependency_positions(
164163
content: &str,
165164
name: &str,
@@ -169,26 +168,50 @@ fn find_dependency_positions(
169168
let mut name_range = Range::default();
170169
let mut version_range = None;
171170

172-
let search_pattern = format!("\"{}\"", name);
171+
let name_pattern = format!("\"{}\"", name);
173172

174-
if let Some(name_start_idx) = content.find(&search_pattern) {
173+
// Find all occurrences of the name pattern and check which one is a dependency key
174+
let mut search_start = 0;
175+
while let Some(rel_idx) = content[search_start..].find(&name_pattern) {
176+
let name_start_idx = search_start + rel_idx;
177+
let after_name = &content[name_start_idx + name_pattern.len()..];
178+
179+
// Check if this is a JSON key (followed by optional whitespace and colon)
180+
let trimmed = after_name.trim_start();
181+
if !trimmed.starts_with(':') {
182+
// Not a key, continue searching
183+
search_start = name_start_idx + name_pattern.len();
184+
continue;
185+
}
186+
187+
// Found a valid key, calculate position
175188
let name_start = line_table.position_from_offset(name_start_idx + 1);
176189
let name_end = line_table.position_from_offset(name_start_idx + 1 + name.len());
177190
name_range = Range::new(name_start, name_end);
178191

179-
// Find version position (after the name)
192+
// Find version position (after the colon)
180193
if let Some(version) = version_req {
181-
let search_after_name = &content[name_start_idx..];
182194
let version_search = format!("\"{}\"", version);
195+
// Search for version only in the portion after the colon
196+
let colon_offset =
197+
name_start_idx + name_pattern.len() + (after_name.len() - trimmed.len());
198+
let after_colon = &content[colon_offset..];
199+
200+
// Limit search to the next 100 chars to stay within this key-value pair
201+
let search_limit = after_colon.len().min(100 + version.len());
202+
let search_area = &after_colon[..search_limit];
183203

184-
if let Some(rel_idx) = search_after_name.find(&version_search) {
185-
let version_start_idx = name_start_idx + rel_idx + 1;
204+
if let Some(ver_rel_idx) = search_area.find(&version_search) {
205+
let version_start_idx = colon_offset + ver_rel_idx + 1;
186206
let version_start = line_table.position_from_offset(version_start_idx);
187207
let version_end =
188208
line_table.position_from_offset(version_start_idx + version.len());
189209
version_range = Some(Range::new(version_start, version_end));
190210
}
191211
}
212+
213+
// Found valid dependency, stop searching
214+
break;
192215
}
193216

194217
(name_range, version_range)
@@ -403,4 +426,96 @@ mod tests {
403426
Some("file:../local-package".into())
404427
);
405428
}
429+
430+
#[test]
431+
fn test_scoped_package() {
432+
let json = r#"{
433+
"devDependencies": {
434+
"@vitest/coverage-v8": "^3.1.4"
435+
}
436+
}"#;
437+
438+
let result = parse_package_json(json).unwrap();
439+
assert_eq!(result.dependencies.len(), 1);
440+
assert_eq!(result.dependencies[0].name, "@vitest/coverage-v8");
441+
assert_eq!(result.dependencies[0].version_req, Some("^3.1.4".into()));
442+
assert!(result.dependencies[0].version_range.is_some());
443+
}
444+
445+
#[test]
446+
fn test_package_name_in_scripts_not_confused() {
447+
// Regression test: "vitest" appears in scripts as a value,
448+
// but should only be found as a dependency key
449+
let json = r#"{
450+
"scripts": {
451+
"test": "vitest",
452+
"coverage": "vitest run --coverage"
453+
},
454+
"devDependencies": {
455+
"vitest": "^3.1.4"
456+
}
457+
}"#;
458+
459+
let result = parse_package_json(json).unwrap();
460+
assert_eq!(result.dependencies.len(), 1);
461+
462+
let vitest = &result.dependencies[0];
463+
assert_eq!(vitest.name, "vitest");
464+
assert_eq!(vitest.version_req, Some("^3.1.4".into()));
465+
// Verify version_range is found (this was the bug)
466+
assert!(
467+
vitest.version_range.is_some(),
468+
"vitest should have a version_range"
469+
);
470+
// Verify position is in devDependencies, not scripts
471+
// devDependencies starts at line 6
472+
assert!(
473+
vitest.name_range.start.line >= 5,
474+
"vitest should be found in devDependencies, not scripts"
475+
);
476+
}
477+
478+
#[test]
479+
fn test_multiple_packages_same_version() {
480+
// Both packages have the same version - each should have distinct positions
481+
let json = r#"{
482+
"devDependencies": {
483+
"@vitest/coverage-v8": "^3.1.4",
484+
"vitest": "^3.1.4"
485+
}
486+
}"#;
487+
488+
let result = parse_package_json(json).unwrap();
489+
assert_eq!(result.dependencies.len(), 2);
490+
491+
// Find both dependencies
492+
let coverage = result
493+
.dependencies
494+
.iter()
495+
.find(|d| d.name == "@vitest/coverage-v8")
496+
.expect("@vitest/coverage-v8 should be parsed");
497+
let vitest = result
498+
.dependencies
499+
.iter()
500+
.find(|d| d.name == "vitest")
501+
.expect("vitest should be parsed");
502+
503+
// Both should have version ranges
504+
assert!(
505+
coverage.version_range.is_some(),
506+
"@vitest/coverage-v8 should have version_range"
507+
);
508+
assert!(
509+
vitest.version_range.is_some(),
510+
"vitest should have version_range"
511+
);
512+
513+
// Positions should be different
514+
let coverage_pos = coverage.version_range.unwrap();
515+
let vitest_pos = vitest.version_range.unwrap();
516+
assert_ne!(
517+
coverage_pos.start.line, vitest_pos.start.line,
518+
"version positions should be on different lines"
519+
);
520+
}
406521
}

0 commit comments

Comments
 (0)