[james-elicx]

Summary

• respect server.host and server.port from the project's Vite config when running vinext dev
• keep explicit CLI flags as the highest-precedence values while preserving vinext's existing defaults
• update dev lockfile metadata from Vite's resolved server configuration

Testing

• vp test run tests/cli-dev-config.test.ts tests/cli-args.test.ts tests/dev-lockfile.test.ts
• vp check packages/vinext/src/cli.ts packages/vinext/src/cli-dev-config.ts tests/cli-dev-config.test.ts
• vp run vinext#build

[james-elicx]

/bigbonk review for issues

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/@vinext/cloudflare@2379

npm i https://pkg.pr.new/vinext@2379

commit: 82311ee

[github-actions]

Example	Preview	Production	Original
app-router-cloudflare	preview	production
pages-router-cloudflare	preview	production
app-router-playground	preview	production	original
realworld-api-rest	preview	production
nextra-docs-template	preview	production
benchmarks	preview	production
hackernews	preview	production	original
workers-cache	preview	production
web	preview	production

[github-actions]

Performance benchmarks

Compared 82311ee against base 25a3c26 using alternating same-runner rounds. Next.js was unchanged and skipped.

0 improved · 0 regressed · 6 within ±1.5%

Scenario	Framework	Baseline	Current	Change
Client bundle size (gzip)	vinext	117.1 KB	117.1 KB	⚫ +0.0%
Client entry size (gzip)	vinext	112.5 KB	112.5 KB	⚫ -0.0%
Dev server cold start	vinext	2.44 s	2.46 s	⚫ +0.7%
Production build time	vinext	2.95 s	2.94 s	⚫ -0.3%
RSC entry closure size (gzip)	vinext	93.1 KB	93.1 KB	⚫ +0.0%
Server bundle size (gzip)	vinext	157.0 KB	157.0 KB	⚫ +0.0%

View detailed results and traces

_{🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head}

[ask-bonk]

Review: respect Vite dev server config

I reviewed the change end-to-end (read cli.ts, cli-dev-config.ts, dev-lockfile.ts, ran the PR tests + vp check, and empirically validated the Vite config-hook precedence with a Vite 8 resolveConfig harness). Overall this is a clean, well-scoped fix that does what the PR description claims. Tests and checks pass locally.

What works well

• Precedence is correct. I verified empirically against Vite 8 resolveConfig:
  • No CLI flags + values in vite.config.ts → config values are respected (e.g. 5555 / config.example.test).
  • CLI flags present → they override the config (e.g. --port 4000 --hostname 0.0.0.0 wins).
  • Nothing set anywhere → vinext defaults (3000 / localhost) win, not Vite's default 5173. The ?? 3000 / ?? "localhost" fallbacks in applyDevServerDefaults are load-bearing here — without them users would silently get 5173. Good catch keeping the vinext default.
• normalizeDevServerHostname correctly maps Vite's host: true → 0.0.0.0 and false/undefined → localhost, matching Vite semantics and the existing wildcard-display logic.
• Lockfile safety. The new intermediate update() between createServer() and listen() is safe: release() keys off the owner PID, so a later release() in the catch still cleans up correctly regardless of intervening updates.
• Field shapes line up: parseArgs returns { port?, hostname? }, exactly matching DevServerCliOptions, so passing parsed directly is fine.

Issues / suggestions

1. (minor, edge case) CLI flags can be overridden by a user config hook ordered after the vinext plugin.

The plugin applies CLI flags inside a config hook with order: "post". If a user's own plugin also uses a config hook with order: "post" and resolves after the vinext plugin, it overrides the explicit CLI flags. I confirmed this with a harness:

plugins: [createDevServerConfigPlugin({ port: 4000 }), userPostPlugin /* sets 9999 */]
=> resolved server.port = 9999   // CLI --port 4000 lost

In the real dev() path the vinext plugin is pushed last into the inline config, which makes this unlikely in practice, but it's a real divergence from "CLI flags are highest precedence." The PR's own test only passes because the vinext plugin is positioned last in the array. This is probably acceptable as a known limitation, but worth a one-line comment noting that CLI-flag precedence relies on the vinext plugin running last among post config hooks.

2. (nit) enforce: "post" is redundant for a config hook. Config-hook ordering is governed by the hook's own order: "post", not the plugin's enforce. The enforce: "post" doesn't affect the config hook here. Harmless, but it can be dropped to avoid implying it controls config ordering.

3. (nit) The intermediate lockfile update() is largely redundant with the post-listen update — it only matters in the brief window before listen() resolves. Not a problem, just noting it adds a third write path; fine to keep for accuracy during startup.

Verification

• vp test run tests/cli-dev-config.test.ts tests/cli-args.test.ts tests/dev-lockfile.test.ts → 79 passed
• vp check packages/vinext/src/cli.ts packages/vinext/src/cli-dev-config.ts tests/cli-dev-config.test.ts → clean (format, lint, types)

No blocking issues. The two nits and the edge-case note are optional. LGTM with a suggestion to document the precedence assumption in #1.

[ask-bonk]

Review posted to PR #2379.

Summary

I reviewed PR #2379 (fix(cli): respect Vite dev server config) thoroughly — read the changed files plus dev-lockfile.ts, ran the PR's tests and vp check, and empirically validated the Vite config-hook precedence using a Vite 8 resolveConfig harness.

Verdict: LGTM, no blocking issues. Tests (79 passed) and checks (format/lint/types) are clean.

Key findings:

