feat(migrate): preserve dynamic Oxlint and Oxfmt configs - #2483
Conversation
`vp migrate` detected and inlined only the JSON forms of the Oxlint and Oxfmt configs, so a project using `oxlint.config.ts` or `oxfmt.config.mjs` silently lost its configuration. Detection now covers the dynamic `.ts`/`.mts`/`.cts`/`.js`/`.mjs`/`.cjs` forms, and the existing tsdown merger is generalized to target `lint` and `fmt` so those files are preserved and imported into `vite.config.*` the same way tsdown configs already are. Their bare `oxlint` and `oxfmt` runtime imports are rewritten to complete `vite-plus/lint` and `vite-plus/fmt` subpaths, which migration otherwise leaves unresolvable under strict pnpm layouts once the direct packages are removed. Closes voidzero-dev#2430
…-config-migration
✅ Deploy Preview for viteplus-preview canceled.
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3853a39fd5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Reuse an existing default import when a partially migrated config | ||
| // already has one. Otherwise prepend it using JavaScript extensions for | ||
| // TypeScript source files, matching TypeScript module resolution. | ||
| let import_prefix = format!("import {import_name} from"); | ||
| if !vite_config_content.contains(&import_prefix) { |
There was a problem hiding this comment.
Choose a collision-free name for the injected config import
If vite.config.ts already declares oxlintConfig or oxfmtConfig without the exact text import <name> from—for example a local constant, named import alias, type-only import, or multiline default import—this branch prepends another binding with the same name and leaves the migrated config syntactically invalid. Detect bindings through the AST and either reuse a compatible import or generate a unique local identifier before inserting the config property.
Useful? React with 👍 / 👎.
| mergeDynamicConfigFile( | ||
| projectPath, | ||
| viteConfig, | ||
| configs.oxlintConfig, | ||
| 'oxlintConfig', |
There was a problem hiding this comment.
Retain type-check defaults for dynamic lint configs
When oxlint.config.ts does not explicitly set options.typeAware or options.typeCheck, this branch inserts lint: oxlintConfig; the subsequent injectLintTypeCheckDefaults() sees that top-level key and returns without adding either default. This contradicts the migration behavior applied to JSON and config-less projects and silently makes vp check stop performing the type-aware lint and TypeScript checks that vp migrate enables by default. Merge the missing option defaults around the imported config while preserving explicit values and the baseUrl exception.
AGENTS.md reference: AGENTS.md:L185-L185
Useful? React with 👍 / 👎.
| configs.oxlintConfig, | ||
| 'oxlintConfig', | ||
| 'lint', | ||
| silent, | ||
| report, |
There was a problem hiding this comment.
Add Svelte rune globals to dynamic lint configs
When a dynamic Oxlint config has an override targeting .svelte files but omits explicit rune globals, this path bypasses ensureSvelteRuneGlobals(), which is still applied to migrated JSON configs to prevent $state, $derived, and the other built-in runes from being reported as undefined. Importing the object unchanged therefore introduces false-positive lint failures specifically for Svelte projects using the newly supported config format; apply the same runtime normalization before assigning the imported config.
AGENTS.md reference: AGENTS.md:L185-L185
Useful? React with 👍 / 👎.
| mergeDynamicConfigFile( | ||
| projectPath, | ||
| viteConfig, | ||
| configs.oxlintConfig, | ||
| 'oxlintConfig', |
There was a problem hiding this comment.
Rewrite imported configs even when gitignored
If a generated or machine-local oxlint.config.ts/oxfmt.config.ts is listed in .gitignore, detectConfigs() still selects it and this branch imports it into vite.config.ts, but the later rewriteAllImports() walk respects gitignore and never changes its bare oxlint/oxfmt helper import. Since migration removes those direct dependencies, strict pnpm/Yarn layouts then fail while loading the newly imported config. Rewrite the detected file directly regardless of ignore rules, or avoid removing its dependency when it was skipped.
Useful? React with 👍 / 👎.
|
I was about to pick up #2430, found this PR already does it (nice!), and had a few empirical probes lying around that seem useful as review input. All verified against the bundled oxlint 1.79.0 / oxfmt 0.64.0 binaries on this repo's checkout. 1. PR description vs code: extension list. The description says detection covers 2. Post-migration shadowing (may deserve an info line in the summary). With both Right after this migration the two are consistent because 3. Edge: both |
|
Thanks — these are useful, and (1) was a real error in the description rather than the code. 1. Confirmed and fixed. 2. and 3. Both read as genuine, and I'd rather a maintainer decide whether they belong in this PR or a follow-up, since each widens the scope past what #2430 asked for. Happy to add either on request:
Worth noting for (3) that oxlint hard-errors on that combination, so any project hitting it is already broken before migration — the warning would make the outcome legible rather than change it. |
|
(not a maintainer, but the person who created the linked issue) in the cases of 2 and 3, just my two cents of "what makes the most sense":
|
…imports The Oxc rewrite pass was applied unconditionally, unlike the vite, vitest and tsdown passes, which each honor the package-level `SkipPackages` flags. A workspace package that intentionally declares `oxlint` or `oxfmt` as its own runtime or peer dependency therefore had its library sources rewritten to `vite-plus/lint` / `vite-plus/fmt` even though `rewritePackageJson` keeps that dependency, so published consumers could receive an undeclared import. Split the combined Oxc rule set into separate oxlint and oxfmt rule sets and gate each on its own skip flag, computed from `peerDependencies` and `dependencies` exactly like the existing three. The fast pre-filter honors the same flags so a fully skipped package short-circuits before parsing. Projects that do not declare either package are unaffected.
|
Pushed Fixed — P1 "Preserve declared Oxc package boundaries during rewrites"Confirmed, and it is this PR's regression. The vite, vitest and tsdown passes in The fix splits
Confirmed, not fixed here — P1 "Retain type-check defaults for dynamic lint configs" and P2 "Add Svelte rune globals"Both are real, and they are two symptoms of one gap rather than two independent bugs. The common cause: everything the JSON branch does, it does by mutating a parsed object before writing it back. When the config is an opaque imported binding there is nothing to mutate, so this can't be fixed by "merging the missing defaults around" the import — it needs either generated wrapper code at the call site, or an explicit warning that the defaults were not applied. That's a design choice with user-visible output either way, so I'd rather you pick. Happy to implement whichever, in this PR or a follow-up. Confirmed but pre-existing — P2 "Choose a collision-free name for the injected config import"Real: Worth noting it is not introduced here: the same function already backs Unverified — P2 "Rewrite imported configs even when gitignored"Half-confirmed only. Validation limitsThis sandbox can't complete @KTrain5169 — thanks, that's exactly the steer I was asking for. On (2): agreed on the stronger wording and on applying it to On (3): agreed, and I'll leave it out. Your reasoning matches what I found — projects in that state are already broken because oxlint hard-errors on it — and surveying how other migrators handle the case is genuinely outside this issue. Generated by Claude Code |
|
Both dispositions look right from where I sit — (1) matches the code now, and leaving (3) out is consistent with what I reproduced (oxlint hard-errors on that combination before migration ever sees it, so the warning would only narrate an already-broken state). On (2): once a maintainer settles the wording, if the snapshot round trip is the blocker — I have a local checkout that runs the PTY suite fine, so I'm happy to run |
|
Thanks — and that offer is genuinely useful, since it removes the half of the blocker I can't solve in my environment. One clarification on what's actually left, though: the snapshot regeneration was the second gate, not the only one. The wording itself is still a maintainer call. @KTrain5169 proposed:
applied to So the order stands as: once a maintainer settles the wording, I'll push the info line and take you up on the Generated by Claude Code |
|
@fengmk2 ^ |
Agreed, interrupting with an error is more reasonable, let user to manually address config conflicts first. |
Oxlint refuses to run when a directory holds both a JSON config and a
dynamic one ("Only one of `.oxlintrc.json` and `oxlint.config.ts` is allowed
per directory"), so such a project cannot lint before migration either.
Migration had no non-destructive path through that state: first-match
detection inlined and deleted the JSON config while leaving the dynamic one
on disk unreferenced, where it then silently shadows the freshly inlined
`lint` block for direct `oxlint` invocations — the settings the user just
migrated stop applying, with no diagnostic.
Detect the ambiguity up front and interrupt instead, naming every directory
and the files that collide, so the user resolves the conflict before
anything is rewritten. The check runs before any file is touched and covers
the workspace root and every workspace package, on both the full-migration
and the already-Vite+ paths.
Two JSON forms are deliberately not treated as a conflict: the ambiguity
this guards against is JSON-vs-dynamic, which is the combination oxlint
itself rejects and the one that orphans a config across migration.
|
Thanks — that settles (3), so I've implemented it. Pushed What it does
then exits 1 via Two scope calls, both easy to trim if you disagree
Verification
Still open
Generated by Claude Code |
|
Ran the offer against Also cross-checked your "no existing snapshot output changes" claim by running
[[case]]
name = "migration_oxc_config_conflict"
vp = "global"
steps = [
{ argv = ["vp", "migrate", "--no-interactive"], comment = "migration should refuse to start on conflicting Oxc configs", continue-on-failure = true },
{ argv = ["vpt", "stat-file", ".oxlintrc.json", "--assert", "file"], comment = "both configs left untouched by the interrupt", continue-on-failure = true },
{ argv = ["vpt", "stat-file", "oxlint.config.ts", "--assert", "file"], continue-on-failure = true },
{ argv = ["vpt", "stat-file", "vite.config.ts", "--assert", "missing"], comment = "no file was written before the interrupt", continue-on-failure = true },
{ argv = ["vpt", "print-file", "package.json"], comment = "package.json unchanged", continue-on-failure = true },
]
{
"name": "migration-oxc-config-conflict",
"scripts": {
"lint": "oxlint"
},
"devDependencies": {
"oxlint": "^1.0.0",
"vite": "^7.0.0"
}
}
{
"rules": {
"no-console": "error"
}
}
import { defineConfig } from 'oxlint';
export default defineConfig({
rules: {
eqeqeq: 'error',
},
});
# migration_oxc_config_conflict
## `vp migrate --no-interactive`
migration should refuse to start on conflicting Oxc configs
**Exit code:** 1
```
VITE+ - The Unified Toolchain for the Web
✘ Conflicting Oxc configs:
- the project root has `.oxlintrc.json` and `oxlint.config.ts` — oxlint allows only one config per directory.
Keep a single config per directory, then run `vp migrate` again.
```
## `vpt stat-file .oxlintrc.json --assert file`
both configs left untouched by the interrupt
```
.oxlintrc.json: file
```
## `vpt stat-file oxlint.config.ts --assert file`
```
oxlint.config.ts: file
```
## `vpt stat-file vite.config.ts --assert missing`
no file was written before the interrupt
```
vite.config.ts: missing
```
## `vpt print-file package.json`
package.json unchanged
```
{
"name": "migration-oxc-config-conflict",
"scripts": {
"lint": "oxlint"
},
"devDependencies": {
"oxlint": "^1.0.0",
"vite": "^7.0.0"
}
}
```The steps assert the "before any file is touched" placement, not just the message: both configs still present, no |
…pshot Assert the fail-fast path end to end: `vp migrate` exits 1 naming the offending directory, and — the part that matters more than the message — nothing has been written when it does. The steps check both configs are still on disk, no `vite.config.ts` was created, and `package.json` is byte-identical, so a regression that moved the check after the first write would fail here even if the error text stayed the same. Recorded on macOS arm64 by simulacre7, who also confirmed the existing `migration_dynamic_oxc_configs` case still passes unchanged on this branch. Co-authored-by: simulacre7 <16968090+simulacre7@users.noreply.github.com>
|
@simulacre7 thank you — landed as I didn't take the snapshot on trust, since I can't re-run the recorder here. What I did check before committing it:
Also worth saying explicitly, because it's the reason your step choice is the right one: the placement your steps assert is real, not incidental. The one thing I could not do is run the PTY suite (no built Still open and unchanged by this commit: (2)'s wording needs @fengmk2's call, and the oxfmt scope question from my last comment (whether the guard stays oxlint-only until someone can confirm oxfmt's own multi-config behaviour — deleting the Generated by Claude Code |
|
Closing the oxfmt scope question with the bundled binary (oxfmt 0.63.0 from this branch's Control with only Snapshot side of |
The conflict guard fired only when a directory held a JSON *and* a dynamic config for the same tool. Running the pinned binaries shows the rule both tools enforce is one config per directory, not one config form: oxlint 1.78.0 and oxfmt 0.63.0 each fail with "Both '<a>' and '<b>' found in <dir>" for `.oxlintrc.json` + `.oxlintrc.jsonc` and for `oxlint.config.ts` + `oxlint.config.mts`, exactly as they do for the mixed pair. Both of those shapes previously migrated through, consuming one config and leaving the other on disk unreferenced. Detection now flags any tool with more than one config present, and the conflict carries a single `configs` list in the tool's candidate order instead of the JSON/dynamic split. The rendered line is unchanged for the two-config case and gains comma separation beyond it.
|
@simulacre7 that settles oxfmt — thank you. I reproduced it here against the same pinned pair ( But running the matrix rather than just your one case turned up a real gap in my own guard, so I've pushed I had written, in the comment above, that two JSON forms are deliberately not a conflict, on the grounds that "erroring on
So the rule is one config per directory, not one config form — and the two shapes the old predicate let through are exactly the ones migration would then walk into: first-match detection consumes one config and leaves the other on disk unreferenced. Same failure mode the guard exists to prevent, just reached by a different pair. What changed
Verification
Message rendering is unchanged for the two-config case (so the snapshot stands as recorded) and gains comma separation once a directory holds three or more: Still not run here: the PTY suite, for the same reason as before — no built Unchanged and still needing @fengmk2: (2)'s wording, the stronger "manually merge" line. That's the only open item left on this PR from my side. Generated by Claude Code |
|
PTY suite result for Full detail for honesty: the parallel run finished 151/154, with 3 failures whose names changed between runs ( |
|
@simulacre7 that closes the last gap on my side — thank you. The PTY suite was the one check I'd flagged as not runnable in my sandbox, so Two things I could verify from this end that make the result a bit stronger than a bare pass: The 154 is the complete migration set. Counting The fixture that flaked isn't structurally unusual. If that non-deterministic set ever turns up on Unchanged: (2)'s wording is the only item still open from my side, and it's a maintainer call rather than anything blocked on code or snapshots. Generated by Claude Code |
|
Followed up on the flake question: it reproduces on |
Closes #2430
Problem
vp migratedetected and inlined only the JSON forms of the Oxlint and Oxfmt configs. A project configured withoxlint.config.tsoroxfmt.config.mtshad its configuration silently dropped — the file stayed on disk but nothing referenced it after migration.Changes
.ts/.mtsforms alongside the JSON ones, matching the candidate lists the bundledoxlintandoxfmtbinaries themselves search for.lintandfmt, so dynamic configs are preserved and imported intovite.config.*the same way dynamic tsdown configs already are.oxlintandoxfmtruntime imports are rewritten to completevite-plus/lintandvite-plus/fmtsubpaths.That last part is what makes the rest usable. Migration removes the direct
oxlint/oxfmtdependencies, so under a strict pnpm layout a preserved config that still imports them bare fails to resolve. This is the same failure #701 hit in its e2e run after merge. The new public subpaths and the migration snapshot cover it.Testing
cargo test -p vp_migration— 315 passedvitest run packages/cli/src/migration packages/cli/src/__tests__/exports-map.spec.ts— 409 passed across 11 filesmigration_dynamic_oxc_configs, including rerun idempotencycargo clippywith warnings denied, Rust and JS formatting, and targeted type-aware lint/type checksRe-verified green after merging current
main, which had moved 8 commits since the branch was cut.AI assistance
Claude Opus 5 wrote the implementation, the tests, and this description. The change is agent-authored and has not had a separate human review. The test results quoted above are from actual runs on this branch, not estimates.