-
-
Notifications
You must be signed in to change notification settings - Fork 764
feat(renderer): allow disabling renderer #3822
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
@kricsleo is attempting to deploy a commit to the Nitro Team on Vercel. A member of the Team first needs to authorize it. |
π WalkthroughWalkthroughThe changes implement support for disabling the renderer in Nitro configuration. A new optional property is added to the NitroConfig type allowing renderer to be set to Changes
Estimated code review effortπ― 3 (Moderate) | β±οΈ ~20 minutes
Pre-merge checks and finishing touchesβ Failed checks (1 warning)
β Passed checks (4 passed)
β¨ Finishing touches
π§ͺ Generate unit tests (beta)
π Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro π Files selected for processing (2)
π§ Files skipped from review as they are similar to previous changes (2)
β° 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). (2)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
π§Ή Nitpick comments (1)
src/build/vite/plugin.ts (1)
88-111: LGTM! Client environment input correctly respects renderer configuration.The cached nitro reference and conditional logic correctly prevent setting a client input when the renderer is explicitly disabled.
Minor consistency suggestion: Line 120 calls
useNitro(ctx)again rather than using the cachednitroreference from Line 94. While this doesn't cause issues, using the cached value would be more consistent.Optional refactor for consistency:
environments.client = { consumer: userConfig.environments?.client?.consumer ?? "client", build: { rollupOptions: { input: userConfig.environments?.client?.build?.rollupOptions?.input ?? (nitro.options.renderer === false ? undefined : nitro.options.renderer?.template), }, }, }; debug("[env] Environments:", Object.keys(environments).join(", ")); return { environments, }; }, configEnvironment(name, config) { if (config.consumer === "client") { debug( "[env] Configuring client environment", name === "client" ? "" : ` (${name})` ); config.build!.emptyOutDir = false; - config.build!.outDir = useNitro(ctx).options.output.publicDir; + config.build!.outDir = nitro.options.output.publicDir; } else {
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
π Files selected for processing (6)
src/build/virtual/renderer-template.ts(1 hunks)src/build/vite/plugin.ts(2 hunks)src/build/vite/prod.ts(1 hunks)src/config/resolvers/paths.ts(1 hunks)src/routing.ts(1 hunks)src/types/config.ts(1 hunks)
π§° Additional context used
𧬠Code graph analysis (1)
src/config/resolvers/paths.ts (2)
src/utils/fs.ts (2)
resolveNitroPath(14-32)prettyPath(9-12)src/runtime/meta.ts (1)
runtimeDir(10-10)
π Additional comments (6)
src/types/config.ts (1)
86-86: LGTM! Clean type extension.The type change correctly extends
rendererto acceptfalseas a value, enabling the feature to disable the renderer while preserving backward compatibility for undefined and object values.src/routing.ts (1)
75-81: LGTM! Correct guard implementation.The conditional correctly gates renderer route registration on
renderer !== false, ensuring the renderer route is only added when the renderer is enabled and a handler exists.src/build/vite/prod.ts (1)
55-77: LGTM! Template transformation correctly gated.The guard ensures template transformation is skipped when
rendereris explicitly disabled, while preserving the existing behavior for enabled or undefined renderer configurations.src/build/virtual/renderer-template.ts (1)
13-23: LGTM! Clear renderer disablement logic.The guard logic correctly handles the disabled renderer case by setting template to
undefinedwhenrenderer === falseand returning a no-template response. The explicit check on Line 17 forrenderer === falseadds clarity even though it's technically covered by thetypeof template !== "string"check.src/build/vite/plugin.ts (1)
393-405: LGTM! Default SSR renderer correctly gated.The guard ensures the default SSR renderer is only activated when the renderer is not explicitly disabled, while preserving the existing conditional logic for handler and template absence.
src/config/resolvers/paths.ts (1)
136-180: LGTM! Comprehensive renderer resolution gating.The guard at Line 136 correctly wraps the entire renderer resolution logic, ensuring no renderer-related processing occurs when explicitly disabled. The internal logic flow is preserved:
- Handler resolution (Lines 138-146)
- Template resolution with default fallback (Lines 149-170)
- Default handler assignment when template exists (Lines 173-179)
The distinction between explicit
falseandundefinedis correctly maintained, preserving default behavior when renderer is not explicitly disabled.
commit: |
src/types/config.ts
Outdated
| database: DatabaseConnectionConfigs; | ||
| devDatabase: DatabaseConnectionConfigs; | ||
| renderer?: { handler?: string; static?: boolean; template?: string }; | ||
| renderer?: { handler?: string; static?: boolean; template?: string } | false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional false value can be overridden in NitroConfig (input) interface to explicitly ask nitro do not (auto) detect renderer.template. This way we can keep other places as before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated π
a5f4b2b to
f4f6ff0
Compare
π Linked issue
resolves #3818
β Type of change
π Description
This PR enables disabling the default renderer feature using
renderer: false.(I didn't update the docs, seems like the doc is still WIP :)
π Checklist