feat(build): run lifecycle through Vite - #3377
james-elicx wants to merge 28 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 1 improved · 0 regressed · 5 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. |
|
/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. |
|
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. |
|
Posted 1 actionable inline finding. CI also reproduces the regression. |
|
/bigbonk review for issues Time limit: 8 minutes. Complete the review and post all findings within that limit. |
|
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. |
| if (!viteCliBuildConfigNodeEnv) { | ||
| Reflect.set(process.env, "NODE_ENV", "production"); | ||
| } | ||
| loadDotenv({ root: earlyViteCliInvocation.root, mode: earlyViteCliInvocation.mode }); |
There was a problem hiding this comment.
P2: This eager load runs before Vite resolves envDir, so it loads the CLI root even when the project config sets envDir: false or points at a different directory. Since loadDotenv() never overwrites existing keys, the later config hook cannot correct values loaded from the wrong directory; a workspace .env.production can therefore override the app env, and envDir: false no longer disables vinext dotenv loading for direct CLI builds. Please defer mutation until the resolved env policy is known, or undo/reload the eager values once config resolves.
There was a problem hiding this comment.
This is a deliberate pre-config compatibility boundary. Before this PR, vinext dev/build loaded root dotenv before evaluating vite.config, including when the eventual config set custom envDir or false; the migration contract explicitly preserves that behavior for both command spellings. Waiting for resolved envDir would break apps reading process.env while the config module is evaluated. The README now says custom envDir requires explicit Vite loadEnv to supply config-time values. I am leaving this thread visible for maintainer review of that tradeoff.
|
Posted 2 actionable inline findings. |
|
/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. |
| if (!viteCliBuildConfigNodeEnv) { | ||
| Reflect.set(process.env, "NODE_ENV", "production"); | ||
| } | ||
| loadDotenv({ root: earlyViteCliInvocation.root, mode: earlyViteCliInvocation.mode }); |
There was a problem hiding this comment.
P2: This module side effect still runs too late for dependencies imported before vinext from the Vite config. With the supported --configLoader runner, a config such as import "./config-helper"; import vinext from "vinext" evaluates the helper first, so any module-scope read of process.env there sees neither .env.production nor the production NODE_ENV; only evaluating the later vinext import performs this load. The previous vinext build path initialized the environment before Vite loaded the config graph. Please move direct-CLI initialization ahead of config module evaluation (rather than relying on the position of the vinext import), or otherwise preserve dotenv/NODE_ENV for the whole config dependency graph.
|
Posted 2 actionable inline findings. Targeted lifecycle and CLI tests pass. |
|
/bigbonk review for issues Time limit: 8 minutes. Complete the review and post all findings within that limit. |
|
LGTM! |
Summary
Compatibility boundary
Vite evaluates config imports before plugin hooks. A config dependency that reads dotenv or
NODE_ENVbefore thevinextimport cannot be initialized by this plugin; move that read behind explicit ViteloadEnvin an early config dependency. The open review threads track this tradeoff of using direct Vite without wrapper-side initialization.Validation
vp checkvp run vinext#buildPR 3 of 7.