refactor: env runners by pi0 · Pull Request #3836 · nitrojs/nitro

pi0 · 2025-12-04T00:13:57Z

Runner part from dev server initially refactored as DevWorker primitive.

For purposes other than development mode (preview and testing), we can reuse same logic from env runner.

This PR does initial refactors.

vercel · 2025-12-04T00:14:02Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Preview	Comments	Updated (UTC)
nitro.build	Ready	Preview	Comment	Dec 4, 2025 0:23am

coderabbitai · 2025-12-04T00:14:10Z

Caution

Review failed

The pull request is closed.

📝 Walkthrough

Walkthrough

Refactors the dev-worker abstraction to an EnvRunner across the codebase: renames types and classes, replaces createDevWorker with getEnvRunner and cached _envRunner, updates Vite/dev runtime entry to node-runner, adjusts message/upgrade APIs, and adds shutdown + JSON error handling in the runtime.

Changes

Cohort / File(s)	Summary
Types & exports `src/types/runner.ts`, `src/types/index.ts`, `src/types/hooks.ts`	Rename Dev* types to Runner/Env* (DevMessageListener→RunnerMessageListener, DevRPCHooks→RunnerRPCHooks, DevWorker→EnvRunner, DevWorkerData→EnvRunnerData). WorkerHooks now reference EnvRunner. Remove NitroDevServerOptions. Switch types export from `./dev.ts` → `./runner.ts`.
Build / Vite layer `src/build/vite/env.ts`, `src/build/vite/dev.ts`, `src/build/vite/plugin.ts`, `src/build/vite/types.ts`	Replace createDevWorker → getEnvRunner; cache runner in `ctx._envRunner`; use NodeEnvRunner and runtime entry `internal/vite/node-runner.mjs`; simplify payload to `{ server: true }`; bind `nitro.fetch` to runner.fetch; update upgrade path to `getEnvRunner(ctx).upgrade?.(...)`; add Nitro close hook to close runner; replace `devWorker` with `_envRunner` type.
Dev server & app `src/dev/server.ts`, `src/dev/app.ts`	Replace NodeDevWorker → NodeEnvRunner and related types (DevWorkerData→EnvRunnerData, DevRPCHooks→RunnerRPCHooks, DevMessageListener→RunnerMessageListener). Update NitroDevServer to implement RunnerRPCHooks and adjust message listener APIs. Change proxy import path in `src/dev/app.ts`.
Runner implementation `src/runner/node.ts`	Rename NodeDevWorker → NodeEnvRunner; DevWorkerData→EnvRunnerData; DevMessageListener→RunnerMessageListener; make `hooks` optional with default empty object; update messaging APIs and log/status strings to reference node env runner; adjust constructor/data handling.
Runtime & config `src/runtime/internal/vite/node-runner.mjs`, `src/types/config.ts`	In runtime runner: remove global application reloads, add shutdown handling (close server + post exit), and return JSON error on Accept: application/json. In types: replace NitroDevServerOptions with inline `devServer` object type (port, hostname, watch).

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Pay special attention to cross-file type/name consistency (EnvRunner, EnvRunnerData, RunnerMessageListener, RunnerRPCHooks).
Verify getEnvRunner caching and lifecycle (startup warm-up, Nitro close hook, runner.close).
Review websocket upgrade delegation and the new optional upgrade? typing/guarding.
Inspect runtime node-runner message handling (shutdown path) and renderError JSON behavior.

Possibly related PRs

perf!: resolve virtual templates with single plugin #3819 — Related changes to Nitro virtual VFS/virtual-templates system; may intersect with runtime/template behavior and project bootstrapping.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 30.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'refactor: env runners' follows conventional commits format with a 'refactor' type and clearly summarizes the main change of refactoring dev workers into reusable env runners.
Description check	✅ Passed	The description is directly related to the changeset, explaining the refactoring context and purpose of extracting runner logic from the dev server to enable reuse across different modes.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 463317e and a6062b6.

📒 Files selected for processing (4)

src/build/vite/plugin.ts (2 hunks)
src/dev/server.ts (5 hunks)
src/runtime/internal/vite/node-runner.mjs (2 hunks)
src/types/runner.ts (1 hunks)

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

pkg-pr-new · 2025-12-04T00:17:51Z

Open in StackBlitz

npm i https://pkg.pr.new/nitrojs/nitro@3836

commit: 463317e

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (3)

src/types/runner.ts (1)
27-34: Consider making upgrade optional in the interface.

In src/build/vite/dev.ts (line 135), upgrade is called with optional chaining (upgrade?.()), suggesting not all runners may implement this method. However, the interface declares it as required.
 export interface EnvRunner extends WorkerHooks, RunnerRPCHooks {
   readonly ready: boolean;
   readonly closed: boolean;

   fetch: FetchHandler;
-  upgrade: UpgradeHandler;
+  upgrade?: UpgradeHandler;
   close(): Promise<void>;
 }
