refactor(cli): proxy dev and build to Vite - #3380
james-elicx wants to merge 27 commits into
Conversation
@vinext/cloudflare
create-vinext-app
@vinext/types
vinext
@cloudflare/workers-response-store
commit: |
|
/bigbonk review for issues\n\nTime limit: 8 minutes. Complete the review and post all findings within that limit. |
|
|
Posted 1 actionable inline finding. |
4597370 to
dd4f40d
Compare
|
/bigbonk review for issues\n\nTime limit: 8 minutes. Complete the review and post all findings within that limit. |
|
Posted 2 actionable inline findings. |
dd4f40d to
b422b40
Compare
|
/bigbonk review for issues Time limit: 8 minutes. Complete the review and post all findings within that limit. |
|
Posted 1 actionable inline finding. |
b422b40 to
11dbc58
Compare
|
/bigbonk review for issues\n\nTime limit: 8 minutes. Complete the review and post all findings within that limit. |
|
Posted 1 actionable inline finding. |
e75c601 to
2b85ee3
Compare
|
/bigbonk review for issues\n\nTime limit: 8 minutes. Complete the review and post all findings within that limit. |
2b85ee3 to
4473a51
Compare
|
/bigbonk review for issues\n\nTime limit: 8 minutes. Complete the review and post all findings within that limit. |
|
Posted 1 actionable inline finding. |
3b63168 to
decd15e
Compare
|
/bigbonk review for issues Time limit: 8 minutes. Complete the review and post all findings within that limit. |
| const root = configPreflight(command); | ||
| const cliPath = resolveProjectViteCli(root); | ||
| process.argv = [process.execPath, cliPath, command, ...rawArgs]; | ||
| await import(/* @vite-ignore */ pathToFileURL(cliPath).href); |
There was a problem hiding this comment.
[P2] Load dotenv before every config dependency evaluates
Delegating here means the replacement dotenv bootstrap in index.ts cannot run until the config imports vinext. ESM evaluates earlier static dependencies first, so a config such as import "./env-reader.mjs"; import vinext from "vinext" now lets env-reader.mjs observe .env values as missing (and can fail before vinext is imported). The previous CLI called loadDotenv() before loading Vite or any config module, and the new test only covers a config that imports vinext first. Please restore pre-config dotenv loading for the parsed root/mode, or cover static config dependencies with an equivalent bootstrap.
There was a problem hiding this comment.
Confirmed: an ESM dependency of vite.config evaluates before the vinext plugin hook regardless of import order. Restoring wrapper-only pre-config dotenv would make vinext differ from direct vite, contrary to the required thin-proxy contract. The top adoption PR now explicitly documents the limitation and use of loadEnv in a config factory; I am leaving this thread unresolved as a deliberate compatibility decision for maintainer review.
|
Posted 1 actionable inline finding. |
Summary
vinext devandvinext buildorchestration with in-process project-local Vite CLI delegationDeliberate interface changes
Configless use now asks users to run
vinext init. Vinext-only build flags move to vinext plugin configuration; shared Vite flags pass through unchanged.Two pre-config wrapper behaviors cannot be preserved while these commands remain thin Vite proxies and direct Vite behaves identically:
vinext initmigration. The alias no longer edits package/config files before Vite loads them..envbefore evaluation. Config-time values should be loaded explicitly with ViteloadEnv(mode, process.cwd(), ""); see the README for customenvDirguidance.Both compatibility tradeoffs have open review threads for maintainer decision. They are not claimed as behavior-preserving.
Validation
vp checkvp run vinext#buildPR 6 of 7.