fix(migrate): clean up the full ESLint ecosystem, not just eslint#1682
fix(migrate): clean up the full ESLint ecosystem, not just eslint#1682fengmk2 wants to merge 16 commits into
Conversation
✅ Deploy Preview for viteplus-preview canceled.
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub. |
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
fengmk2
force-pushed
the
feat/migrate-cleanup-eslint-ecosystem
branch
from
a9cf627 to
3bac377
Compare
fengmk2
added
test: e2e
fengmk2
changed the title
fengmk2
changed the title
`vp migrate` left `eslint-plugin-*`, `eslint-config-*`, `typescript-eslint`, and `@typescript-eslint/*` behind in `package.json` after removing `eslint` itself, leaving the user with dead packages that only configure ESLint. Expand `rewriteEslintPackageJson` to drop those plus scoped variants (e.g. `@vue/eslint-config-typescript`). Add a `hasTypeAwareEslintConfig` heuristic that recognizes `recommendedTypeChecked`, `projectService`, and `parserOptions.project` across flat configs, legacy `.eslintrc.*`, and `package.json#eslintConfig`, and record the result on the migration report so downstream messaging can be transparent about preserving type-aware coverage in the resulting Oxlint config. Addresses PR comments r3255784734, r3255786019, r3255859061 from WeakAuras/WeakAuras-Companion#2956.
The `hadTypeAwareEslint` flag and its detection helper had no downstream consumers — `mergeViteConfigFiles` and `injectLintTypeCheckDefaults` both already default type-aware to on, so the recorded value never influenced behavior. Remove the helper, the report field, and the related unit tests; the plugin/config cleanup that addresses the underlying PR comments is unaffected. The snap-test fixture `migration-eslint-type-aware` is kept as a regression check that a type-aware flat config still migrates cleanly (typescript-eslint plugins removed; resulting `vite.config.ts` retains `options.typeAware/typeCheck: true`).
…dings
Address findings from the extra-high-effort review of this PR:
- Iterate `peerDependencies` and `optionalDependencies` in addition to
`devDependencies` and `dependencies`, matching the sister code path
that handles vite/vitest overrides.
- Narrow the `@typescript-eslint/*` namespace removal to the
ESLint-specific entry points (`eslint-plugin`, `parser`,
`rule-tester`); preserve the reusable AST libraries (`utils`,
`typescript-estree`, `scope-manager`, `types`) so codemods and doc
generators that import them directly keep working.
- Recognize `@eslint/*`, `@eslint-community/*`, and `@angular-eslint/*`
scopes as ESLint-only (e.g. `@eslint/js`, `@eslint/eslintrc`,
`@eslint-community/eslint-utils`, `@angular-eslint/template-parser`).
- Recognize flat ESLint-only helpers: `eslint-formatter-*`, `eslintrc`,
`eslint-utils`, `eslint-visitor-keys`, `eslint-scope`,
`eslint-define-config`, `eslint-doc-generator`.
- Recognize `@nuxt/eslint` (Nuxt's ESLint integration module that
`require`s `eslint` at runtime — useless after removal). The unit test
that previously asserted this was preserved is now inverted.
- Strip `@types/` symmetrically: a `@types/<X>` is removable iff `<X>`
is. Fixes the inconsistency where `@types/eslint-plugin-foo` was
removed but `@types/eslint` survived.
- Delete a dependency field entirely (`devDependencies`,
`peerDependencies`, etc.) when our cleanup emptied it, avoiding
`"devDependencies": {}` noise. Visible in the
`migration-eslint-monorepo` snap diff.
- Split `rewriteEslintPackageJson` into root vs workspace modes:
workspace sub-packages get a conservative cleanup (only `eslint`
itself), since they may intentionally publish plugins/configs as part
of a shared lint-preset API consumed outside Vite+.
- Drop the broken `"include": ["src/**/*"]` from the
`migration-eslint-type-aware` fixture's `tsconfig.json` (no `src/`
directory exists).
- Expand the `migration-eslint-plugins-cleanup` fixture to exercise
the wider matrix (scopes, formatters, `@types`, preserved
`@typescript-eslint/utils`, peer `eslint`).
Deferred (out of scope for this PR):
- `detectEslintProject` gating on the literal `eslint` symbol — a
project that has only `typescript-eslint` etc. never triggers the
migration. Pre-existing.
- Stale `--rule '@typescript-eslint/...'` arguments in scripts after
plugin removal — requires Rust ast-grep rule updates.
- Non-atomic `fs.writeFileSync` across N workspace `package.json` files;
a SIGKILL mid-loop leaves the monorepo half-migrated. Pre-existing
utility behavior.
So reviewers can see the lint/staged/fmt block the comprehensive cleanup produces, not just the trimmed package.json.
The @nuxt/eslint module wires ESLint into a Nuxt-specific flow that Vite+ can't migrate cleanly today: the generated vite.config.ts ends up referencing @nuxt/eslint-plugin (no longer installed), the user's nuxt.config.ts still loads the removed module via `modules: [...]`, and `nuxt dev` fails to boot until the user untangles it by hand. Detect @nuxt/eslint in the project's package.json (root or any workspace package) and skip ESLint migration entirely with a clear warning that tells the user how to migrate manually. Their ESLint setup — eslint itself, eslint.config.mjs, plugins, the `eslint` script — is preserved verbatim. vite-plus is still added so the rest of the toolchain adoption proceeds. Verified end-to-end against https://github.com/why-reproductions-are-required/vp-migrate-nuxt-eslint (a minimal Nuxt 4 + @nuxt/eslint reproduction).
fengmk2
force-pushed
the
feat/migrate-cleanup-eslint-ecosystem
branch
from
506bd74 to
7073e84
Compare
|
@cursor review |
The fixture's @nuxt/eslint dep was now being intercepted by the new skip-on-@nuxt/eslint behavior, so the snap.txt would silently drift to show the skip path instead of exercising the comprehensive cleanup it was meant to verify. The skip path has its own fixture (`migration-eslint-nuxt-skip`); this one stays focused on cleanup.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 0368b2d. Configure here.
Drop the `mode: 'root' | 'workspace'` parameter on `rewriteEslintPackageJson` and apply `isEslintEcosystemDep` cleanup uniformly across the root and every workspace package. The split was a compromise that didn't hold up: - The "conservative workspace" mode still removed `eslint` from every dependency field (including `peerDependencies`), which by itself breaks any workspace package that published a shared ESLint preset. Keeping the plugins around in that case didn't restore the publishable package — it just made it half-broken instead of fully. - The rest of the migration already treats workspace packages as in scope for adoption (replacing vite-related overrides, adding vite-plus, etc.). A different policy just for plugin removal was inconsistent with the surrounding behavior. A user who genuinely wants to keep a published shared ESLint preset intact should exclude that package from migration rather than rely on partial cleanup. The existing `migration-eslint-monorepo` snap doesn't change — its fixture only had bare `eslint` in subpackages, which workspace mode removed anyway. A new fixture `migration-eslint-monorepo-plugins-in-packages` covers the actual behavior change: workspace packages with plugins / configs / scoped typescript-eslint deps get the full cleanup (including AST-library preservation for `@typescript-eslint/utils`), and emptied dependency fields are stripped.
cpojer
left a comment
cpojer
left a comment
There was a problem hiding this comment.
Are we running eslint afterwards to ensure that none of the removed packages affect the lint run? For example to avoid removing a necessary JS plugin for Oxlint.
|
@cpojer I will look for the real repo to verify it, and then find a suitable repo to add as an e2e test case for ecosystem-ci before merging. |
vite-plus
@voidzero-dev/vite-plus-core
@voidzero-dev/vite-plus-prompts
@voidzero-dev/vite-plus-test
@voidzero-dev/vite-plus-cli-darwin-arm64
@voidzero-dev/vite-plus-cli-darwin-x64
@voidzero-dev/vite-plus-cli-linux-arm64-gnu
@voidzero-dev/vite-plus-cli-linux-arm64-musl
@voidzero-dev/vite-plus-cli-linux-x64-gnu
@voidzero-dev/vite-plus-cli-linux-x64-musl
@voidzero-dev/vite-plus-cli-win32-arm64-msvc
@voidzero-dev/vite-plus-cli-win32-x64-msvc
@voidzero-dev/vite-plus-darwin-arm64
@voidzero-dev/vite-plus-darwin-x64
@voidzero-dev/vite-plus-linux-arm64-gnu
@voidzero-dev/vite-plus-linux-arm64-musl
@voidzero-dev/vite-plus-linux-x64-gnu
@voidzero-dev/vite-plus-linux-x64-musl
@voidzero-dev/vite-plus-win32-arm64-msvc
@voidzero-dev/vite-plus-win32-x64-msvc
commit: |
…ckages Address review findings on the prior "uniform cleanup" commit: 1. `deleteEslintConfigFiles` was still root-only — a workspace package with its own `eslint.config.mjs` ended up with `package.json` deps stripped but the config file lingering as a broken orphan. 2. `rewriteEslintLintStagedConfigFiles` was also root-only — a workspace package with its own `.lintstagedrc.json` kept stale `eslint --fix` entries after `eslint` was removed. 3. The per-workspace loop had no `fs.existsSync` guard; a missing `package.json` between detection and Step 4 would throw `ENOENT` and abort the entire migration mid-flight. 4. The comment claimed "user should opt out of migration for that package rather than rely on partial cleanup", but no such opt-out mechanism exists. Rewritten to point at the actual workaround (excluding the package from `pnpm-workspace.yaml` / `workspaces`). Collapsed Steps 3-5 into one workspace-aware loop that iterates root + every workspace package, skipping any target whose `package.json` disappears between detection and cleanup. The `migration-eslint-monorepo-plugins-in-packages` fixture now ships a per-workspace `eslint.config.mjs` + `.lintstagedrc.json` to lock the new uniform behavior in.
…t config
`@oxlint/migrate` can emit `lint.jsPlugins` / `lint.plugins` / `lint.rules`
entries that point at packages or plugin namespaces that won't resolve
at lint time. Common case: translating `@unocss/eslint-config` into
`jsPlugins: ["eslint-plugin-unocss"]` even though the user never had
`eslint-plugin-unocss` installed. Result: `vp lint` aborts with
"Failed to load JS plugin" or "Plugin not found" before running any
rule, the generated pre-commit hook fails, and the migration looks
successful but the project is unusable.
Add `sanitizeMigratedOxlintConfig` which runs after `@oxlint/migrate`
generates `.oxlintrc.json` and before merging into `vite.config.ts`:
- drop `jsPlugins[]` string entries whose package isn't present in
the root + workspace `package.json` set (computed after the
ESLint-ecosystem cleanup, so removed plugins are caught here too)
- drop `plugins[]` entries that aren't in Oxlint's native plugin
allowlist (`oxc`, `typescript`, `unicorn`, `react`, `vue`, etc. —
sourced from `LintPluginOptionsSchema` in oxlint's typings) AND
aren't contributed by a surviving JS plugin
- drop `rules` / `overrides[].rules` whose namespace prefix isn't
backed by any surviving plugin
- emit a `warnMigration` describing each class of stripped reference,
pointing the user at the manual install path if they want the
coverage back
Verified end-to-end against the real
WeakAuras/WeakAuras-Companion@9c67bd1: after migration, the generated
warning names `eslint-plugin-unocss`, the merged `vite.config.ts` no
longer references `unocss/*`, and `vp lint src/` runs to completion
(53 real lint findings) where previously it failed with
"x Failed to load JS plugin: eslint-plugin-unocss".
New snap-test fixture `migration-eslint-jsplugins-orphan-strip`
covers the same shape with a minimal `eslint.config.mjs` that
declares a `fictional/*` rule via an inline plugin definition
that translates through `@oxlint/migrate` into an unresolvable
`eslint-plugin-fictional` reference.
CI snap-test `new-create-vite-migrates-eslint-prettier` regressed: in
the merged `vite.config.ts`, `rules` appeared BEFORE `jsPlugins`
instead of after.
Root cause: in `sanitizeMigratedOxlintConfig` I unconditionally did
`config.rules = filterRules(config.rules)`. When `@oxlint/migrate`
produced an `.oxlintrc.json` with NO top-level `rules` key (common —
the create-vite fixture only has `overrides[].rules`), the assignment
added `rules: undefined` as a new key. Object iteration treats that
as ordering-significant: when `ensureVitePlusImportRuleDefaults` later
spreads `{...config, jsPlugins, rules}`, `rules` keeps its newly-added
position (before the spread-added `jsPlugins`), so the final object
has `rules` before `jsPlugins`.
Fix: only reassign `config.rules` / `override.rules` when the key was
already present AND the filter actually dropped something. Same for
overrides[]. No behavior change in the cases the new logic was meant
to handle.
Three issues surfaced by another adversarial review of the
sanitize-migrated-oxlint-config pass:
1. **`overrides[].jsPlugins` / `overrides[].plugins` weren't sanitized**
The base config got the full treatment but per-override entries
were ignored. A `files: ['**/*.test.ts']` override that references
an uninstalled plugin reproduces the exact "Failed to load JS
plugin" abort the PR is supposed to fix.
Fix: walk `config.overrides` and run jsPlugins / plugins / rules
sanitization against a per-override namespace set (base namespaces
∪ override-specific jsPlugin namespaces).
2. **Scoped multi-segment namespaces (`@stylistic/ts/*`) were silently
stripped even when their plugin survived.** `filterRules` used
`key.indexOf('/')` (first slash only), but `deriveJsPluginNamespace`
produces multi-slash namespaces like `@stylistic/ts` from packages
like `@stylistic/eslint-plugin-ts`. The two never reconciled.
Fix: walk every slash position from left to right; first prefix
that's in the namespace set wins.
3. **Workspace-context wasn't reaching all sanitizer call sites.**
`bin.ts`'s post-install ESLint-migration path and the per-sub-package
`rewriteMonorepoProject` both called `mergeViteConfigFiles` without
`packages`, so the sanitizer couldn't see deps that lived elsewhere
in the workspace. Hoisted ESLint plugins (a normal pnpm/yarn-classic
pattern) got falsely flagged as orphans.
Fix: plumb `packages` through bin.ts:886, `rewriteMonorepoProject`,
and `rewriteRootWorkspacePackageJson`. Sub-package callers also pass
a `workspaceRoot` so the sanitizer resolves sibling-package paths
relative to the root, not relative to the sub-package being
sanitized.
Other bits caught during this pass:
- Object-form jsPlugins entries (`{ name, specifier }`) still pass
through without an installability check, but local-path string
specifiers (`./X`, `../X`, `/X`) are now preserved so users with
hand-authored Oxlint plugins survive a `vp migrate` re-run.
- Consolidated the two-warning split (cleanup-removed vs
never-installed) into a single accurate warning. The heuristic
used to distinguish them (`isEslintEcosystemDep` name match) was
unreliable: it misclassified the `@unocss/eslint-config → eslint-plugin-unocss`
WeakAuras case as "we removed it" when the user never had it.
- The fictional fixture now also exercises the override path.
Surveyed real-world `.oxlintrc.json` files on GitHub (electron,
RSSHub, remix, cloudflare/workers-sdk, algolia/instantsearch,
facebook/sapling, posva/pinia-colada, generalaction/emdash, …) to
sanity-check the sanitizer against shapes it'd see in the wild.
Found one real gap: posva/pinia-colada uses
`jsPlugins: ["oxlint-plugin-posva"]` with rules under `posva/*`. The
`oxlint-plugin-<name>` convention is Oxlint-native plugin authors'
norm, distinct from the `eslint-plugin-<name>` convention
`@oxlint/migrate` emits. My `deriveJsPluginNamespace` only handled
`eslint-plugin-*`, so `oxlint-plugin-posva` returned the verbatim
string and rules under `posva/*` would have been silently stripped.
Extend the deriver to recognize both `eslint-plugin-` and
`oxlint-plugin-` prefixes (both bare and scoped). Now:
- `oxlint-plugin-posva` → `posva`
- `@scope/oxlint-plugin-x` → `@scope/x`
- `@scope/oxlint-plugin` → `@scope`
Also cleaned up an orphan JSDoc block and added a proper docstring
to `sanitizeMigratedOxlintConfig` documenting the per-override
sanitization behavior.
Other shapes I verified against the sanitizer (no change needed):
- object-form `{name, specifier}` jsPlugins
(electron's no-only-tests, sapling's custom) → passed through ✓
- local-path string jsPlugins (felipebrgs1, generalaction)
→ preserved via the `./`/`/`/`../` check ✓
- `unicorn-js/*` rule namespace from `unicorn-js` package
(RSSHub) → handled by the verbatim fallback in deriveJsPluginNamespace ✓
The entry was unreachable: when `@nuxt/eslint` is present anywhere in the workspace, `detectIncompatibleEslintIntegration` short-circuits the entire ESLint migration before `rewriteEslintPackageJson` runs, so the second list never gets consulted. Two side effects from keeping it duplicated: - Maintenance hazard: the "what to do about Nuxt" decision lives in two places that could drift. - Misleading future intent: a contributor scanning the cleanup list would conclude `@nuxt/eslint` gets stripped on migration, which isn't true today. The right home for the policy is `INCOMPATIBLE_ESLINT_INTEGRATIONS` (which is already what controls the actual behavior). Added an explicit unit test asserting `rewriteEslintPackageJson` leaves `@nuxt/eslint` alone, with a comment explaining the upstream skip.
Real-world coverage for the @oxlint/migrate → sanitizer pipeline. zustand
ships a flat `eslint.config.mjs` that imports eslint-plugin-react,
eslint-plugin-react-hooks, eslint-plugin-import, eslint-plugin-jest-dom,
eslint-plugin-testing-library, and @vitest/eslint-plugin — the most
common React+testing plugin combo in the wild. After `vp migrate`:
- all six plugins are removed from devDependencies by the ESLint
ecosystem cleanup
- `@oxlint/migrate`'s output references those packages in `lint.jsPlugins`
- our sanitizer strips the now-orphan references
- vitest still runs the full 213-test suite (verified locally)
Originally tried vueuse for richer preset-expansion coverage via
`@antfu/eslint-config`, but it hit a separate `vite-plus-core`
config-resolution bug ("Class extends value undefined" inside
resolve-tsconfig) that we'd need to fix first. zustand is the smallest
real candidate that exercises the same sanitizer code paths without
that confounder.
The matrix command currently only invokes `vp run test:spec`. `vp run
test:lint` and `vp run test:format` both hit an upstream JS-config
loader bug ("Cannot use import statement outside a module" in
oxlint/js_config.js) that aborts before our merged config is even
consulted. Documented inline with a FIXME so the failures are visible
but don't block CI; the `|| true` should be removed once the loader is
fixed.
Verified locally:
- clone → patch-project (vp migrate): 2 config updates applied,
17 files imports rewritten, ESLint → Oxlint migrated, Prettier →
Oxfmt migrated, dependencies installed in 9.3s
- vp install --no-frozen-lockfile: clean
- vp test --run: 13 files, 213 tests passed
zustand was added in the previous commit but doesn't actually load
after migration: it ships without `"type": "module"`, so Node treats
the migrated `vite.config.ts` as CJS and chokes on its `import`
statements. Symptom: both `vp lint` and `vp fmt` abort with
"Cannot use import statement outside a module" in
oxlint's js_config.js loader, before our merged config is reached.
(`vp test` works because vitest has its own loader path.)
vite-plugin-vue is the better single-test pick:
- `"type": "module"` ✅
- Real flat-config eslint.config.js importing `eslint-plugin-import-x`,
`eslint-plugin-n`, `eslint-plugin-regexp` — three niche plugins we
have zero other coverage for
- Maintained by the Vite team; vp's own ecosystem, so churn risk is
aligned with vp's release cadence
- Smaller than vueuse (which hits a separate vite-plus-core
resolve-tsconfig bug) and zustand (CJS)
Verified locally end-to-end:
- clone → patch-project (vp migrate): 4 config updates applied,
36 files imports rewritten, ESLint → Oxlint, Oxfmt configured
- vp install --no-frozen-lockfile: clean
- vp run format: passes (269 files)
- vp run lint: runs cleanly through the sanitized config (real
tsconfig findings in upstream playground code, allowed via `|| true`)
Removes the `vite-plugin-vue` entry (and undoes the earlier `zustand`
attempt). Neither was a clean fit:
- `zustand` is CJS (no `"type": "module"`), so the migrated `vite.config.ts`
with `import` syntax fails to load in `vp lint` / `vp fmt`.
- `vite-plugin-vue` runs end-to-end, but `vp run lint` surfaces 200+
pre-existing project lint debt + 17 `vite-plus(prefer-vite-plus-imports)`
errors from a `rewriteAllImports` gap (type-only imports aren't
rewritten). The `|| true` masks both, so the entry doesn't actually
gate a regression.
Re-adding a good e2e candidate needs separate fixes first:
- rewriteAllImports needs to cover `import type` and test-helper
paths (so the prefer-vite-plus-imports findings are real signal,
not migration leakage).
- The CJS-project loader path needs to handle the migrated config
(so we can also cover zustand-shaped projects).
2 participants
After
vp migrateremovedeslint, the rest of the ESLint ecosystem stuck around inpackage.json. This expands the cleanup, tightens its scope, and adds a safety valve for framework integrations that can't be migrated cleanly today.Skipped entirely
If
@nuxt/eslintis detected anywhere inpackage.json(root or workspace package), the ESLint migration is skipped with a warning —eslint,eslint.config.mjs, plugins, and thelintscript are all preserved verbatim. The user is told to migrate manually when they're ready.Why:
@nuxt/eslintintegrates ESLint via a Nuxt module that the auto-migration cannot reproduce cleanly (the result leavesnuxt.config.tsbroken andvite.config.tsreferencing the removed@nuxt/eslint-plugin). Verified end-to-end against why-reproductions-are-required/vp-migrate-nuxt-eslint.Removed at root and at every workspace package
The cleanup is applied uniformly — same predicate, same eslint-config deletion, same lint-staged rewrite — at the root and at every workspace package detected via
pnpm-workspace.yaml/workspaces.eslint,typescript-eslinteslint-plugin-*,eslint-config-*,eslint-formatter-*@typescript-eslint/eslint-plugin,@typescript-eslint/parser,@typescript-eslint/rule-tester@eslint/*,@eslint-community/*,@angular-eslint/*@vue/eslint-config-typescript,@nuxt/eslint-config,@stylistic/eslint-plugin*, etc.eslintrc,eslint-utils,eslint-visitor-keys,eslint-scope,eslint-define-config,eslint-doc-generator@types/<X>whenever<X>is removedPer-workspace cleanup also: deletes the workspace package's own
eslint.config.*/.eslintrc.*if present, and rewriteseslintreferences in the workspace package's.lintstagedrc.json.Preserved
@typescript-eslint/utils,/typescript-estree,/scope-manager,/types— reusable AST libraries codemods and doc generators import directly.Other changes
peerDependenciesandoptionalDependenciestoo, matching the vite/vitest peer cleanup atmigrator.ts:1761."devDependencies": {}noise).fs.existsSync(package.json)so a transient missing file doesn't abort the whole migration.Note on workspace shared-config packages: if a workspace package publishes a shared ESLint preset that you want to keep intact, exclude it from
pnpm-workspace.yaml/workspacesbefore runningvp migrate, then add it back afterwards. There's no per-package opt-out flag.Test plan
migrator.spec.ts(cleanup variations +@nuxt/eslintdetection)migration-eslint-plugins-cleanup— wider matrix end-to-end (scopes, formatters,@types, preserved@typescript-eslint/utils, peereslint)migration-eslint-nuxt-skip— verifies the skip path leaves ESLint state untouchedmigration-eslint-monorepo-plugins-in-packages— workspace cleanup with per-packageeslint.config.mjs,.lintstagedrc.json, and a published-shared-config-stylepackages/lint-config/migration-eslint-monorepo/snap.txtupdates (emptydevDependencies: {}removed)vp test --run→ 431 passedvp check --fixcleanDeferred
detectEslintProjectonly fires on literaleslint— projects with onlytypescript-eslintnever trigger migration (pre-existing).--rule '@typescript-eslint/...'flags in scripts after plugin removal (needs Rust ast-grep updates).vp migrate.Addresses WeakAuras/WeakAuras-Companion#2956 review comments r3255784734, r3255786019, r3255859061, plus follow-ups from two rounds of in-PR code review.