• Precedence is correct in all three cases: config-file values respected when no flags; CLI flags override config; vinext defaults (3000/localhost) win when nothing is set. I confirmed the ?? 3000 / ?? "localhost" fallbacks are load-bearing — without them users would silently get Vite's default 5173.
• Lockfile safety verified — the new intermediate update() is safe because release() keys off the owner PID.
• normalizeDevServerHostname correctly maps Vite's host: true → 0.0.0.0.

Non-blocking notes I raised:

1. (edge case) CLI flags can be overridden by a user config hook with order: "post" positioned after the vinext plugin — CLI-flag precedence relies on the vinext plugin running last. Worth a clarifying comment.
2. (nit) enforce: "post" is redundant for a config hook (ordering is governed by order: "post").
3. (nit) The intermediate lockfile update is largely redundant with the post-listen update, but harmless.

github run

[james-elicx]

/bigbonk review for issues

[ask-bonk]

Review: respect Vite dev server config (re-review @ 82311ee)

I re-reviewed the latest state including the new 82311ee ("docs(cli): clarify Vite config hook ordering"). I read cli.ts, cli-dev-config.ts, dev-lockfile.ts, cli-args.ts, ran the PR tests + vp check, and empirically validated the plugin-ordering precedence against @voidzero-dev/vite-plus-core (Vite 8) resolveConfig in the real-world shape (user plugin loaded via configFile, vinext plugin appended to the inline config).

Verdict: LGTM, no blocking issues. vp check is clean (format/lint/types) and the targeted tests pass (79 passed).

Verification

• vp test run tests/cli-dev-config.test.ts tests/cli-args.test.ts tests/dev-lockfile.test.ts → 79 passed
• vp check packages/vinext/src/cli.ts packages/vinext/src/cli-dev-config.ts tests/cli-dev-config.test.ts → clean

What's correct

• Precedence holds in the real path. I reproduced the actual dev() shape: user vite.config exports a plugin with enforce: "post" + a config hook with order: "post" that sets port: 9999, and vinext's plugin is appended to the inline config. Result: server.port = 4000, host = 127.0.0.1 — CLI flags win even against a maximally-aggressive user post plugin. The combination of inline placement + enforce: "post" + config order: "post" is what makes vinext run last.
• ?? 3000 / ?? "localhost" are load-bearing — without them users silently get Vite's 5173 default. Good.
• normalizeDevServerHostname maps true → 0.0.0.0, false/undefined → localhost, matching Vite semantics and the wildcard-display substitution.
• Lockfile safety. release() keys off the PID captured at acquire time (dev-lockfile.ts:243), so the new intermediate update() between createServer() and listen() cannot break cleanup on a listen() failure. Confirmed.
• Type shape. ParsedArgs ({ port?: number; hostname?: string; ... }) structurally satisfies DevServerCliOptions, so passing parsed directly is sound; the extra fields are ignored.
• The new 82311ee comment in cli-dev-config.ts accurately documents the two-level ordering requirement, and the added createDevServerConfigPlugin test exercises the late-user-config case. This resolves the precedence-documentation concern from the prior review.

One correction to the prior review

The earlier review's nit #2 — "enforce: "post" is redundant for a config hook" — is incorrect. It only holds for plugins in the same inline array. In the actual dev() path the user's plugins are loaded from configFile and vinext's plugin is appended to the inline config. I verified empirically:

• vinext plugin without enforce: "post" (config order: "post" only), user post plugin via configFile → resolved port = 9999 (CLI flag lost).
• vinext plugin with enforce: "post" → resolved port = 4000 (CLI flag wins).

So enforce: "post" is load-bearing, not redundant — it's required to order vinext's config hook after a user's config-file plugin that also enforces post. The new comment in cli-dev-config.ts ("Both levels are required…") captures this correctly. Do not drop enforce: "post".

Non-blocking nits

1. The intermediate lockfile update() (before listen()) writes the configured port, which the post-listen update() later corrects to the actual bound port. Redundant but intentional/harmless for startup accuracy.
2. Minor inconsistency: acquire uses cwd: root while the updates use cwd: process.cwd(). Equivalent (root === process.cwd()), purely cosmetic.

No changes required to merge.

[ask-bonk]

Review posted to PR #2379.

Summary

I re-reviewed PR #2379 at its current tip (82311ee, including the new "docs(cli): clarify Vite config hook ordering" commit). I read all changed files plus dev-lockfile.ts and cli-args.ts, ran the PR's tests and vp check, and empirically validated the plugin-ordering precedence against Vite 8 resolveConfig in the real-world shape (user plugin from configFile, vinext plugin appended to inline config).

Verdict: LGTM, no blocking issues. 79 tests pass; vp check clean.

Key findings:

• Precedence is robust. CLI flags win even against a maximally-aggressive user post-enforced config-file plugin, because vinext's plugin combines inline placement + enforce: "post" + config order: "post".
• enforce: "post" is load-bearing, not redundant — this corrects the prior review's nit Published npm package ships sourcemaps pointing to missing source files #2. I proved empirically that dropping enforce: "post" causes CLI flags to be silently lost when the user has a post plugin in their vite.config.ts. The new 82311ee comment documents this correctly.
• Lockfile safety verified — release() keys off the acquire-time PID, so the new intermediate update() is safe.
• Type shape of ParsedArgs structurally satisfies DevServerCliOptions.

The new commit resolves the prior review's precedence-documentation concern (added comment + test). Only two cosmetic non-blocking nits remain. No changes required to merge.

github run