refactor: modularize vite.config.ts and fix Windows script compatibility#379
refactor: modularize vite.config.ts and fix Windows script compatibility#379facusturla wants to merge 2 commits intokoala73:mainfrom
Conversation
|
@facusturla is attempting to deploy a commit to the Elie Team on Vercel. A member of the Team first needs to authorize it. |
Code ReviewBLOCKING1. 100 2. In the new import { fileURLToPath } from 'url';
const __dirname = fileURLToPath(new URL('.', import.meta.url));Or simply use the default 3.
SUGGESTIONS4. Indentation style changed — Original 5. 6. "build:full": "cross-env-shell VITE_VARIANT=full \"tsc && vite build\""NITPICK
Verdict: Request Changes — The lock file peer churn (#1) is a deployment risk and the |
545bd31 to
66b8095
Compare
|
Thanks for the thorough review! I've addressed all the feedback: BLOCKING fixes:
SUGGESTION fixes: Converted all vite/*.ts files from 4-space to 2-space to match project convention. Exported a VariantMeta interface from variants.tsand imported it in plugins.ts. Replaced double cross-env calls with cross-env-shell for all chained scripts. Verified locally: npm run build passes (tsc + vite build in ~40s). |
koala73
left a comment
There was a problem hiding this comment.
PR Review: Modularize vite.config.ts + Windows script compatibility
Overall
Good direction — splitting a 690-line config into focused modules improves readability. cross-env for Windows compat is the right tool. Several issues need addressing before merge.
BLOCKING
1. RSS_PROXY_ALLOWED_DOMAINS in vite/plugins.ts is ~62 domains behind api/rss-proxy.js
The dev-server RSS proxy plugin has its own hardcoded domain allowlist that has drifted massively from the production source of truth (api/rss-proxy.js). Missing domains include all euronews locales, Indian/Greek/LatAm/African regional sources, many international news outlets added over recent months, and happy-variant sources like dailygood.org, goodgoodgood.co, etc.
Also adds www.huffpost.com which doesn't exist in production.
This creates a situation where feeds work in production but fail silently in dev (403 from the proxy), making debugging painful.
Suggested fix: Import the domain list from a shared constant, or — simpler — just import/re-use from api/rss-proxy.js. Having two copies guarantees drift.
2. sebufApiPlugin path resolution changed from relative imports to resolve(serverRoot, ...)
Current code uses:
import('./server/router')
import('./src/generated/server/worldmonitor/seismology/v1/service_server')PR changes to:
import(resolve(serverRoot, 'server/router'))
import(resolve(serverRoot, 'src/generated/server/worldmonitor/seismology/v1/service_server'))Where serverRoot = fileURLToPath(new URL('.', import.meta.url)) which resolves to the project root. These should be equivalent, but dynamic import() with resolve() paths bypasses Vite's module graph — meaning HMR invalidation of server modules may not work correctly. The existing file watcher (server.watcher.on('change', ...)) nullifies cachedRouter, but Vite won't track the import dependencies. This is a functional regression risk for dev experience.
3. __dirname polyfill is unnecessary and subtly wrong
const __dirname = fileURLToPath(new URL('.', import.meta.url));Vite already provides __dirname in config files via its CJS compatibility layer. The existing code on main uses __dirname directly without any polyfill (lines 815-845). This shadows the built-in with a manually constructed version. While the value should be identical, it's unnecessary complexity.
SUGGESTIONS
4. cross-env-shell quoting may break on some Windows shells
Scripts like:
"build:full": "cross-env-shell VITE_VARIANT=full \"tsc && vite build\""The nested escaped quotes work in most cases but are fragile with some Windows terminal emulators. Consider testing on actual Windows (cmd.exe, PowerShell, Git Bash).
5. Missing newline at end of package.json
The diff shows \ No newline at end of file — the trailing newline was removed. This causes unnecessary git noise on the next edit.
6. Comments stripped from vite.config.ts
Several useful comments were removed during the refactor (e.g., "Geospatial bundles are expected to be large", "onnxruntime-web ships a minified browser bundle", "Give lazy-loaded locale chunks a recognizable prefix"). These explain non-obvious decisions. The manualChunks one-liners are less readable without them.
7. process.env.FRED_API_KEY in vite/proxy.ts leaks server env into proxy config
The FRED proxy rewrite function reads process.env.FRED_API_KEY directly. This works but means the proxy config function is impure (depends on env state at call time). Consider passing it as a parameter or documenting the dependency.
Summary
The modularization structure is good. The main blocker is the RSS domain list drift (62+ missing domains) — this will cause dev/prod parity issues. The sebufApiPlugin path resolution change needs testing to confirm HMR still works. Fix those two and this is ready to merge.
…malize indentation, type activeMeta, use cross-env-shell
…ths, remove __dirname polyfill - Extract RSS allowed domains into shared api/rss-allowed-domains.js (single source of truth for both prod and dev proxy) - Revert sebufApiPlugin to relative import() paths for Vite module graph compat - Remove unnecessary __dirname polyfill (Vite provides it natively) - Restore explanatory comments in manualChunks/onwarn - Document FRED_API_KEY env dependency in proxy.ts - Restore trailing newline in package.json
5ae50dd to
635722f
Compare
Addresses PR review feedbackBlocking fixes1. RSS domain drift resolved — Created Suggestions addressed4. cross-env-shell quoting — The nested escaped quotes
Rebase noteRebased onto latest |
This PR addresses two maintenance and developer experience issues:
package.jsonwithcross-env.npm run dev:techandnpm run build:financework correctly on Windows PowerShell/CMD.vite.config.tsinto a modular structure under a newvite/directory.vite/variants.ts: Site variant metadata.vite/proxy.ts: Proxy rules for APIs and RSS feeds.vite/plugins.ts: Custom Vite plugins (Sebuf, RSS proxy, etc.).Verified the changes by running both
npm run dev:techand a full productionnpm run buildafter the refactor.