Skip to content

refactor: modularize vite.config.ts and fix Windows script compatibility#379

Open
facusturla wants to merge 2 commits intokoala73:mainfrom
facusturla:refactor/modular-vite-windows-fix
Open

refactor: modularize vite.config.ts and fix Windows script compatibility#379
facusturla wants to merge 2 commits intokoala73:mainfrom
facusturla:refactor/modular-vite-windows-fix

Conversation

@facusturla
Copy link
Contributor

This PR addresses two maintenance and developer experience issues:

  1. Windows Script Compatibility (Fixes TODO-005):
    • Replaced direct environment variable assignments in package.json with cross-env.
    • This ensures scripts like npm run dev:tech and npm run build:finance work correctly on Windows PowerShell/CMD.
    • Verified locally on a Windows environment.
  2. Vite Configuration Modularization:
    • Refactored the monolithic 1,200-line vite.config.ts into a modular structure under a new vite/ directory.
    • Extracted logic into:
      • 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.).
    • This significantly improves readability and makes the build pipeline easier to maintain as the project grows.
      Verified the changes by running both npm run dev:tech and a full production npm run build after the refactor.

@vercel
Copy link

vercel bot commented Feb 25, 2026

@facusturla is attempting to deploy a commit to the Elie Team on Vercel.

A member of the Team first needs to authorize it.

@koala73
Copy link
Owner

koala73 commented Feb 26, 2026

Code Review

BLOCKING

1. package-lock.json has massive peer flag churn

100 "peer": true entries removed, 22 added. This flips install behavior for @amcharts, @arcgis, @deck.gl, and many others — packages previously not installed (peer-only) will now be installed as regular dependencies, bloating node_modules and potentially causing version conflicts. This is almost certainly an artifact of running npm install on a different npm version or without --legacy-peer-deps. The lock file should be regenerated cleanly on the same npm version used by CI.

2. const __dirname = resolve() is incorrect

In the new vite.config.ts, resolve() with no args returns process.cwd(), NOT the file's directory. This works accidentally when running from repo root but breaks if Vite is invoked from a subdirectory or by tools that change CWD. Should be:

import { fileURLToPath } from 'url';
const __dirname = fileURLToPath(new URL('.', import.meta.url));

Or simply use the default __dirname that Vite provides to config files.

3. proxyConfig(process) leaks Node global as any

vite/proxy.ts declares export const proxyConfig = (process: any) — accepting the global process as a parameter typed as any. This defeats type checking on process.env.FRED_API_KEY. Since Vite config files run in Node, process is already available globally. Just reference process.env directly in proxy.ts — no need to pass it.


SUGGESTIONS

4. Indentation style changed — Original vite.config.ts uses 2-space indentation. The new vite/*.ts files use 4-space. Should match the project convention (2 spaces).

5. activeMeta typed as anyhtmlVariantPlugin(activeVariant: string, activeMeta: any) should use the actual type from variants.ts instead of any.

6. cross-env called twice per chained script — Scripts like "build:full": "cross-env VITE_VARIANT=full tsc && cross-env VITE_VARIANT=full vite build" repeat cross-env because env doesn't persist across &&. Consider cross-env-shell instead:

"build:full": "cross-env-shell VITE_VARIANT=full \"tsc && vite build\""

NITPICK

  • PR bundles two unrelated concerns (Windows compat + config modularization). Ideally separate commits or PRs for cleaner history and easier revert.
  • The vite/ directory name is ambiguous with the vite npm package. Consider config/vite/ or build/.

Verdict: Request Changes — The lock file peer churn (#1) is a deployment risk and the __dirname issue (#2) is a latent bug. Fix those + the any typing and this is a clean refactor.

@facusturla facusturla force-pushed the refactor/modular-vite-windows-fix branch 2 times, most recently from 545bd31 to 66b8095 Compare February 26, 2026 13:31
@facusturla
Copy link
Contributor Author

Thanks for the thorough review! I've addressed all the feedback:

BLOCKING fixes:

  1. package-lock.json peer churn — I restored the lock file from main and re-ran npm install to only add the cross-env dependency. However, my local npm version (v11.6.2) still produces some peer flag differences compared to the upstream lock. If you'd prefer a completely clean lock, please regenerate it on your end with your CI npm version after merging the source changes.

  2. __dirname fix — Replaced resolve() with fileURLToPath(new URL('.', import.meta.url)).

  3. proxyConfig(process) leak — Removed the process: any parameter; process.env is accessed directly (it's a global in Node).

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).

Copy link
Owner

@koala73 koala73 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@facusturla facusturla force-pushed the refactor/modular-vite-windows-fix branch from 5ae50dd to 635722f Compare February 26, 2026 19:10
@facusturla
Copy link
Contributor Author

Addresses PR review feedback

Blocking fixes

1. RSS domain drift resolved — Created api/rss-allowed-domains.js as a shared
single source of truth. Both api/rss-proxy.js (production) and vite/plugins.ts
(dev proxy) now import from this file. ~62 missing domains restored, spurious
www.huffpost.com removed. Two copies can never drift again.
2. sebufApiPlugin path resolution reverted — Changed back from
import(resolve(serverRoot, '...')) to relative import('./...'), matching the
original main branch pattern. This restores Vite's module graph tracking so HMR
correctly invalidates server modules. The serverRoot parameter was removed from
the function signature.
3. __dirname polyfill removed — Removed the manual
const __dirname = fileURLToPath(new URL('.', import.meta.url)) and its
import { fileURLToPath } from 'url'. Vite provides __dirname natively in
config files via its CJS compatibility layer.

Suggestions addressed

4. cross-env-shell quoting — The nested escaped quotes
(cross-env-shell VITE_VARIANT=full \"tsc && vite build\") follow the
recommended pattern from the cross-env docs. Tested on Windows PowerShell
and cmd.exe. Git Bash also works since cross-env handles the normalization.
5. package.json trailing newline — Restored the missing \n at EOF.
6. Explanatory comments restored — Added back three comments that explain
non-obvious build decisions:

  • // Geospatial bundles are expected to be large even when split.
  • // onnxruntime-web ships a minified browser bundle that intentionally uses eval.
  • // Give lazy-loaded locale chunks a recognizable prefix so the service-worker can glob-ignore them.
    7. FRED_API_KEY env dependency documented — Added a note in vite/proxy.ts
    that FRED_API_KEY is read from process.env at proxy-creation time and must
    be set in .env before starting the dev server.

Rebase note

Rebased onto latest main (fe413fa). Conflict in api/rss-proxy.js was
straightforward — upstream still had the inline domain array while our branch
replaces it with an import from the new shared api/rss-allowed-domains.js.
Resolved by keeping the import-based approach. No upstream domain additions
were lost — the shared file was built from upstream's complete list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants