feat(build): run lifecycle through Vite - #3377
james-elicx wants to merge 19 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. |
|
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
Posted 3 actionable inline findings. |
078c7d0 to
ab7311c
Compare
|
/bigbonk review for issues\n\nTime limit: 8 minutes. Complete the review and post all findings within that limit. |
|
Posted 3 actionable inline findings. |
ab7311c to
86e2104
Compare
|
/bigbonk review for issues Time limit: 8 minutes. Complete the review and post all findings within that limit. |
|
Posted 1 actionable inline finding. Targeted tests pass after rebuilding |
|
/bigbonk review for issues\n\nTime limit: 8 minutes. Complete the review and post all findings within that limit. |
|
Posted 1 actionable inline finding. |
7f41ce5 to
9341cf4
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. |
9341cf4 to
ae580bb
Compare
|
/bigbonk review for issues Time limit: 8 minutes. Complete the review and post all findings within that limit. |
|
Posted 2 actionable inline findings. Targeted unit tests pass. |
|
@james-elicx Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues\n\nTime limit: 8 minutes. Complete the review and post all findings within that limit. |
|
/bigbonk review for issues Time limit: 8 minutes. Complete the review and post all findings within that limit. |
| if (process.env.NODE_ENV !== resolvedNodeEnv) { | ||
| const preserveTestNodeEnv = | ||
| process.env.NODE_ENV === "test" && | ||
| (mode === "test" || viteCliBuildConfigNodeEnv === "test"); |
There was a problem hiding this comment.
P1: This still preserves NODE_ENV=test solely because Vite's mode is test, even when there is no direct CLI invocation to justify config-time compatibility. The existing programmatic config contract now fails (tests/build-optimization.test.ts:946): calling this hook for { command: "build", mode: "test" } leaves process.env.NODE_ENV as test instead of switching the actual build to production. CI's Vitest (unit 1/3) is failing on exactly that assertion. Please gate preservation on the captured direct-CLI test environment rather than mode === "test"; mode selects config/env files but should not select build-time NODE_ENV.
|
Posted 1 actionable inline finding. Targeted lifecycle tests pass; CI confirms the reported |
|
/bigbonk review for issues\n\nTime limit: 8 minutes. Complete the review and post all findings within that limit. |
| index++; | ||
| continue; | ||
| } | ||
| if (arg.startsWith("-")) continue; |
There was a problem hiding this comment.
P2: Treat -- as the end of option parsing instead of skipping it. CAC treats everything after this marker as positional input, so vite -- build starts the dev server with build as the project root (confirmed against the bundled Vite CLI), while this loop skips -- and classifies it as a production build. Because getViteCliInvocation() runs at module load, that valid dev invocation gets NODE_ENV=production and loads .env.production before its Vite config executes. Stop command detection at -- and preserve the remaining positional arguments for the default dev command.
|
Posted 1 actionable inline finding. Targeted unit tests pass. |
|
/bigbonk review for issues Time limit: 8 minutes. Complete the review and post all findings within that limit. |
| index++; | ||
| continue; | ||
| } | ||
| if (arg.startsWith("-")) continue; |
There was a problem hiding this comment.
P2: CAC also expands clustered short options, but this scanner skips the cluster wholesale. For example, the bundled Vite CLI accepts vite -dm staging build (-d plus -m staging) and selects the production build command, while commandArguments() skips -dm, sees staging as the first positional, and classifies the invocation as a dev server rooted there. That suppresses the vinext lifecycle and also loads the wrong config-time mode/env. Please account for CAC's short-option clustering (or derive command/mode from the authoritative parsed invocation) rather than treating every option-looking token as one flag.
|
Posted 1 actionable inline finding. Targeted CLI invocation tests pass. |
|
/bigbonk review for issues Time limit: 8 minutes. Complete the review and post all findings within that limit. |
|
No new actionable findings beyond the existing inline review comments. |
|
/bigbonk review for issues Time limit: 8 minutes. Complete the review and post all findings within that limit. |
|
/bigbonk review for issues Time limit: 8 minutes. Complete the review and post all findings within that limit. |
|
Posted 1 actionable inline finding. |
|
/bigbonk review for issues Time limit: 8 minutes. Complete the review and post all findings within that limit. |
|
No new actionable findings beyond the existing inline review comments. |
Summary
Validation
vp checkvp run vinext#buildPR 3 of 7.