Repository navigation
fix(migrate): warn on esbuild-specific config that fails under Vite 8 - #3178
john-okeefe wants to merge 7 commits into
Conversation
|
Could we extend this into a targeted config migration? For a static #2004 scopes import rewrites to symbols that I’d use the existing AST analysis to identify the config property and preserve comments. The regex can match comments or unrelated plugin options, and it misses quoted property names. For dynamic configs, esbuild-specific options, or We should also adjust the warning: Vite 8 still supports |
Vite 8 no longer bundles esbuild, so a project migrated from Vite <= 7 with build.minify: 'esbuild' fails its next vp build on the deprecated transformWithEsbuild path (esbuild is no longer a vite dependency), and direct transformWithEsbuild usage has no vite-plus equivalent. The error names neither the config file nor the option, and migrate gives no heads-up. Add warnEsbuildConfigInViteConfig and call it wherever migrate finalizes a project's vite config (standalone, monorepo root + packages, per-project rewrite, and the existing-vite+ finalize pass). It emits one migration warning per detected pattern, pointing at the oxc replacements (minify: true, transformWithOxc). Like the rest of migrate it does not rewrite config semantics. Closes voidzero-dev#3176
Review feedback on voidzero-dev#3178: extend the diagnostic into a targeted config migration, use AST analysis instead of a regex, and acknowledge that Vite 8 still supports esbuild when it is installed separately. - build.minify: 'esbuild' inside a default-exported config is rewritten in place to minify: true (the oxc minifier) via span replacement: comments and unrelated plugin options are untouched, quoted keys ('minify') are handled, and the switch is reported through the success log plus a new esbuildMinifyMigrationCount summary counter. - Dynamic minify values, minify options outside the build object, and transformWithEsbuild usage keep their code and get a diagnostic that presents both options: install esbuild to keep the behavior, or migrate to oxc. - Detection walks the serialized oxc AST (analyzeMigrationSource) and covers defineConfig() calls, env-config arrays, TS wrappers, and quoted keys; raw-text scanning is now only a diagnostic fallback and skips comment ranges. - Warning wording no longer implies esbuild support is gone, and the rewrite prefers minify: true over removing the property (removal can disable minification for SSR builds).
e038243 to
f9be853
Compare
|
Thanks @fengmk2 — all four points are addressed in f9be853:
Tests cover the rewrite (incl. comment preservation), quoted keys, unrelated options, dynamic values, |
Audit pass over the two esbuild-migration commits: - finalizeCoreMigrationForExistingVitePlus tracks every migration's outcome in CoreMigrationFinalizationResult; the esbuild migration now does too (esbuildMinifyConfig flag), and the deep-equal result expectations in migrator.spec.ts are updated accordingly. - Helper placement: this file declares helpers before their consumers; the oxc AST helpers sat after migrateEsbuildConfigInViteConfig — moved above it. - Dropped the duplicate SpanLike interface (OxcSpan already describes the same shape). - Condensed the migrateEsbuildConfigInViteConfig JSDoc to the length used by sibling migrations.
|
Follow-up (7d41849): the migration is now also tracked in |
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d418492dc
ℹ️ 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".
| result.esbuildMinifyConfig = | ||
| migrateEsbuildConfigInViteConfig(projectPath, silent, report) || result.esbuildMinifyConfig; |
There was a problem hiding this comment.
When an already-on-Vite+ project has no other pending work and this is the only rewrite, this function returns esbuildMinifyConfig: true, but the didMigrate predicate in packages/cli/src/migration/bin.ts:1161-1168 checks only through taskCacheConfig. The early exit at bin.ts:1203-1222 then reports that the project is already using Vite+ and skips the migration summary/formatting even though vite.config was modified; include this new result flag in that predicate.
Useful? React with 👍 / 👎.
| default: | ||
| return; |
There was a problem hiding this comment.
When a supported config uses a callback such as defineConfig(({ mode }) => ({ build: { minify: mode === 'legacy' ? 'esbuild' : true } })), the call arm passes the ArrowFunctionExpression here and this default branch drops it. The fallback regex only matches a quote immediately after minify:, so this production-only esbuild selection emits neither a rewrite nor a warning and that Vite 8 build still fails; traverse direct callback return objects before falling back.
Useful? React with 👍 / 👎.
| if (key.type === 'Identifier' && typeof key.name === 'string') { | ||
| return key.name; | ||
| } |
There was a problem hiding this comment.
When a config has a computed key such as const build = 'plugin'; export default { [build]: { minify: 'esbuild' } }, ESTree still gives the key an Identifier named build, so this helper classifies it as Vite's literal build key and later rewrites the unrelated string to true. Check prop.computed before accepting identifier keys (while still allowing static string-literal computed keys) so migration does not alter non-Vite options.
Useful? React with 👍 / 👎.
finalizeCoreMigrationForExistingVitePlus records the build.minify 'esbuild' -> true switch in esbuildMinifyConfig, but the didMigrate predicate never read it. On an otherwise up-to-date existing Vite+ project the static rewrite emits no warnings, so the 'already using Vite+' fast exit swallowed the change: the summary, the formatting pass, and any mention of the switch were skipped even though vite.config was rewritten on disk. Include the flag in the predicate so the rewrite surfaces like every other config update. Covered by a new CLI snapshot fixture that reintroduces minify: 'esbuild' on a converged project (fails without the fix) and a unit test asserting the flag and its convergence.
…he esbuild config check
Two static-analysis gaps in migrateEsbuildConfigInViteConfig:
- staticConfigObjects dropped function bodies, so a production-only
esbuild selection inside a callback — defineConfig(({ mode }) => ({
build: { minify: mode === 'legacy' ? 'esbuild' : true } })
— produced neither a rewrite nor a warning, and the Vite 8 build
failed silently. Config factories now expose the objects built by
their direct returns; expression bodies directly, block bodies via
top-level return statements. Returns nested in conditionals stay
with the raw-occurrence fallback diagnostics.
- propertyName matched computed identifier keys, so
const build = 'plugin' + export default { [build]: { minify:
'esbuild' } } was misread as Vite's build option and the unrelated
string was rewritten to true. Computed identifiers no longer match;
computed string literals (['build']) still do, since they are the
same key.
The raw-occurrence diagnostic still covers everything the static walk
does not account for, so both gaps now warn instead of staying
silent.
|
All three review findings are addressed:
Validation: 377/377 unit tests, |
|
To use Codex here, create a Codex account and connect to github. |
Problem
vp migraterewrites the imports vite-plus owns but leaves config semantics untouched (per the scoped-rewrite policy in #2004). One leftover is not a silent behavioral difference but a guaranteed build failure: Vite 8 no longer bundles esbuild, so a project migrating from Vite ≤ 7 withfails on its very next
vp buildwith:The error names neither
minifynor the config file, and nothing in the migration output flags it. Repro + full details in #3176.What this does (updated after review)
Per @fengmk2's review, this now performs a targeted config migration rather than only warning:
build.minify: 'esbuild'inside thebuildobject of a default-exported config is rewritten in place tominify: truevia span replacement over the serialized oxc AST (analyzeMigrationSource). Comments and unrelated plugin options are preserved, quoted keys ('minify') are handled, and the switch is reported through the success log plus a newesbuildMinifyMigrationCountin the migration summary. Rewriting (rather than removing the property) keeps minification enabled for SSR builds.esbuildseparately to keep esbuild behavior, or migrate to oxc — for: dynamic minify values,minifyoptions outside thebuildobject, andtransformWithEsbuildusage (which may need argument changes). Vite 8 still supports esbuild-specific config when esbuild is installed separately, so the diagnostics no longer imply support is gone.defineConfig({...}),defineConfig([{...}, ...])env-config arrays, TS wrappers (as/satisfies), and quoted keys. Raw-text scanning exists only to feed diagnostics for shapes the static walk can't see (spread configs, helper-built objects), and it skips comment ranges.Where it hooks in
Called everywhere migrate finalizes a project's vite config, mirroring
migrateTaskCacheConfigInViteConfig:rewriteStandaloneProjectrewriteMonorepo(workspace root + each package)rewriteMonorepoProjectfinalizeCoreMigrationForExistingVitePlus(already-on-vite+ projects running the finalize pass)Test plan
New tests in
migrator.spec.tscover: the static rewrite (including comment preservation), quoted keys, unrelatedminifyoptions left alone with a diagnostic, dynamic values diagnosed without rewriting,transformWithEsbuilddiagnostics, comment-only mentions ignored, oxc-compatible configs untouched, and the missing-config no-op.pnpm exec vitest run src/migration/__tests__/migrator.spec.ts— 370 passed (362 pre-existing + 8 new).Closes #3176