Skip to content

Commit 9aa3b7d

Browse files
committed
fix(lint): address review on the Oxlint plugin API rewrite
Seven fixes from review on #2328. Correctness of the rewrite: - A statement that mixes the two `oxlint` surfaces, such as `import { defineConfig, defineRule } from 'oxlint'`, is now left alone. The rewrite replaces the whole specifier, so moving it stripped `defineConfig` of its module. Splitting is the user's call. - `require('@oxlint/plugins')` and `require('oxlint/plugins-dev')` no longer rewrite. The `vite-plus/lint/*` exports are ESM-only, so a rewritten `require()` failed with ERR_PACKAGE_PATH_NOT_EXPORTED. Static import, export, and dynamic `import()` still rewrite, because those resolve through the `import` condition. The published-plugin exemption, which did not work end to end: - `rewritePackageJson` strips `oxlint` before the import rewriter reads the manifests, so `SkipPackages::skip_oxlint` never saw the signal in a real migration. `collectOxlintOwnerDirs` now captures it before the edit and passes the directories through to the rewriter. Note that `skip_tsdown` has the same latent flaw, since `tsdown` is also in `REMOVE_PACKAGES`; this change does not touch it. - `vp lint --fix` rewrote a published plugin's source unconditionally, undoing the exemption the migration had just honored. The rule now checks the nearest manifest, reusing the mtime-keyed cache shape already used for `@nuxt/test-utils`. - The `oxlint` peer entry is no longer stripped from a package that owns the plugin API. A peer is a consumer contract, not a tool the package runs, and removing it left the source importing a package the manifest no longer declared. - `declare module '@oxlint/plugins'` and the `oxlint` forms are preserved, the same way the rule already preserves Vitest-family augmentations. The re-exported types keep their upstream module identity, so a retargeted augmentation stopped merging. Cleanup: - The migration now drops a dead `@oxlint/plugins` devDependency, since nothing imports it after the rewrite. Only from devDependencies: a `dependencies` or `peerDependencies` edge marks a published plugin. Tests: 3 Rust cases for the mixed, `require`, and dynamic-import rules; 8 lint-rule cases for the published-plugin and augmentation guards; and a new `migration_oxlint_published_plugin` snapshot fixture covering the skip.
1 parent cf00aad commit 9aa3b7d

17 files changed

Lines changed: 492 additions & 80 deletions

File tree

crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/migration_oxlint_js_plugin_imports/snapshots.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ steps = [
1111
"vpt",
1212
"print-file",
1313
"package.json",
14-
], comment = "oxlint is removed and nothing replaces it. The API now comes from vite-plus. @oxlint/plugins stays on purpose: it is inert once the imports are rewritten, and removing it would also remove the peer dependency of a published Oxlint plugin", continue-on-failure = true },
14+
], comment = "oxlint and @oxlint/plugins are both gone from devDependencies, and nothing replaces them. The API now comes from vite-plus", continue-on-failure = true },
1515
{ argv = [
1616
"vpt",
1717
"print-file",

crates/vp_cli_snapshots/tests/cli_snapshots/fixtures/migration_oxlint_js_plugin_imports/snapshots/migration_oxlint_js_plugin_imports.md

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ VITE+ - The Unified Toolchain for the Web
1414

1515
## `vpt print-file package.json`
1616

17-
oxlint is removed and nothing replaces it. The API now comes from vite-plus. @oxlint/plugins stays on purpose: it is inert once the imports are rewritten, and removing it would also remove the peer dependency of a published Oxlint plugin
17+
oxlint and @oxlint/plugins are both gone from devDependencies, and nothing replaces them. The API now comes from vite-plus
1818

1919
```
2020
{
@@ -24,7 +24,6 @@ oxlint is removed and nothing replaces it. The API now comes from vite-plus. @ox
2424
"prepare": "vp config"
2525
},
2626
"devDependencies": {
27-
"@oxlint/plugins": "^1.0.0",
2827
"vite": "catalog:",
2928
"vite-plus": "catalog:"
3029
},
Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
import { defineRule } from 'oxlint';
2+
3+
export const noFoo = defineRule({
4+
meta: { messages: { noFoo: 'Do not name things "foo".' } },
5+
create(context) {
6+
return {
7+
Identifier(node) {
8+
if (node.name === 'foo') {
9+
context.report({ node, messageId: 'noFoo' });
10+
}
11+
},
12+
};
13+
},
14+
});
Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
{
2+
"name": "oxlint-plugin-example",
3+
"version": "1.0.0",
4+
"scripts": {
5+
"lint": "oxlint ."
6+
},
7+
"peerDependencies": {
8+
"oxlint": "^1.0.0"
9+
},
10+
"devDependencies": {
11+
"vite": "^7.0.0"
12+
}
13+
}
Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
[[case]]
2+
name = "migration_oxlint_published_plugin"
3+
vp = "global"
4+
steps = [
5+
{ argv = [
6+
"vp",
7+
"migrate",
8+
"--no-interactive",
9+
], comment = "this package declares `oxlint` as a peer dependency, which marks it a published Oxlint plugin", continue-on-failure = true },
10+
{ argv = [
11+
"vpt",
12+
"print-file",
13+
"lint/index.js",
14+
], comment = "the authoring import stays on 'oxlint'. Consumers of a published plugin may run plain Oxlint, so a rewrite to vite-plus would break them. This also covers the ordering trap: rewritePackageJson strips `oxlint` before the import rewriter reads the manifest, so the skip signal is captured up front", continue-on-failure = true },
15+
{ argv = [
16+
"vpt",
17+
"print-file",
18+
"package.json",
19+
], comment = "the `oxlint` peer entry survives. It is a consumer contract, not a tool this package runs, and stripping it would leave the source importing a package the manifest no longer declares", continue-on-failure = true },
20+
]
Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
# migration_oxlint_published_plugin
2+
3+
## `vp migrate --no-interactive`
4+
5+
this package declares `oxlint` as a peer dependency, which marks it a published Oxlint plugin
6+
7+
```
8+
VITE+ - The Unified Toolchain for the Web
9+
10+
◇ Migrated . to Vite+ <version>
11+
• Node <version> pnpm <version>
12+
• 2 config updates applied
13+
```
14+
15+
## `vpt print-file lint/index.js`
16+
17+
the authoring import stays on 'oxlint'. Consumers of a published plugin may run plain Oxlint, so a rewrite to vite-plus would break them. This also covers the ordering trap: rewritePackageJson strips `oxlint` before the import rewriter reads the manifest, so the skip signal is captured up front
18+
19+
```
20+
import { defineRule } from 'oxlint';
21+
22+
export const noFoo = defineRule({
23+
meta: { messages: { noFoo: 'Do not name things "foo".' } },
24+
create(context) {
25+
return {
26+
Identifier(node) {
27+
if (node.name === 'foo') {
28+
context.report({ node, messageId: 'noFoo' });
29+
}
30+
},
31+
};
32+
},
33+
});
34+
```
35+
36+
## `vpt print-file package.json`
37+
38+
the `oxlint` peer entry survives. It is a consumer contract, not a tool this package runs, and stripping it would leave the source importing a package the manifest no longer declares
39+
40+
```
41+
{
42+
"name": "oxlint-plugin-example",
43+
"version": "1.0.0",
44+
"scripts": {
45+
"lint": "vp lint .",
46+
"prepare": "vp config"
47+
},
48+
"peerDependencies": {
49+
"oxlint": "^1.0.0"
50+
},
51+
"devDependencies": {
52+
"vite": "catalog:",
53+
"vite-plus": "catalog:"
54+
},
55+
"devEngines": {
56+
"packageManager": {
57+
"name": "pnpm",
58+
"version": "<version>",
59+
"onFail": "download"
60+
}
61+
}
62+
}
63+
```

crates/vp_migration/src/import_rewriter.rs

Lines changed: 88 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
use std::{
2-
collections::HashMap,
2+
collections::{HashMap, HashSet},
33
path::{Path, PathBuf},
44
sync::LazyLock,
55
};
@@ -1596,12 +1596,20 @@ fix: $NEW_IMPORT
15961596
/// plugin type names. An unrecognized name falls on the side of fixing the
15971597
/// breakage.
15981598
///
1599+
/// A statement that mixes the two surfaces, such as
1600+
/// `import { defineConfig, defineRule } from 'oxlint'`, is left alone. The
1601+
/// rewrite replaces the whole specifier, so moving it would strip
1602+
/// `defineConfig` of its module. Splitting the statement is the user's call.
1603+
///
15991604
/// These forms name no specifier, so the rewrite skips them: namespace imports
16001605
/// (`import * as`), default imports, bare side-effect imports,
16011606
/// `require('oxlint')`, and `import('oxlint')`.
16021607
///
16031608
/// `@oxlint/plugins` and `oxlint/plugins-dev` are unambiguous. They expose only
1604-
/// the plugin API and the dev-time utilities, so every statement form rewrites.
1609+
/// the plugin API and the dev-time utilities, so import, export, and dynamic
1610+
/// `import()` statements all rewrite. `require()` does NOT: the
1611+
/// `vite-plus/lint/*` exports are ESM-only, so a rewritten `require()` would
1612+
/// fail to resolve with ERR_PACKAGE_PATH_NOT_EXPORTED.
16051613
///
16061614
/// The rewrite skips a package that declares `oxlint` or `@oxlint/plugins` in
16071615
/// `dependencies` or `peerDependencies`. Those are published Oxlint plugins,
@@ -1639,27 +1647,6 @@ transform:
16391647
by: "vite-plus/lint/plugins"
16401648
fix: $NEW_IMPORT
16411649
---
1642-
id: rewrite-oxlint-plugins-require
1643-
language: TypeScript
1644-
rule:
1645-
pattern: $STR
1646-
kind: string
1647-
regex: ^['"]@oxlint/plugins['"]$
1648-
inside:
1649-
kind: arguments
1650-
inside:
1651-
kind: call_expression
1652-
has:
1653-
field: function
1654-
regex: ^require$
1655-
transform:
1656-
NEW_IMPORT:
1657-
replace:
1658-
source: $STR
1659-
replace: "@oxlint/plugins"
1660-
by: "vite-plus/lint/plugins"
1661-
fix: $NEW_IMPORT
1662-
---
16631650
id: rewrite-oxlint-plugins-dynamic-import
16641651
language: TypeScript
16651652
rule:
@@ -1713,27 +1700,6 @@ transform:
17131700
by: "vite-plus/lint/plugins-dev"
17141701
fix: $NEW_IMPORT
17151702
---
1716-
id: rewrite-oxlint-plugins-dev-require
1717-
language: TypeScript
1718-
rule:
1719-
pattern: $STR
1720-
kind: string
1721-
regex: ^['"]oxlint/plugins-dev['"]$
1722-
inside:
1723-
kind: arguments
1724-
inside:
1725-
kind: call_expression
1726-
has:
1727-
field: function
1728-
regex: ^require$
1729-
transform:
1730-
NEW_IMPORT:
1731-
replace:
1732-
source: $STR
1733-
replace: oxlint/plugins-dev
1734-
by: "vite-plus/lint/plugins-dev"
1735-
fix: $NEW_IMPORT
1736-
---
17371703
id: rewrite-oxlint-plugins-dev-dynamic-import
17381704
language: TypeScript
17391705
rule:
@@ -1763,13 +1729,17 @@ rule:
17631729
regex: ^['"]oxlint['"]$
17641730
inside:
17651731
kind: import_statement
1766-
has:
1767-
kind: import_specifier
1768-
stopBy: end
1769-
not:
1770-
has:
1771-
field: name
1772-
regex: ^(defineConfig|AllowWarnDeny|DummyRule|DummyRuleMap|ExternalPluginEntry|ExternalPluginsConfig|OxlintConfig|OxlintEnv|OxlintGlobals|OxlintOverride|RuleCategories)$
1732+
all:
1733+
- has:
1734+
kind: import_specifier
1735+
stopBy: end
1736+
- not:
1737+
has:
1738+
kind: import_specifier
1739+
stopBy: end
1740+
has:
1741+
field: name
1742+
regex: ^(defineConfig|AllowWarnDeny|DummyRule|DummyRuleMap|ExternalPluginEntry|ExternalPluginsConfig|OxlintConfig|OxlintEnv|OxlintGlobals|OxlintOverride|RuleCategories)$
17731743
transform:
17741744
NEW_IMPORT:
17751745
replace:
@@ -2187,11 +2157,22 @@ struct PackageRewriteContext {
21872157
}
21882158

21892159
/// Options controlling directory-wide import rewriting.
2190-
#[derive(Debug, Clone, Copy, Default)]
2160+
#[derive(Debug, Clone, Default)]
21912161
pub struct RewriteImportsOptions {
21922162
/// Preserve `vitest` and `vitest/*` module specifiers throughout packages
21932163
/// whose nearest package.json declares `@nuxt/test-utils`.
21942164
pub preserve_vitest_in_nuxt_packages: bool,
2165+
/// Directories of packages that declared `oxlint` or `@oxlint/plugins` in
2166+
/// `dependencies` or `peerDependencies` BEFORE the migration edited their
2167+
/// manifests.
2168+
///
2169+
/// `rewritePackageJson` strips `oxlint` (it is in `REMOVE_PACKAGES`) before
2170+
/// import rewriting reads the manifests, so `get_package_rewrite_context`
2171+
/// can no longer see that signal on disk. The caller captures it up front
2172+
/// and passes it here, otherwise a published Oxlint plugin that declared
2173+
/// the legacy `oxlint` package would lose its exemption and get rewritten
2174+
/// to depend on Vite+.
2175+
pub oxlint_owner_dirs: Vec<PathBuf>,
21952176
}
21962177

21972178
impl SkipPackages {
@@ -2406,15 +2387,26 @@ pub fn rewrite_imports_in_directory_with_options(
24062387

24072388
// Pre-compute package context for each file (requires mutable cache, done sequentially).
24082389
let mut package_context_cache: HashMap<PathBuf, PackageRewriteContext> = HashMap::new();
2390+
// Packages whose manifest declared `oxlint` / `@oxlint/plugins` before the
2391+
// migration edited it. Matched by the package DIRECTORY because the
2392+
// manifest itself no longer carries the signal (see `oxlint_owner_dirs`).
2393+
let oxlint_owner_dirs: HashSet<PathBuf> = options.oxlint_owner_dirs.iter().cloned().collect();
2394+
24092395
let files_with_context: Vec<(PathBuf, PackageRewriteContext)> = walk_result
24102396
.files
24112397
.into_iter()
24122398
.map(|file_path| {
24132399
let package_context =
24142400
if let Some(package_json_path) = find_nearest_package_json(&file_path, root) {
2415-
*package_context_cache
2401+
let mut context = *package_context_cache
24162402
.entry(package_json_path.clone())
2417-
.or_insert_with(|| get_package_rewrite_context(&package_json_path))
2403+
.or_insert_with(|| get_package_rewrite_context(&package_json_path));
2404+
if let Some(package_dir) = package_json_path.parent()
2405+
&& oxlint_owner_dirs.contains(package_dir)
2406+
{
2407+
context.skip_packages.skip_oxlint = true;
2408+
}
2409+
context
24182410
} else {
24192411
PackageRewriteContext::default()
24202412
};
@@ -3379,7 +3371,10 @@ import { mockNuxtImport } from '@nuxt/test-utils/runtime';"#,
33793371

33803372
let result = rewrite_imports_in_directory_with_options(
33813373
temp.path(),
3382-
RewriteImportsOptions { preserve_vitest_in_nuxt_packages: true },
3374+
RewriteImportsOptions {
3375+
preserve_vitest_in_nuxt_packages: true,
3376+
..RewriteImportsOptions::default()
3377+
},
33833378
)
33843379
.unwrap();
33853380

@@ -3417,7 +3412,10 @@ import { mockNuxtImport } from '@nuxt/test-utils/runtime';"#,
34173412

34183413
let result = rewrite_imports_in_directory_with_options(
34193414
temp.path(),
3420-
RewriteImportsOptions { preserve_vitest_in_nuxt_packages: true },
3415+
RewriteImportsOptions {
3416+
preserve_vitest_in_nuxt_packages: true,
3417+
..RewriteImportsOptions::default()
3418+
},
34213419
)
34223420
.unwrap();
34233421

@@ -4028,6 +4026,40 @@ new RuleTester().run('no-foo', noFoo, { valid: [], invalid: [] });"#
40284026
);
40294027
}
40304028

4029+
#[test]
4030+
fn test_rewrite_import_content_oxlint_mixed_surfaces_are_left_alone() {
4031+
// Replacing the specifier would move `defineConfig` to an entry that
4032+
// does not export it. Splitting the statement is the user's call.
4033+
let mixed = r#"import { defineConfig, defineRule } from 'oxlint';"#;
4034+
4035+
let result = rewrite_import_content(mixed, &SkipPackages::default()).unwrap();
4036+
assert!(!result.updated);
4037+
assert_eq!(result.content, mixed);
4038+
}
4039+
4040+
#[test]
4041+
fn test_rewrite_import_content_oxlint_require_is_left_alone() {
4042+
// `vite-plus/lint/plugins` is an ESM-only export, so a rewritten
4043+
// `require()` would fail with ERR_PACKAGE_PATH_NOT_EXPORTED.
4044+
let cjs = r#"const { defineRule } = require('@oxlint/plugins');
4045+
const { RuleTester } = require('oxlint/plugins-dev');"#;
4046+
4047+
let result = rewrite_import_content(cjs, &SkipPackages::default()).unwrap();
4048+
assert!(!result.updated);
4049+
assert_eq!(result.content, cjs);
4050+
}
4051+
4052+
#[test]
4053+
fn test_rewrite_import_content_oxlint_dynamic_import_still_rewrites() {
4054+
// Dynamic `import()` resolves through the `import` condition, so the
4055+
// ESM-only export is reachable.
4056+
let dynamic = r#"const plugins = await import('@oxlint/plugins');"#;
4057+
4058+
let result = rewrite_import_content(dynamic, &SkipPackages::default()).unwrap();
4059+
assert!(result.updated);
4060+
assert_eq!(result.content, r#"const plugins = await import('vite-plus/lint/plugins');"#);
4061+
}
4062+
40314063
#[test]
40324064
fn test_rewrite_import_content_oxlint_skipped_for_published_plugins() {
40334065
let plugin = r#"import { defineRule } from '@oxlint/plugins';"#;

packages/cli/binding/index.d.cts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3621,6 +3621,7 @@ export declare function rewriteEslint(scriptsJson: string): string | null;
36213621
export declare function rewriteImportsInDirectory(
36223622
root: string,
36233623
preserveVitestInNuxtPackages?: boolean | undefined | null,
3624+
oxlintOwnerDirs?: Array<string> | undefined | null,
36243625
): BatchRewriteResult;
36253626

36263627
/**

packages/cli/binding/src/migration.rs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -290,11 +290,17 @@ pub fn wrap_lazy_plugins(vite_config_path: String) -> Result<MergeJsonConfigResu
290290
pub fn rewrite_imports_in_directory(
291291
root: String,
292292
preserve_vitest_in_nuxt_packages: Option<bool>,
293+
oxlint_owner_dirs: Option<Vec<String>>,
293294
) -> Result<BatchRewriteResult> {
294295
let result = vp_migration::rewrite_imports_in_directory_with_options(
295296
Path::new(&root),
296297
vp_migration::RewriteImportsOptions {
297298
preserve_vitest_in_nuxt_packages: preserve_vitest_in_nuxt_packages.unwrap_or(false),
299+
oxlint_owner_dirs: oxlint_owner_dirs
300+
.unwrap_or_default()
301+
.into_iter()
302+
.map(std::path::PathBuf::from)
303+
.collect(),
298304
},
299305
)
300306
.map_err(anyhow::Error::from)?;

0 commit comments

Comments
 (0)