src/build/vite/plugin.ts (1)
16-19: Env runner integration in setupNitroContext is sound; minor comment typo

The new getEnvRunner(ctx) import, dev-only warm-up, ctx.nitro.fetch = (req) => getEnvRunner(ctx).fetch(req), and close hook that calls ctx._envRunner.close() together provide a coherent lifecycle for the shared env runner without obvious behavioral regressions. Only nit: the comment // Warm uop env runner for dev has a small typo (“uop” → “up”).
-  // Warm uop env runner for dev
+  // Warm up env runner for dev
Also applies to: 435-453
src/dev/server.ts (1)
74-92: Ensure all runners are actually closed on reload/dev:error

Because onClose removes a worker from this.#workers, iterating with for (const worker of this.#workers) while calling worker.close() can skip elements when the array is mutated during iteration, leaving some old workers alive (resource leak and stray RPC targets). Iterating over a shallow copy avoids this.
    nitro.hooks.hook("dev:error", (cause: unknown) => {
      this.#buildError = cause;
      this.#building = false;
-      for (const worker of this.#workers) {
-        worker.close();
-      }
+      for (const worker of [...this.#workers]) {
+        worker.close();
+      }
    });

  reload() {
-    for (const worker of this.#workers) {
-      worker.close();
-    }
+    for (const worker of [...this.#workers]) {
+      worker.close();
+    }
    const worker = new NodeEnvRunner({
Also applies to: 156-183

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3a2d266 and 463317e.

⛔ Files ignored due to path filters (1)

pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml

📒 Files selected for processing (12)

src/build/vite/dev.ts (2 hunks)
src/build/vite/env.ts (3 hunks)
src/build/vite/plugin.ts (2 hunks)
src/build/vite/types.ts (2 hunks)
src/dev/app.ts (1 hunks)
src/dev/server.ts (4 hunks)
src/runner/node.ts (7 hunks)
src/runtime/internal/vite/node-runner.mjs (2 hunks)
src/types/config.ts (1 hunks)
src/types/hooks.ts (2 hunks)
src/types/index.ts (1 hunks)
src/types/runner.ts (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (8)

src/types/hooks.ts (1)

src/runner/node.ts (1)

EnvRunnerData (18-21)

src/build/vite/plugin.ts (2)

src/build/vite/env.ts (1)

getEnvRunner (11-17)

src/dev/app.ts (1)

NitroDevApp (19-99)

src/build/vite/types.ts (1)

src/types/runner.ts (1)

EnvRunner (27-34)

src/build/vite/env.ts (3)

src/build/vite/types.ts (1)

NitroPluginContext (59-72)

src/runner/node.ts (1)

NodeEnvRunner (23-221)

src/runtime/meta.ts (1)

runtimeDir (9-9)

src/build/vite/dev.ts (1)

src/build/vite/env.ts (1)

getEnvRunner (11-17)

src/runtime/internal/vite/node-runner.mjs (1)

src/dev/server.ts (1)

error (226-269)

src/dev/server.ts (2)

src/types/runner.ts (3)

RunnerRPCHooks (14-18)

EnvRunner (27-34)

RunnerMessageListener (6-6)

src/runner/node.ts (2)

EnvRunnerData (18-21)

NodeEnvRunner (23-221)

src/runner/node.ts (1)

src/types/runner.ts (3)

EnvRunner (27-34)

WorkerHooks (22-25)

RunnerMessageListener (6-6)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)

GitHub Check: tests-rollup (ubuntu-latest)
GitHub Check: tests-rollup (windows-latest)
GitHub Check: tests-rolldown (windows-latest)

🔇 Additional comments (14)

src/dev/app.ts (1)

12-12: Import path change is valid and correctly refactored.

The module now exists at src/runner/proxy.ts with the createHTTPProxy export. The old module has been removed from src/dev/proxy.ts, and usage remains unchanged.

src/types/config.ts (1)

176-180: Inline type definition is safe—no breaking changes detected.

Verification confirms that NitroDevServerOptions is not referenced anywhere in the codebase and was not part of the public API. The inline type definition preserves the three essential fields (port, hostname, watch) and simplifies the configuration structure without side effects.

src/types/index.ts (1)

4-4: LGTM!

The export change correctly aligns with the refactoring from DevWorker to EnvRunner types. The new runner.ts module properly consolidates the runner-related type exports.

src/build/vite/dev.ts (2)

17-17: LGTM!

The import correctly brings in getEnvRunner for the refactored upgrade handler integration.

135-135: LGTM!

The upgrade handler delegation is correctly refactored to use getEnvRunner(ctx). The optional chaining on upgrade?.() is appropriate since the method may not be implemented on all runner types.

src/types/runner.ts (2)

6-6: LGTM!

The type rename from DevMessageListener to RunnerMessageListener aligns with the broader refactoring.

14-18: LGTM!

The interface rename to RunnerRPCHooks is consistent with the refactoring, and the method signatures correctly use the renamed RunnerMessageListener type.

src/runtime/internal/vite/node-runner.mjs (1)

193-197: LGTM!

Clean shutdown handling implementation. The server is properly closed before signaling exit to the parent, ensuring graceful termination.

src/types/hooks.ts (2)

1-1: LGTM!

The import correctly references the new EnvRunnerData type from the refactored runner module location.

14-17: LGTM!

The dev:reload hook payload correctly uses the renamed EnvRunnerData type, maintaining consistency with the broader refactoring.

src/build/vite/types.ts (1)

3-3: EnvRunner type and context field wiring look consistent

Importing EnvRunner and caching it on NitroPluginContext._envRunner aligns with the new getEnvRunner(ctx) helper and keeps the runner clearly scoped per plugin context; no issues from a typing or lifecycle perspective.

Also applies to: 68-72

src/build/vite/env.ts (1)

4-17: Shared EnvRunner caching and wiring across environments looks correct

getEnvRunner(ctx) properly memoizes a NodeEnvRunner on ctx._envRunner, and both createNitroEnvironment and createServiceEnvironment now pass that shared runner into createFetchableDevEnvironment, which matches the intended reuse of environment runner logic across dev/service environments without obvious downsides.

Also applies to: 45-52, 78-85

src/dev/server.ts (1)

5-7: Dev server runner refactor to EnvRunner/RunnerRPCHooks is coherent

Switching to NodeEnvRunner/EnvRunnerData, storing EnvRunner[] in #workers, and implementing RunnerRPCHooks (including sendMessage, onMessage, offMessage) keeps the dev server aligned with the new runner abstractions. The construction of NodeEnvRunner in reload() with data: this.#workerData and the propagation of message listeners to each active worker all look consistent with the updated NodeEnvRunner API.

Also applies to: 29-40, 160-177, 185-205

src/runner/node.ts (1)

5-9: NodeEnvRunner implementation matches the new EnvRunner/RunnerRPCHooks contracts

The refactor to EnvRunnerData and NodeEnvRunner implements EnvRunner—including RunnerMessageListener-typed onMessage/offMessage, the updated 503 message in fetch, and the clearer force-close warning—lines up with the new runner typings and expected behavior, with no apparent correctness or lifecycle issues.

Also applies to: 18-21, 23-24, 28-40, 67-72, 92-99, 101-107, 122-126, 196-203

src/runtime/internal/vite/node-runner.mjs

refactor: env runners

463317e

vercel bot deployed to Preview December 4, 2025 00:14 View deployment

coderabbitai bot reviewed Dec 4, 2025

View reviewed changes

src/runtime/internal/vite/node-runner.mjs Show resolved Hide resolved

small changes

a6062b6

pi0 merged commit 497db05 into main Dec 4, 2025
7 of 9 checks passed

pi0 deleted the refactor/runner branch December 4, 2025 00:23

vercel bot deployed to Preview December 4, 2025 00:23 View deployment

This was referenced Dec 12, 2025

nitro/vite in node preset should serve public assets from the server folder and not going up to the public directory #3868

Closed

fix: use module condition and main fields for bundling #3886

Merged

coderabbitai bot mentioned this pull request Jan 5, 2026

feat(vite): experimental rsc plugin support #3919

Merged

8 tasks

coderabbitai bot mentioned this pull request Jan 21, 2026

fix: use static replacement for websocket feature #3964

Merged

coderabbitai bot mentioned this pull request Feb 10, 2026

fix: restore runtime close hook and implement graceful shutdown env vars #4017

Open

8 tasks

coderabbitai bot mentioned this pull request Feb 18, 2026

feat: nitro preview #4024

Merged

3 tasks

coderabbitai bot mentioned this pull request Mar 10, 2026

feat: use unjs/env-runner #4088

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: env runners#3836

refactor: env runners#3836
pi0 merged 2 commits intomainfrom
refactor/runner

pi0 commented Dec 4, 2025

Uh oh!

vercel bot commented Dec 4, 2025 •

edited

Loading

Uh oh!

coderabbitai bot commented Dec 4, 2025 •

edited

Loading

Review failed

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Uh oh!

pkg-pr-new bot commented Dec 4, 2025

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

pi0 commented Dec 4, 2025

Uh oh!

vercel bot commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai bot commented Dec 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review failed

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Pre-merge checks and finishing touches

Uh oh!

pkg-pr-new bot commented Dec 4, 2025

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

vercel bot commented Dec 4, 2025 •

edited

Loading

coderabbitai bot commented Dec 4, 2025 •

edited

Loading