Skip to content

feat(prerender): log server startup#1094

Merged
james-elicx merged 1 commit intocloudflare:mainfrom
NathanDrake2406:nathan/prerender-server-logs
May 6, 2026
Merged

feat(prerender): log server startup#1094
james-elicx merged 1 commit intocloudflare:mainfrom
NathanDrake2406:nathan/prerender-server-logs

Conversation

@NathanDrake2406
Copy link
Copy Markdown
Contributor

Summary

  • Add a prerender-specific production server startup log that includes the actual local URL/port.
  • Keep the default production server startup log unchanged for normal vinext start.
  • Thread the narrow purpose: "prerender" option through app-only, pages-only, and hybrid/shared prerender server startup paths.
  • Add focused coverage for Pages Router, App Router, and default production server startup logs.

Part of #563.

Validation

  • vp test run tests/prod-server-logs.test.ts
  • vp test run tests/prerender.test.ts -t "runPrerender — output"
  • vp check tests/prod-server-logs.test.ts packages/vinext/src/server/prod-server.ts packages/vinext/src/build/prerender.ts packages/vinext/src/build/run-prerender.ts

@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented May 6, 2026

Open in StackBlitz

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

commit: 29842aa

@NathanDrake2406 NathanDrake2406 marked this pull request as ready for review May 6, 2026 07:14
@james-elicx
Copy link
Copy Markdown
Collaborator

/bigbonk review

Copy link
Copy Markdown
Contributor

@ask-bonk ask-bonk Bot left a comment

Choose a reason for hiding this comment

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

Clean, minimal change. The purpose option threads correctly through all three startup paths (app-only, pages-only, hybrid/shared), the logProdServerStarted helper is a nice consolidation of the two duplicated console.log lines, and the test coverage is solid.

One minor observation on the tests below, but nothing blocking.

messages.push(message);
});

const { startProdServer } = await import("../packages/vinext/src/server/prod-server.js");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: all three tests await import(...) the same module. Since Vitest (and Node) cache dynamic imports by specifier, the second and third tests will get the same module object as the first. This works today because startProdServer is a stateless function, but if it ever acquired module-level mutable state (e.g., a cached config), the tests could leak between each other.

Consider adding vi.resetModules() in the afterEach (or a beforeEach) to make each test get a fresh module instance. Not urgent — just future-proofing.

return (response as ResponseWithVinextStreamingMetadata).__vinextStreamedHtmlResponse === true;
}

function logProdServerStarted(host: string, port: number, purpose: ProdServerOptions["purpose"]) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice consolidation — this eliminates the duplicated log strings that previously lived in both startAppRouterServer and startPagesRouterServer.

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented May 6, 2026

Review submitted. Here's a summary:

Approved -- this is a clean, well-scoped change. The purpose: "prerender" option threads correctly through all three server startup paths (startProdServer -> startAppRouterServer / startPagesRouterServer, plus the hybrid shared server in run-prerender.ts). The logProdServerStarted helper consolidates the previously duplicated console.log calls in both router servers.

One minor observation: the test file uses await import(...) for the same module across all three test cases without vi.resetModules(), so they share the same cached module instance. This works fine today since startProdServer is stateless, but adding resetModules in afterEach would future-proof against module-level state leaks.

github run

@james-elicx james-elicx merged commit 70410ba into cloudflare:main May 6, 2026
26 checks passed
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