diff --git a/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/migration_duplicate_imports/package.json b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/migration_duplicate_imports/package.json new file mode 100644 index 0000000000..1518f9368a --- /dev/null +++ b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/migration_duplicate_imports/package.json @@ -0,0 +1,10 @@ +{ + "name": "migration-duplicate-imports", + "private": true, + "type": "module", + "devDependencies": { + "vite": "npm:@voidzero-dev/vite-plus-core@1.5.0", + "vite-plus": "1.5.0" + }, + "packageManager": "pnpm@10.18.0" +} diff --git a/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/migration_duplicate_imports/snapshots.toml b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/migration_duplicate_imports/snapshots.toml new file mode 100644 index 0000000000..b48c411d8b --- /dev/null +++ b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/migration_duplicate_imports/snapshots.toml @@ -0,0 +1,17 @@ +[[case]] +name = "migration_duplicate_imports" +vp = "global" +steps = [ + ["vp", "migrate", "--no-interactive", "--no-hooks"], + { argv = ["vpt", "print-file", "vite.config.ts"], comment = "Merge the rewritten type import into the existing value import (issue #3430)." }, +] + +[[case]] +name = "migration_duplicate_imports_preserve_code" +vp = "global" +steps = [ + { argv = ["vpt", "replace-file-content", "vite.config.ts", "import { defineConfig }", "const keep = 'keep';\nimport './setup';\nimport { defineConfig }"], snapshot = false }, + { argv = ["vpt", "write-file", "setup.ts", "export {};\n"], snapshot = false }, + ["vp", "migrate", "--no-interactive", "--no-hooks"], + { argv = ["vpt", "print-file", "vite.config.ts"], comment = "Keep statements and side-effect imports between matching imports." }, +] diff --git a/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/migration_duplicate_imports/snapshots/migration_duplicate_imports.md b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/migration_duplicate_imports/snapshots/migration_duplicate_imports.md new file mode 100644 index 0000000000..1612b29514 --- /dev/null +++ b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/migration_duplicate_imports/snapshots/migration_duplicate_imports.md @@ -0,0 +1,29 @@ +# migration_duplicate_imports + +## `vp migrate --no-interactive --no-hooks` + +``` +VITE+ - The Unified Toolchain for the Web + +◇ Updated . to Vite+ +• Node pnpm +• Dependencies: + vite-plus 1.5.0 → + vite → +• 1 file had imports rewritten +• Package manager settings configured +``` + +## `vpt print-file vite.config.ts` + +Merge the rewritten type import into the existing value import (issue #3430). + +``` +import { defineConfig, type Plugin } from 'vite-plus'; + +const noop = (): Plugin => ({ name: 'noop' }); + +export default defineConfig({ + plugins: [noop()], +}); +``` diff --git a/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/migration_duplicate_imports/snapshots/migration_duplicate_imports_preserve_code.md b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/migration_duplicate_imports/snapshots/migration_duplicate_imports_preserve_code.md new file mode 100644 index 0000000000..a5390e6a7c --- /dev/null +++ b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/migration_duplicate_imports/snapshots/migration_duplicate_imports_preserve_code.md @@ -0,0 +1,41 @@ +# migration_duplicate_imports_preserve_code + +## `vpt replace-file-content vite.config.ts 'import { defineConfig }' 'const keep = '\''keep'\''; +import '\''./setup'\''; +import { defineConfig }'` + + +## `vpt write-file setup.ts 'export {}; +'` + + +## `vp migrate --no-interactive --no-hooks` + +``` +VITE+ - The Unified Toolchain for the Web + +◇ Updated . to Vite+ +• Node pnpm +• Dependencies: + vite-plus 1.5.0 → + vite → +• 1 file had imports rewritten +• Package manager settings configured +``` + +## `vpt print-file vite.config.ts` + +Keep statements and side-effect imports between matching imports. + +``` +import type { Plugin } from 'vite-plus'; +const keep = 'keep'; +import './setup'; +import { defineConfig } from 'vite-plus'; + +const noop = (): Plugin => ({ name: 'noop' }); + +export default defineConfig({ + plugins: [noop()], +}); +``` diff --git a/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/migration_duplicate_imports/vite.config.ts b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/migration_duplicate_imports/vite.config.ts new file mode 100644 index 0000000000..9f923c5fb9 --- /dev/null +++ b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/migration_duplicate_imports/vite.config.ts @@ -0,0 +1,8 @@ +import type { Plugin } from 'vite'; +import { defineConfig } from 'vite-plus'; + +const noop = (): Plugin => ({ name: 'noop' }); + +export default defineConfig({ + plugins: [noop()], +}); diff --git a/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/migration_merge_vite_config_ts/snapshots/migration_merge_wrapped_callback.md b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/migration_merge_vite_config_ts/snapshots/migration_merge_wrapped_callback.md index 4e9d33ad43..c23dae4297 100644 --- a/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/migration_merge_vite_config_ts/snapshots/migration_merge_wrapped_callback.md +++ b/crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/migration_merge_vite_config_ts/snapshots/migration_merge_wrapped_callback.md @@ -25,7 +25,7 @@ VITE+ - The Unified Toolchain for the Web - Vitest v5: 1 review item vite.config.ts - 9:29 REVIEW [dynamic-config] Review this dynamic config; explicit v4 compatibility settings need manual insertion. + 7:29 REVIEW [dynamic-config] Review this dynamic config; explicit v4 compatibility settings need manual insertion. Docs: https://viteplus.dev/guide/vitest-v5#resolve-migration-findings ``` @@ -36,9 +36,7 @@ import { join } from 'node:path'; import react from '@vitejs/plugin-react'; import { playwright } from 'vite-plus/test/browser-playwright'; -import { defineConfig, lazyPlugins } from 'vite-plus'; - -import type { UserConfigFn } from 'vite-plus'; +import { defineConfig, type UserConfigFn, lazyPlugins } from 'vite-plus'; export default defineConfig((((() => { return { diff --git a/crates/vp_migration/src/import_merge.rs b/crates/vp_migration/src/import_merge.rs new file mode 100644 index 0000000000..d2dcee5e33 --- /dev/null +++ b/crates/vp_migration/src/import_merge.rs @@ -0,0 +1,322 @@ +use oxc::{ + allocator::Allocator, + ast::ast::{ + ImportDeclaration, ImportDeclarationSpecifier, ImportOrExportKind, Program, Statement, + }, + parser::Parser, + span::{GetSpan, SourceType}, +}; + +/// Merge adjacent imports when migration changed at least one of their sources. +/// Keep unsupported declarations and comments intact rather than reconstructing them. +pub fn merge_duplicate_imports( + original: &str, + content: &str, + filename: Option<&str>, +) -> Option { + let allocator = Allocator::default(); + let source_type = filename + .and_then(|name| SourceType::from_path(name).ok()) + .unwrap_or_else(SourceType::ts) + .with_module(true); + let before = Parser::new(&allocator, original, source_type).parse(); + let after = Parser::new(&allocator, content, source_type).parse(); + if !before.diagnostics.is_empty() || !after.diagnostics.is_empty() { + return None; + } + + let original_imports = imports(&before.program); + let rewritten_imports = imports(&after.program); + // Source rewrites must not add or remove declarations. If they do, we + // cannot reliably identify which imports changed. + if original_imports.len() != rewritten_imports.len() { + return None; + } + + let mut groups: Vec, bool)>> = Vec::new(); + for (original_import, import) in original_imports.iter().zip(rewritten_imports) { + let changed = original_import.source.value != import.source.value; + if let Some(group) = groups.last_mut() { + let previous = group.last().expect("non-empty group").0; + if previous.source.value == import.source.value + && content[previous.span.end as usize..import.span.start as usize] + .chars() + .all(char::is_whitespace) + { + group.push((import, changed)); + continue; + } + } + groups.push(vec![(import, changed)]); + } + + let mut result = content.to_string(); + let mut updated = false; + // Apply edits backwards so all spans still refer to the input text. + for group in groups.into_iter().rev() { + if group.len() < 2 || !group.iter().any(|(_, changed)| *changed) { + continue; + } + let start = group[0].0.span.start; + let end = group.last().expect("non-empty group").0.span.end; + if after + .program + .comments + .iter() + .any(|comment| comment.span.start < end && comment.span.end > start) + { + continue; + } + if let Some(merged) = merge_group(&group, content) { + result.replace_range(start as usize..end as usize, &merged); + updated = true; + } + } + updated.then_some(result) +} + +fn imports<'a>(program: &'a Program<'a>) -> Vec<&'a ImportDeclaration<'a>> { + program + .body + .iter() + .filter_map(|stmt| match stmt { + Statement::ImportDeclaration(import) => Some(import.as_ref()), + _ => None, + }) + .collect() +} + +fn merge_group(group: &[(&ImportDeclaration<'_>, bool)], content: &str) -> Option { + let type_only = group.iter().all(|(import, _)| import.import_kind == ImportOrExportKind::Type); + let mut default = None; + let mut locals = Vec::new(); + let mut named = Vec::new(); + for &(import, _) in group { + if import.with_clause.is_some() || import.phase.is_some() { + return None; + } + let specifiers = import.specifiers.as_deref()?; + // Empty and side-effect imports can force module evaluation. + if specifiers.is_empty() { + return None; + } + for specifier in specifiers { + let local = match specifier { + ImportDeclarationSpecifier::ImportSpecifier(specifier) => { + let is_type = import.import_kind == ImportOrExportKind::Type + || specifier.import_kind == ImportOrExportKind::Type; + let text = specifier.span.source_text(content); + // Reuse the original specifier, including quoted export + // names, aliases, escapes, and existing inline `type`. + let text = if !type_only && import.import_kind == ImportOrExportKind::Type { + format!("type {text}") + } else { + text.to_string() + }; + named.push((is_type, text)); + &specifier.local + } + ImportDeclarationSpecifier::ImportDefaultSpecifier(specifier) => { + if default.is_some() || import.import_kind == ImportOrExportKind::Type { + return None; + } + default = Some(specifier.local.span.source_text(content)); + &specifier.local + } + ImportDeclarationSpecifier::ImportNamespaceSpecifier(_) => return None, + }; + // Different aliases for the same export are valid. Repeated + // locals, including a default/named collision, are not. + if locals.contains(&local.name) { + return None; + } + locals.push(local.name); + } + } + + let mut parts = Vec::new(); + if let Some(default) = default { + parts.push(default.to_string()); + } + if !named.is_empty() { + named.sort_by_key(|(is_type, _)| *is_type); + let named = named.into_iter().map(|(_, text)| text).collect::>().join(", "); + parts.push(format!("{{ {named} }}")); + } + let kind = if type_only { " type" } else { "" }; + let source = group[0].0.source.span().source_text(content); + Some(format!("import{kind} {} from {source};", parts.join(", "))) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn merge(content: &str) -> Option { + let original = content.replace("vite-plus", "vite"); + merge_duplicate_imports(&original, content, Some("file.ts")) + } + + #[test] + fn merges_value_and_type_only_vite_plus_imports() { + // After the migration rewriter has rewritten the `vite` specifier to + // `vite-plus`, this helper collapses the resulting duplicate imports. + let input = r#"import type { Plugin } from 'vite-plus' +import { defineConfig } from 'vite-plus' +"#; + let expected = "import { defineConfig, type Plugin } from 'vite-plus';\n"; + assert_eq!(merge(input).as_deref(), Some(expected)); + } + + #[test] + fn merges_two_value_imports_from_same_source() { + let input = r#"import { defineConfig } from 'vite-plus' +import { Plugin } from 'vite-plus' +"#; + let expected = "import { defineConfig, Plugin } from 'vite-plus';\n"; + assert_eq!(merge(input).as_deref(), Some(expected)); + } + + #[test] + fn merges_two_type_only_imports() { + let input = r#"import type { Plugin } from 'vite-plus' +import type { UserConfig } from 'vite-plus' +"#; + let expected = "import type { Plugin, UserConfig } from 'vite-plus';\n"; + assert_eq!(merge(input).as_deref(), Some(expected)); + } + + #[test] + fn does_not_merge_namespace_imports() { + let input = r#"import * as Vite from 'vite-plus' +import { defineConfig } from 'vite-plus' +"#; + assert_eq!(merge(input), None); + } + + #[test] + fn does_not_merge_default_with_type_only_decl() { + let input = r#"import type Default from 'vite-plus' +import { a } from 'vite-plus' +"#; + assert_eq!(merge(input), None); + } + + #[test] + fn does_not_merge_different_sources() { + let input = r#"import { a } from 'x' +import { b } from 'y' +"#; + assert_eq!(merge(input), None); + } + + #[test] + fn does_not_merge_conflicting_local_bindings() { + let input = r#"import { a } from 'vite-plus' +import { b as a } from 'vite-plus' +"#; + assert_eq!(merge(input), None); + } + + #[test] + fn preserves_statements_and_comments_between_imports() { + for separator in [ + "const keep = 1;", + "run();", + "export const keep = 1;", + "import './setup';", + "import 'vite-plus';", + "import {} from 'vite-plus';", + "// Keep this comment", + "/* Keep this comment */", + "// @ts-expect-error: upstream type mismatch", + ] { + let input = format!( + "import {{ a }} from 'vite-plus';\n{separator}\nimport {{ b }} from 'vite-plus';" + ); + assert_eq!(merge(&input), None, "separator: {separator}"); + } + } + + #[test] + fn preserves_unsupported_imports_and_binding_conflicts() { + for input in [ + "import { /* keep */ a } from 'vite-plus';\nimport { b } from 'vite-plus';", + "import { a } from 'vite-plus' with { type: 'json' };\nimport { b } from 'vite-plus';", + "import a from 'vite-plus';\nimport b from 'vite-plus';", + "import a from 'vite-plus';\nimport { b as a } from 'vite-plus';", + "import { a } from 'vite-plus';\nimport { a } from 'vite-plus';", + "import type { a } from 'vite-plus';\nimport { a } from 'vite-plus';", + "import type { a } from 'vite-plus';\nimport {} from 'vite-plus';", + "import { a } from 'vite-plus';\nimport { b } from 'vite-plus';\nconst = ;", + ] { + assert_eq!(merge(input), None, "input: {input}"); + } + } + + #[test] + fn preserves_quoted_names_escapes_and_aliases() { + let input = r#"import { "a-b" as a, "say\"hi" as hi } from "vite-plus"; +import { "a-b" as b, \u0063 as c } from 'vite-plus';"#; + assert_eq!( + merge(input).as_deref(), + Some( + r#"import { "a-b" as a, "say\"hi" as hi, "a-b" as b, \u0063 as c } from "vite-plus";"# + ) + ); + } + + #[test] + fn merges_default_and_inline_type_imports() { + let input = "import vite, { type Plugin } from 'vite-plus';\nimport { defineConfig } from 'vite-plus';"; + assert_eq!( + merge(input).as_deref(), + Some("import vite, { defineConfig, type Plugin } from 'vite-plus';") + ); + } + + #[test] + fn preserves_inline_type_import_evaluation() { + let input = "import { type Plugin } from 'vite-plus';\nimport type { UserConfig } from 'vite-plus';"; + assert_eq!( + merge(input).as_deref(), + Some("import { type Plugin, type UserConfig } from 'vite-plus';") + ); + } + + #[test] + fn preserves_unchanged_import_groups() { + let input = "import { a } from 'vite-plus';\nimport { b } from 'vite-plus';\nconst x = import('vite');"; + let rewritten = input.replace("import('vite')", "import('vite-plus')"); + assert_eq!(merge_duplicate_imports(input, &rewritten, Some("file.ts")), None); + } + + #[test] + fn merges_multiple_groups_without_changing_surrounding_code() { + let input = "// leading\nimport { a } from 'vite-plus';\nimport { b } from 'vite-plus'; // trailing\nconst keep = '你好';\nimport type { C } from 'vite-plus';\nimport type { D } from 'vite-plus';\n"; + assert_eq!( + merge(input).as_deref(), + Some( + "// leading\nimport { a, b } from 'vite-plus'; // trailing\nconst keep = '你好';\nimport type { C, D } from 'vite-plus';\n" + ) + ); + } + + #[test] + fn supports_jsx_and_typescript_extensions() { + for filename in + ["config.tsx", "config.jsx", "config.mts", "config.cts", "config.mjs", "config.cjs"] + { + let tail = if filename.ends_with("x") { "\nconst view =
;" } else { "" }; + let input = + format!("import {{ a }} from 'vite';\nimport {{ b }} from 'vite-plus';{tail}"); + let rewritten = input.replace("'vite'", "'vite-plus'"); + let expected = format!("import {{ a, b }} from 'vite-plus';{tail}"); + assert_eq!( + merge_duplicate_imports(&input, &rewritten, Some(filename)), + Some(expected), + "filename: {filename}" + ); + } + } +} diff --git a/crates/vp_migration/src/import_rewriter.rs b/crates/vp_migration/src/import_rewriter.rs index fd272760b5..da8643be9f 100644 --- a/crates/vp_migration/src/import_rewriter.rs +++ b/crates/vp_migration/src/import_rewriter.rs @@ -10,7 +10,7 @@ use rayon::prelude::*; use regex::Regex; use vp_error::Error; -use crate::{ast_grep, file_walker, pack_config}; +use crate::{ast_grep, file_walker, import_merge, pack_config}; /// ast-grep rules for rewriting vite imports and declare module statements /// @@ -2424,6 +2424,7 @@ fn rewrite_import( skip_packages, preserve_vitest_in_nuxt_package, is_vite_config, + file_path.to_str(), )?; if let Some(preserve_defaults) = preserve_defaults { let rewritten = @@ -2465,16 +2466,7 @@ fn rewrite_import_content( content: &str, skip_packages: &SkipPackages, ) -> Result { - rewrite_import_content_with_options(content, skip_packages, false) -} - -#[cfg(test)] -fn rewrite_import_content_with_options( - content: &str, - skip_packages: &SkipPackages, - preserve_unscoped_vitest: bool, -) -> Result { - rewrite_import_content_full(content, skip_packages, preserve_unscoped_vitest, true) + rewrite_import_content_full(content, skip_packages, false, true, None) } fn rewrite_import_content_full( @@ -2482,6 +2474,7 @@ fn rewrite_import_content_full( skip_packages: &SkipPackages, preserve_unscoped_vitest: bool, vite_config_file: bool, + filename: Option<&str>, ) -> Result { // Fast path: skip AST parsing if the file doesn't contain any target strings if !content_may_need_rewriting(content, skip_packages) { @@ -2560,6 +2553,16 @@ fn rewrite_import_content_full( &mut preserved_vitest, ); + // Merge adjacent duplicate imports that the per-statement ast-grep rules + // above create when two imports end up referencing the same module. Only + // run when the rules actually changed something, so files that merely + // already happen to have duplicate imports are left untouched. + if updated + && let Some(merged) = import_merge::merge_duplicate_imports(content, &new_content, filename) + { + new_content = merged; + } + Ok(RewriteResult { content: new_content, updated, preserved_vitest, warnings: Vec::new() }) } @@ -2957,6 +2960,55 @@ export default defineConfig({ assert_eq!(result.content, vite_config); } + #[test] + fn test_rewrite_import_content_vite_plus_duplicate_merge() { + let vite_config = r#"import type { Plugin } from 'vite' +import { defineConfig } from 'vite-plus' + +const noop = (): Plugin => ({ name: 'noop' }) + +export default defineConfig({ + plugins: [noop()], +}); +"#; + + let result = rewrite_import_content(vite_config, &SkipPackages::default()).unwrap(); + assert!(result.updated); + assert_eq!( + result.content, + r#"import { defineConfig, type Plugin } from 'vite-plus'; + +const noop = (): Plugin => ({ name: 'noop' }) + +export default defineConfig({ + plugins: [noop()], +}); +"# + ); + } + + #[test] + fn test_rewrite_import_content_duplicate_merge_preserves_other_content() { + let input = "import { defineConfig } from 'vite';\nconst keep = 1;\nimport type { Plugin } from 'vite-plus';\nimport { a } from 'other';\nimport { b } from 'other';"; + let result = rewrite_import_content(input, &SkipPackages::default()).unwrap(); + assert!(result.updated); + assert_eq!(result.content, input.replace("'vite'", "'vite-plus'")); + } + + #[test] + fn test_rewrite_import_content_preserves_existing_duplicate_imports() { + let imports = + "import { defineConfig } from 'vite-plus';\nimport type { Plugin } from 'vite-plus';"; + let result = rewrite_import_content(imports, &SkipPackages::default()).unwrap(); + assert!(!result.updated); + assert_eq!(result.content, imports); + + let input = format!("{imports}\nconst test = import('vitest');"); + let result = rewrite_import_content(&input, &SkipPackages::default()).unwrap(); + assert!(result.updated); + assert_eq!(result.content, input.replace("'vitest'", "'vite-plus/test'")); + } + #[test] fn test_rewrite_import_with_file() { // Create temporary directory (automatically cleaned up when dropped) @@ -4187,8 +4239,7 @@ import type { Context, ESTree } from '@oxlint/plugins';"#; assert!(result.updated); assert_eq!( result.content, - r#"import { definePlugin, defineRule } from "vite-plus/lint/plugins"; -import type { Context, ESTree } from 'vite-plus/lint/plugins';"# + r#"import { definePlugin, defineRule, type Context, type ESTree } from "vite-plus/lint/plugins";"# ); } @@ -4219,8 +4270,7 @@ import { defineRule as rule } from "oxlint";"#; assert!(result.updated); assert_eq!( result.content, - r#"import type { Context } from 'vite-plus/lint/plugins'; -import { defineRule as rule } from "vite-plus/lint/plugins";"# + "import { defineRule as rule, type Context } from 'vite-plus/lint/plugins';" ); } diff --git a/crates/vp_migration/src/lib.rs b/crates/vp_migration/src/lib.rs index 250147e70f..00ced33720 100644 --- a/crates/vp_migration/src/lib.rs +++ b/crates/vp_migration/src/lib.rs @@ -9,6 +9,7 @@ mod ast_grep; mod eslint; mod file_walker; +mod import_merge; mod import_rewriter; mod pack_config; mod package;