refactor(nitro,nuxt): extract @nuxt/nitro-server package
#33462
Conversation
|
|
packages/nuxt/src/core/server.ts
Outdated
| } catch (error: any) { | ||
| await nuxt.callHook('build:error', error) | ||
|
|
||
| if (error.toString().includes('Cannot find module \'@nuxt/webpack-builder\'')) { |
| server: { | ||
| builder: { | ||
| $resolve: (val) => { | ||
| if (typeof val === 'string') { return val } | ||
| return '@nuxt/nitro-server' | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
is this the right api?
@nuxt/kit
nuxt
@nuxt/rspack-builder
@nuxt/schema
@nuxt/vite-builder
@nuxt/webpack-builder
commit: |
CodSpeed Performance ReportMerging #33462 will not alter performanceComparing Summary
|
WalkthroughAdds a standalone Nitro server package (packages/nitro-server) with build config, utilities, templates and TS type generation. Moves Nitro bootstrap entry from initNitro to bundle and wires a new Nuxt server orchestrator (bundleServer) that dynamically loads server builders. Shifts many Nuxt-internal type imports to nuxt/app and relocates/removes nitropack module augmentations from Nuxt to the nitro-server template. Adds schema support for server.builder with a resolver defaulting to '@nuxt/nitro-server'. Updates UI template paths and gitignore/templates for Nitro runtime. Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/nitro-server/src/runtime/utils/renderer/islands.ts (1)
62-86: Escape dynamic values in RegExp to avoid incorrect matches.
uid,clientId, andslotare interpolated into regexes without escaping. If they contain regex meta, replacements may fail or misbehave.Apply:
+import escapeRE from 'escape-string-regexp' @@ - html = html.replace(new RegExp(` data-island-uid="${uid}" data-island-component="${clientId}"[^>]*>`), (full) => { + const eUid = escapeRE(uid) + const eClientId = escapeRE(clientId) + html = html.replace(new RegExp(` data-island-uid="${eUid}" data-island-component="${eClientId}"[^>]*>`), (full) => { return full + teleports[key] }) @@ - html = html.replace(new RegExp(` data-island-uid="${uid}" data-island-slot="${slot}"[^>]*>`), (full) => { + const eUid2 = escapeRE(uid) + const eSlot = escapeRE(slot) + html = html.replace(new RegExp(` data-island-uid="${eUid2}" data-island-slot="${eSlot}"[^>]*>`), (full) => { return full + teleports[key] })
♻️ Duplicate comments (1)
packages/nuxt/types.d.mts (1)
1-1: Mirror of DTS change; verify Nitro typings source.Same note as in
packages/nuxt/types.d.ts: ensure Nitro augmentations are generated from@nuxt/nitro-server.
🧹 Nitpick comments (9)
packages/nitro-server/src/runtime/utils/renderer/build-files.ts (2)
39-44: Avoid relying on hoisting for local renderToStringrenderToString is referenced before its declaration. This works due to function hoisting, but moving the declaration above improves clarity and avoids edge cases in transformed output.
- // Create renderer - const renderer = createRenderer(createSSRApp, { - precomputed, - manifest: import.meta.dev ? await getClientManifest() : undefined, - renderToString, - buildAssetsURL, - }) - - type RenderToStringParams = Parameters<typeof _renderToString> - async function renderToString (input: RenderToStringParams[0], context: RenderToStringParams[1]) { + type RenderToStringParams = Parameters<typeof _renderToString> + async function renderToString (input: RenderToStringParams[0], context: RenderToStringParams[1]) { const html = await _renderToString(input, context) // In development with vite-node, the manifest is on-demand and will be available after rendering if (import.meta.dev && process.env.NUXT_VITE_NODE_OPTIONS) { renderer.rendererContext.updateManifest(await getClientManifest()) } return APP_ROOT_OPEN_TAG + html + APP_ROOT_CLOSE_TAG } + // Create renderer + const renderer = createRenderer(createSSRApp, { + precomputed, + manifest: import.meta.dev ? await getClientManifest() : undefined, + renderToString, + buildAssetsURL, + })
77-85: Dev parity: update manifest in SPA path with vite-nodeSSR updates the manifest post-render in dev with vite-node; SPA path doesn’t. Mirroring the update improves consistency for tools relying on rendererContext.
const renderer = createRenderer(() => () => {}, { precomputed, manifest: import.meta.dev ? await getClientManifest() : undefined, renderToString: () => spaTemplate, buildAssetsURL, }) const result = await renderer.renderToString({}) + if (import.meta.dev && process.env.NUXT_VITE_NODE_OPTIONS) { + renderer.rendererContext.updateManifest(await getClientManifest()) + }packages/schema/src/types/schema.ts (1)
1529-1535: Document default and object support forserver.builder.Types allow an object
{ bundle(nuxt) }, but runtime resolver currently ignores non-string values. Please:
- Clarify in JSDoc that default is '@nuxt/nitro-server'.
- Ensure the resolver preserves object builders (see comment in config/nitro.ts).
packages/nuxt/src/core/server.ts (1)
3-3: Tighten server builder typing and remove stray webpack error check
- The server builder exposes
bundle, which doesn't align withNuxtBuilder. Use a localServerBuildertype instead.- The webpack-specific error message is unrelated here; it can confuse users. Let
loadServerBuilderraise a contextual error.- Prefer
unknownin catch.-import type { Nuxt, NuxtBuilder } from 'nuxt/schema' +import type { Nuxt } from 'nuxt/schema' + +type ServerBuilder = { + bundle: (nuxt: Nuxt) => Promise<void> | void +} export async function bundleServer (nuxt: Nuxt) { try { const { bundle } = !nuxt.options.server.builder || typeof nuxt.options.server.builder === 'string' ? await loadServerBuilder(nuxt, nuxt.options.server.builder) : nuxt.options.server.builder await bundle(nuxt) - } catch (error: any) { + } catch (error: unknown) { await nuxt.callHook('build:error', error) - - if (error.toString().includes('Cannot find module \'@nuxt/webpack-builder\'')) { - throw new Error('Could not load `@nuxt/webpack-builder`. You may need to add it to your project dependencies, following the steps in `https://github.com/nuxt/framework/pull/2812`.') - } - throw error } } -async function loadServerBuilder (nuxt: Nuxt, builder = '@nuxt/nitro-server'): Promise<NuxtBuilder> { +async function loadServerBuilder (nuxt: Nuxt, builder = '@nuxt/nitro-server'): Promise<ServerBuilder> { try { return await importModule(builder, { url: [directoryToURL(nuxt.options.rootDir), new URL(import.meta.url)] }) } catch (err) { // TODO: docs throw new Error(`Loading \`${builder}\` server builder failed.`, { cause: err }) } }Also applies to: 5-12, 15-17, 23-30
packages/nitro-server/src/index.ts (1)
128-130: Harden Nuxt version resolutionWrap
readPackageJSON('nuxt', …)to avoid aborting when the package cannot be resolved; you already have a fallback.-const { version: nuxtVersion } = await readPackageJSON('nuxt', { from: import.meta.url }) +const { version: nuxtVersion } = await readPackageJSON('nuxt', { from: import.meta.url }).catch(() => ({} as any))packages/nuxt/src/core/nuxt.ts (1)
236-241: Normalise server builder type reference to a relative pathKeep consistency with the Vite builder: if
serverBuilderTypePathresolves to an absolute path, convert it to a path relative tobuildDirbefore adding it to TS references.-const serverBuilderTypePath = typeof nuxt.options.server.builder === 'string' - ? nuxt.options.server.builder === '@nuxt/nitro-server' - ? resolveModulePath(nuxt.options.server.builder, { from: import.meta.url }) - : nuxt.options.server.builder - : undefined +const _serverBuilderTypePath = typeof nuxt.options.server.builder === 'string' + ? nuxt.options.server.builder === '@nuxt/nitro-server' + ? resolveModulePath(nuxt.options.server.builder, { from: import.meta.url }) + : nuxt.options.server.builder + : undefined +const serverBuilderTypePath = + _serverBuilderTypePath && isAbsolute(_serverBuilderTypePath) + ? relative(nuxt.options.buildDir, _serverBuilderTypePath) + : _serverBuilderTypePath @@ - if (serverBuilderTypePath) { - opts.references.push({ types: serverBuilderTypePath }) - opts.nodeReferences.push({ types: serverBuilderTypePath }) - } + if (serverBuilderTypePath) { + opts.references.push({ types: serverBuilderTypePath }) + opts.nodeReferences.push({ types: serverBuilderTypePath }) + } @@ - if (serverBuilderTypePath) { - opts.references.push({ types: serverBuilderTypePath }) - } + if (serverBuilderTypePath) { + opts.references.push({ types: serverBuilderTypePath }) + }Additional import required:
-import { join, normalize, relative, resolve } from 'pathe' +import { join, normalize, relative, resolve, isAbsolute } from 'pathe'Also applies to: 266-269, 295-297
packages/nitro-server/src/templates.ts (1)
13-20: Also normalise absolutetypesreferencesWhen
typesis an absolute path, convert it to a relative path like you do forpath.- ...references.map((ref) => { - if ('path' in ref && isAbsolute(ref.path)) { - ref.path = relative(sourceDir, ref.path) - } - return `/// <reference ${renderAttrs(ref)} />` - }), + ...references.map((ref) => { + if ('path' in ref && isAbsolute(ref.path)) { + ref.path = relative(sourceDir, ref.path) + } + if ('types' in ref && typeof (ref as any).types === 'string' && isAbsolute((ref as any).types)) { + (ref as any).types = relative(sourceDir, (ref as any).types) + } + return `/// <reference ${renderAttrs(ref as any)} />` + }),packages/nitro-server/src/augments.ts (2)
8-20: DRY the duplicated augmentation shapes to prevent drift.Both
'nitropack'and'nitropack/types'blocks duplicate identical shapes. Extract local shared types and have each interface extend them.Apply these interface tweaks inside the listed ranges:
declare module 'nitropack' { - interface NitroRuntimeConfigApp { - buildAssetsDir: string - cdnURL: string - } - interface NitroRouteRules { - ssr?: boolean - noScripts?: boolean - /** @deprecated Use `noScripts` instead */ - experimentalNoScripts?: boolean - appMiddleware?: Record<string, boolean> - } + interface NitroRuntimeConfigApp extends _NitroRuntimeConfigAppFields {} + interface NitroRouteRules extends _NitroRouteRulesFields {} } declare module 'nitropack/types' { - interface NitroRuntimeConfigApp { - buildAssetsDir: string - cdnURL: string - } - interface NitroRouteRules { - ssr?: boolean - noScripts?: boolean - /** @deprecated Use `noScripts` instead */ - experimentalNoScripts?: boolean - appMiddleware?: Record<string, boolean> - } + interface NitroRuntimeConfigApp extends _NitroRuntimeConfigAppFields {} + interface NitroRouteRules extends _NitroRouteRulesFields {} } // Note: Keep in sync with packages/nuxt/src/core/templates.ts declare module 'nitropack' { // eslint-disable-next-line @typescript-eslint/no-empty-object-type interface NitroRuntimeConfig extends RuntimeConfig {} - interface NitroRouteConfig { - ssr?: boolean - noScripts?: boolean - /** @deprecated Use `noScripts` instead */ - experimentalNoScripts?: boolean - } - interface NitroRuntimeHooks { - 'dev:ssr-logs': (ctx: { logs: LogObject[], path: string }) => void | Promise<void> - 'render:html': (htmlContext: NuxtRenderHTMLContext, context: { event: H3Event }) => void | Promise<void> - 'render:island': (islandResponse: NuxtIslandResponse, context: { event: H3Event, islandContext: NuxtIslandContext }) => void | Promise<void> - } + interface NitroRouteConfig extends _NitroRouteConfigFields {} + interface NitroRuntimeHooks extends _NitroRuntimeHooksShape {} } declare module 'nitropack/types' { // eslint-disable-next-line @typescript-eslint/no-empty-object-type interface NitroRuntimeConfig extends RuntimeConfig {} - interface NitroRouteConfig { - ssr?: boolean - noScripts?: boolean - /** @deprecated Use `noScripts` instead */ - experimentalNoScripts?: boolean - } - interface NitroRuntimeHooks { - 'dev:ssr-logs': (ctx: { logs: LogObject[], path: string }) => void | Promise<void> - 'render:html': (htmlContext: NuxtRenderHTMLContext, context: { event: H3Event }) => void | Promise<void> - 'render:island': (islandResponse: NuxtIslandResponse, context: { event: H3Event, islandContext: NuxtIslandContext }) => void | Promise<void> - } + interface NitroRouteConfig extends _NitroRouteConfigFields {} + interface NitroRuntimeHooks extends _NitroRuntimeHooksShape {} }Add these shared shapes above the first
declare module:type _NitroRuntimeConfigAppFields = { buildAssetsDir: string cdnURL: string } type _NitroRouteRulesFields = { ssr?: boolean noScripts?: boolean /** @deprecated Use `noScripts` instead */ experimentalNoScripts?: boolean appMiddleware?: Record<string, boolean> } type _NitroRouteConfigFields = { ssr?: boolean noScripts?: boolean /** @deprecated Use `noScripts` instead */ experimentalNoScripts?: boolean } type _NitroRuntimeHooksShape = { 'dev:ssr-logs': (ctx: { logs: LogObject[], path: string }) => void | Promise<void> 'render:html': (htmlContext: NuxtRenderHTMLContext, context: { event: H3Event }) => void | Promise<void> 'render:island': (islandResponse: NuxtIslandResponse, context: { event: H3Event, islandContext: NuxtIslandContext }) => void | Promise<void> } ```<!-- review_comment_end --> Also applies to: 22-34, 37-51, 52-66 --- `40-45`: **Confirm omission of `appMiddleware` from `NitroRouteConfig` is intentional.** `NitroRouteRules` includes `appMiddleware`, but `NitroRouteConfig` does not. If that’s by design (rules vs per‑route config), all good; otherwise add it for parity. <!-- review_comment_end --> <!-- file_end --> Also applies to: 55-60 </blockquote></details> </blockquote></details> <details> <summary>📜 Review details</summary> **Configuration used**: CodeRabbit UI **Review profile**: CHILL **Plan**: Pro <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 3c38d1f8bdff7c6d86579a2722507590a17022c9 and 0295d5050d51093f68863b668526afa41673f114. </details> <details> <summary>⛔ Files ignored due to path filters (4)</summary> * `package.json` is excluded by `!package.json`, `!**/package.json` * `packages/nitro-server/package.json` is excluded by `!**/package.json` * `packages/nuxt/package.json` is excluded by `!**/package.json` * `pnpm-lock.yaml` is excluded by `!**/pnpm-lock.yaml`, `!pnpm-lock.yaml` </details> <details> <summary>📒 Files selected for processing (25)</summary> * `packages/nitro-server/.attw.json` (1 hunks) * `packages/nitro-server/.gitignore` (1 hunks) * `packages/nitro-server/build.config.ts` (1 hunks) * `packages/nitro-server/src/augments.ts` (1 hunks) * `packages/nitro-server/src/index.ts` (13 hunks) * `packages/nitro-server/src/runtime/handlers/error.ts` (1 hunks) * `packages/nitro-server/src/runtime/handlers/island.ts` (1 hunks) * `packages/nitro-server/src/runtime/handlers/renderer.ts` (1 hunks) * `packages/nitro-server/src/runtime/utils/renderer/app.ts` (1 hunks) * `packages/nitro-server/src/runtime/utils/renderer/build-files.ts` (1 hunks) * `packages/nitro-server/src/runtime/utils/renderer/islands.ts` (1 hunks) * `packages/nitro-server/src/templates.ts` (1 hunks) * `packages/nitro-server/src/utils.ts` (1 hunks) * `packages/nuxt/build.config.ts` (0 hunks) * `packages/nuxt/src/app/types.ts` (1 hunks) * `packages/nuxt/src/app/types/augments.ts` (0 hunks) * `packages/nuxt/src/core/nuxt.ts` (6 hunks) * `packages/nuxt/src/core/plugins/import-protection.ts` (1 hunks) * `packages/nuxt/src/core/server.ts` (1 hunks) * `packages/nuxt/src/core/templates.ts` (2 hunks) * `packages/nuxt/types.d.mts` (1 hunks) * `packages/nuxt/types.d.ts` (1 hunks) * `packages/schema/src/config/nitro.ts` (1 hunks) * `packages/schema/src/types/schema.ts` (1 hunks) * `packages/ui-templates/lib/render.ts` (1 hunks) </details> <details> <summary>💤 Files with no reviewable changes (2)</summary> * packages/nuxt/build.config.ts * packages/nuxt/src/app/types/augments.ts </details> <details> <summary>🧰 Additional context used</summary> <details> <summary>📓 Path-based instructions (1)</summary> <details> <summary>**/*.{ts,tsx}</summary> **📄 CodeRabbit inference engine (.github/copilot-instructions.md)** > Follow standard TypeScript conventions and best practices Files: - `packages/schema/src/config/nitro.ts` - `packages/nitro-server/src/runtime/utils/renderer/islands.ts` - `packages/nitro-server/src/utils.ts` - `packages/nitro-server/src/runtime/handlers/renderer.ts` - `packages/ui-templates/lib/render.ts` - `packages/nuxt/src/core/templates.ts` - `packages/nitro-server/build.config.ts` - `packages/nitro-server/src/runtime/handlers/error.ts` - `packages/nitro-server/src/augments.ts` - `packages/schema/src/types/schema.ts` - `packages/nuxt/src/core/server.ts` - `packages/nuxt/types.d.ts` - `packages/nitro-server/src/runtime/utils/renderer/build-files.ts` - `packages/nitro-server/src/runtime/utils/renderer/app.ts` - `packages/nuxt/src/app/types.ts` - `packages/nitro-server/src/index.ts` - `packages/nuxt/src/core/plugins/import-protection.ts` - `packages/nitro-server/src/runtime/handlers/island.ts` - `packages/nuxt/src/core/nuxt.ts` - `packages/nitro-server/src/templates.ts` </details> </details><details> <summary>🧬 Code graph analysis (7)</summary> <details> <summary>packages/nitro-server/build.config.ts (2)</summary><blockquote> <details> <summary>packages/nuxt/build.config.ts (1)</summary> * `ctx` (25-27) </details> <details> <summary>debug/build-config.ts (1)</summary> * `addRollupTimingsPlugin` (20-24) </details> </blockquote></details> <details> <summary>packages/nitro-server/src/augments.ts (1)</summary><blockquote> <details> <summary>packages/nuxt/src/app/types.ts (3)</summary> * `NuxtRenderHTMLContext` (39-46) * `NuxtIslandResponse` (30-37) * `NuxtIslandContext` (21-28) </details> </blockquote></details> <details> <summary>packages/nuxt/src/core/server.ts (1)</summary><blockquote> <details> <summary>packages/nitro-server/src/index.ts (1)</summary> * `bundle` (37-766) </details> </blockquote></details> <details> <summary>packages/nuxt/src/app/types.ts (1)</summary><blockquote> <details> <summary>packages/nuxt/src/app/index.ts (4)</summary> * `NuxtAppLiterals` (14-14) * `NuxtIslandContext` (14-14) * `NuxtIslandResponse` (14-14) * `NuxtRenderHTMLContext` (14-14) </details> </blockquote></details> <details> <summary>packages/nitro-server/src/index.ts (2)</summary><blockquote> <details> <summary>packages/nitro-server/src/templates.ts (1)</summary> * `nitroSchemaTemplate` (4-82) </details> <details> <summary>packages/nitro-server/src/utils.ts (1)</summary> * `distDir` (11-11) </details> </blockquote></details> <details> <summary>packages/nuxt/src/core/plugins/import-protection.ts (1)</summary><blockquote> <details> <summary>packages/schema/src/types/config.ts (1)</summary> * `NuxtOptions` (82-94) </details> </blockquote></details> <details> <summary>packages/nuxt/src/core/nuxt.ts (1)</summary><blockquote> <details> <summary>packages/nuxt/src/core/server.ts (1)</summary> * `bundleServer` (5-21) </details> </blockquote></details> </details> </details> <details> <summary>🔇 Additional comments (14)</summary><blockquote> <details> <summary>packages/nuxt/src/core/plugins/import-protection.ts (1)</summary><blockquote> `16-16`: **LGTM! Function declaration refactor is appropriate.** The conversion from a const arrow function to a traditional function declaration is a harmless stylistic change. The function signature and body remain unchanged, and whilst this alters hoisting semantics, there are no references to the function within the module that would be affected. </blockquote></details> <details> <summary>packages/nitro-server/src/runtime/utils/renderer/build-files.ts (1)</summary><blockquote> `1-1`: **Correct import entry for vue-bundle-renderer** Importing createRenderer from 'vue-bundle-renderer/runtime' is the right entry. LGTM. Based on learnings </blockquote></details> <details> <summary>packages/nitro-server/.gitignore (1)</summary><blockquote> `1-2`: **LGTM!** Appropriate .gitignore entries for generated error templates. </blockquote></details> <details> <summary>packages/nitro-server/.attw.json (1)</summary><blockquote> `1-3`: **LGTM!** Appropriate configuration for handling CommonJS/ESM interoperability in the new package. </blockquote></details> <details> <summary>packages/nitro-server/src/runtime/handlers/error.ts (1)</summary><blockquote> `4-4`: **LGTM!** Correctly updates type import to use the public `nuxt/app` API instead of internal aliases, aligning with the package extraction goals. </blockquote></details> <details> <summary>packages/nitro-server/src/runtime/utils/renderer/app.ts (1)</summary><blockquote> `4-4`: **LGTM!** Type import correctly updated to use the public `nuxt/app` API. </blockquote></details> <details> <summary>packages/nitro-server/src/utils.ts (2)</summary><blockquote> `4-6`: **LGTM!** Simple, type-safe utility for normalising values to arrays. --- `8-11`: **LGTM!** Correctly computes the distribution directory, accounting for bundler output structures such as `dist/chunks` or `dist/shared`. </blockquote></details> <details> <summary>packages/ui-templates/lib/render.ts (1)</summary><blockquote> `203-208`: **LGTM!** Correctly redirects the error-500 template from Nuxt runtime to Nitro runtime templates, aligning with the package extraction strategy. </blockquote></details> <details> <summary>packages/nitro-server/build.config.ts (1)</summary><blockquote> `4-21`: **LGTM!** Well-structured build configuration for the extracted Nitro server package. The declaration generation, runtime entry separation, and external dependencies are all appropriate. </blockquote></details> <details> <summary>packages/nitro-server/src/runtime/handlers/island.ts (1)</summary><blockquote> `10-10`: **LGTM!** Correctly updates island types to use the public `nuxt/app` API, ensuring consistency across the refactored package. </blockquote></details> <details> <summary>packages/nuxt/types.d.ts (1)</summary><blockquote> `1-1`: **Verify Nitro typings are provided via `@nuxt/nitro-server` templates.** Removing Nuxt-side nitropack augmentations shifts responsibility. Ensure the new package generates the necessary module augmentations (e.g. render:html hooks, route rules, runtime config) for consumers. To confirm, check template generation in `packages/nitro-server/src/templates.ts` and the resulting `.nuxt/types` output includes nitro augmentations. </blockquote></details> <details> <summary>packages/nitro-server/src/runtime/handlers/renderer.ts (1)</summary><blockquote> `15-15`: **Type import alignment looks good.** Switching `NuxtRenderHTMLContext` to `nuxt/app` matches the new typing surface. No runtime impact. </blockquote></details> <details> <summary>packages/nuxt/src/core/templates.ts (1)</summary><blockquote> `14-14`: **Approve removal of unused TSReference import** No TSReference usage remains in this file; the import update is correct. </blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/nuxt/src/app/types.ts (1)
9-12: Missinghtmlfield (previously flagged).This issue was already raised in a previous review: the
NuxtIslandSlotResponseinterface is missing thehtmlfield, which creates type inconsistency with how slots are used elsewhere. Please refer to the previous review comment for the suggested fix.
🧹 Nitpick comments (2)
packages/nitro-server/src/augments.ts (1)
36-66: Duplication and manual sync requirement increase maintenance burden.Similar to the first augmentation block, these interfaces are duplicated between
'nitropack'and'nitropack/types'. Additionally, the comment on line 36 introduces a manual synchronisation requirement withpackages/nuxt/src/core/templates.ts, which creates risk of drift if changes are made in one location but not the other.Consider:
- Applying the same DRY refactor suggested for lines 8-34 to extract shared definitions
- Investigating whether these augmentations could be generated from a single source of truth (e.g., via a build-time template) to eliminate the manual sync requirement
For the manual sync concern, you could add automated verification:
#!/bin/bash # Check for consistency between augments.ts and core/templates.ts echo "=== Checking for NitroRuntimeConfig in templates.ts ===" rg -n "interface NitroRuntimeConfig" packages/nuxt/src/core/templates.ts -A 5 || echo "Not found in templates.ts" echo -e "\n=== Comparing NitroRouteConfig definitions ===" echo "In augments.ts:" rg -n "interface NitroRouteConfig" packages/nitro-server/src/augments.ts -A 5 echo -e "\nIn templates.ts:" rg -n "interface NitroRouteConfig" packages/nuxt/src/core/templates.ts -A 5 || echo "Not found in templates.ts"packages/nuxt/src/app/types.ts (1)
14-19: Consider stricter typing forprops.The
props: unknowntype at line 16 is quite permissive. Consider usingprops?: Record<string, any>or a more specific type to improve type safety and developer experience whilst maintaining flexibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/nitro-server/package.jsonis excluded by!**/package.json
📒 Files selected for processing (5)
docs/3.api/6.nuxt-config.md(2 hunks)packages/nitro-server/src/augments.ts(1 hunks)packages/nuxt/src/app/types.ts(1 hunks)packages/nuxt/src/core/server.ts(1 hunks)packages/schema/src/config/nitro.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Follow standard TypeScript conventions and best practices
Files:
packages/schema/src/config/nitro.tspackages/nuxt/src/core/server.tspackages/nuxt/src/app/types.tspackages/nitro-server/src/augments.ts
🧬 Code graph analysis (2)
packages/nuxt/src/core/server.ts (1)
packages/nitro-server/src/index.ts (1)
bundle(37-766)
packages/nitro-server/src/augments.ts (1)
packages/nuxt/src/app/types.ts (3)
NuxtRenderHTMLContext(39-46)NuxtIslandResponse(30-37)NuxtIslandContext(21-28)
⏰ 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). (15)
- GitHub Check: test-fixtures (windows-latest, built, vite, default, manifest-off, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, dev, vite, async, manifest-off, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, vite, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, vite, async, manifest-on, js, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, webpack, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite, async, manifest-off, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite, async, manifest-on, js, lts/-1)
- GitHub Check: release-pkg-pr-new
- GitHub Check: typecheck (windows-latest, bundler)
- GitHub Check: test-benchmark
- GitHub Check: test-size
- GitHub Check: typecheck (ubuntu-latest, bundler)
- GitHub Check: code
🔇 Additional comments (6)
packages/nitro-server/src/augments.ts (2)
1-1: Past concern resolved: type-only import now used.The previous review flagged the runtime side-effect import. The current type-only syntax (
import type {} from 'nitropack/types') correctly ensures zero runtime impact whilst making the nitropack type augmentations available.
8-34: Verify necessity of duplicate augmentations in packages/nitro-server/src/augments.tsThe interfaces
NitroRuntimeConfigAppandNitroRouteRulesare declared identically for both'nitropack'and'nitropack/types'. Confirm whether'nitropack/types'genuinely exports these types; if it re-exports from the main module, drop the redundant augmentation or extract shared definitions to avoid duplication.packages/nuxt/src/app/types.ts (4)
1-1: LGTM!The import of
SerializableHeadfrom@unhead/vueis correct and properly used in theNuxtIslandResponseinterface.
21-28: Verify theOmitpattern for slots.Line 26 omits only
'fallback'fromNuxtIslandSlotResponse, leaving just thepropsfield. Please confirm this is the intended design, especially given thatNuxtIslandClientResponsecomponents (line 27) omit'html'. This asymmetry may be intentional, but worth verifying for consistency.
30-37: LGTM!The interface correctly uses
SerializableHeadand has a well-structured response shape for island rendering.
39-46: LGTM!The render context interface is well-structured with appropriate string array types for HTML rendering.
| $resolve: (val) => { | ||
| if (typeof val === 'string') { | ||
| return val | ||
| } | ||
| if (val && typeof val === 'object' && 'bundle' in val) { | ||
| return val | ||
| } | ||
| return '@nuxt/nitro-server' |
There was a problem hiding this comment.
Validate custom builder objects before accepting them.
The resolver now accepts any object that merely has a bundle key. If a user provides { bundle: 'foo' } (easy to do in JS config), schema will pass it through, but when bundleServer later executes await bundle(nuxt) it will explode with “bundle is not a function”. Please gate this branch on typeof val.bundle === 'function' (and otherwise fall back to the default or throw) so we fail fast and predictably.
🤖 Prompt for AI Agents
In packages/schema/src/config/nitro.ts around lines 6 to 13, the resolver
currently accepts any object that contains a bundle key which may be a
non-function and later causes runtime errors; update the branch so it only
accepts objects where typeof val.bundle === 'function' (otherwise return the
default '@nuxt/nitro-server' or throw), i.e. validate that val is an object with
a callable bundle before returning it to ensure fast, predictable failure.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/nuxt/src/app/types.ts (1)
9-12: Assess whetherhtmlfield is required inNuxtIslandSlotResponse.A previous review identified that
htmlmay be missing fromNuxtIslandSlotResponse. Currently, onlypropsandfallbackare defined. Verify whether slots require anhtmlfield based on runtime usage in the Nitro server handlers that consume these types.Run the following script to check how
NuxtIslandSlotResponseis used in the codebase:#!/bin/bash # Description: Search for usage of NuxtIslandSlotResponse and check if html property is accessed # Search for NuxtIslandSlotResponse usage rg -nP --type=ts -C5 'NuxtIslandSlotResponse' # Search for slot.html or slots.*?.html patterns rg -nP --type=ts -C3 '\bslots?\[[^\]]+\]\.html\b|\bslots?\.[a-zA-Z_$][a-zA-Z0-9_$]*\.html\b'
🧹 Nitpick comments (3)
packages/schema/src/config/nitro.ts (1)
4-16: Consider adding runtime validation for the bundle property.The resolver correctly preserves object builders with a
bundleproperty, addressing the previous concern. However, it doesn't validate thatbundleis actually a function, which could allow invalid objects like{ bundle: 'not-a-function' }to pass through.Whilst TypeScript types should prevent this at compile time, adding a runtime check would provide an extra safety layer and clearer error messages:
server: { builder: { $resolve: (val) => { if (typeof val === 'string') { return val } - if (val && typeof val === 'object' && 'bundle' in val) { + if (val && typeof val === 'object' && 'bundle' in val && typeof (val as any).bundle === 'function') { return val } return '@nuxt/nitro-server' }, }, },Alternatively, if you want to provide a clearer error for invalid configurations:
$resolve: (val) => { if (typeof val === 'string') { return val } if (val && typeof val === 'object' && 'bundle' in val) { if (typeof (val as any).bundle !== 'function') { throw new TypeError('server.builder.bundle must be a function') } return val } return '@nuxt/nitro-server' }packages/nuxt/src/app/types.ts (2)
24-24: Consider using a more specific type forprops.The
propsfield usesRecord<string, any>, which bypasses type safety. If the prop structure is known or can be constrained, consider usingRecord<string, unknown>or a more specific type to improve type safety.Apply this diff if
unknownis acceptable:- props?: Record<string, any> + props?: Record<string, unknown>
34-34: Consider using a more specific type for nestedprops.The nested
propsfield usesRecord<string, Record<string, any>>, which weakens type safety. If the structure can be constrained, consider usingRecord<string, Record<string, unknown>>for better type safety.Apply this diff if
unknownis acceptable:- props?: Record<string, Record<string, any>> + props?: Record<string, Record<string, unknown>>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/nitro-server/package.jsonis excluded by!**/package.json
📒 Files selected for processing (5)
docs/3.api/6.nuxt-config.md(2 hunks)packages/nitro-server/src/augments.ts(1 hunks)packages/nuxt/src/app/types.ts(1 hunks)packages/nuxt/src/core/server.ts(1 hunks)packages/schema/src/config/nitro.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/nuxt/src/core/server.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Follow standard TypeScript conventions and best practices
Files:
packages/nitro-server/src/augments.tspackages/nuxt/src/app/types.tspackages/schema/src/config/nitro.ts
🧬 Code graph analysis (2)
packages/nitro-server/src/augments.ts (1)
packages/nuxt/src/app/types.ts (3)
NuxtRenderHTMLContext(39-46)NuxtIslandResponse(30-37)NuxtIslandContext(21-28)
packages/nuxt/src/app/types.ts (1)
packages/nuxt/src/app/index.ts (4)
NuxtAppLiterals(14-14)NuxtIslandContext(14-14)NuxtIslandResponse(14-14)NuxtRenderHTMLContext(14-14)
⏰ 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). (16)
- GitHub Check: test-fixtures (windows-latest, built, rspack, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, vite, async, manifest-off, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, built, vite, default, manifest-off, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, dev, vite, async, manifest-off, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, dev, vite, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, built, vite, async, manifest-off, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, vite, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (windows-latest, dev, vite, default, manifest-off, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, vite, async, manifest-on, js, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, built, webpack, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite, async, manifest-off, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite, async, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite, default, manifest-on, json, lts/-1)
- GitHub Check: test-fixtures (ubuntu-latest, dev, vite, async, manifest-on, js, lts/-1)
- GitHub Check: typecheck (windows-latest, bundler)
- GitHub Check: test-benchmark
🔇 Additional comments (4)
docs/3.api/6.nuxt-config.md (2)
427-480: Excellent documentation with comprehensive examples.The expanded
builderdocumentation is clear and well-structured, providing:
- Type information with all supported builder options
- Examples for each built-in builder (vite, webpack, rspack)
- Clear guidance on custom builder objects with the
bundlefunction- Instructions for creating custom builder packages
The progression from simple to complex use cases makes this easy to follow.
1272-1287: Clear documentation with appropriate cautionary guidance.The
server.builderdocumentation is concise and includes:
- Accurate type definition matching the implementation
- Clear explanation of the default behaviour
- Appropriate warning callout about the API being internal and not finalised
The warning callout (lines 1285-1287) is particularly valuable as it sets clear expectations for users considering this option.
packages/nuxt/src/app/types.ts (2)
39-46: LGTM!The
NuxtRenderHTMLContextinterface is well-structured and clearly represents the different sections of HTML rendering context.
1-1: Dependency@unhead/vueverified: declared inpackages/nuxt/package.json(version ^2.0.14).
📚 Description
in preparation for the move to nitro v3 (and also supporting nitro as a vite plugin), this extracts the nitro server implementation in to a separate and configurable
@nuxt/nitro-serverpackage.the idea would be we can allow people to override this package to test nitro v3 early, and we can also disable it automatically (from the vite builder) when enabling nitro as a vite plugin. at that point we will also need to refactor this implementation so we can keep nitro configuration in sync between the nitro-server + vite-builder.
I'm still thinking about the right api for configuring this.
server.builderwith abundleexport might not be right, and I'd welcome thoughts from the rest of the team. 🙏cc: @pi0
🚧 TODO