feat(kit): add forward-compatible nitro types#34036
Conversation
|
|
@nuxt/kit
@nuxt/nitro-server
nuxt
@nuxt/rspack-builder
@nuxt/schema
@nuxt/vite-builder
@nuxt/webpack-builder
commit: |
WalkthroughAdds 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: 1
🤖 Fix all issues with AI Agents
In @packages/kit/src/nitro.ts:
- Around line 28-30: The push uses an unsafe cast "as any" in addServerHandler
when calling normalizeHandlerMethod; remove the cast and instead give
normalizeHandlerMethod an explicit return type that reflects the added method
property (e.g. NitroEventHandler & { method?: Uppercase<HANDLER_METHOD_RE> }) or
define a dedicated EnrichedHandler type and update the type of
useNuxt().options.serverHandlers to accept that enriched handler; then push the
normalized handler without casting and update any call sites or imports
referencing normalizeHandlerMethod, NitroEventHandler, serverHandlers, or
HANDLER_METHOD_RE accordingly.
🧹 Nitpick comments (1)
packages/kit/src/nitro.ts (1)
8-9: Consider renaming the type to avoid naming collision.Both the regex constant and the type alias share the name
HANDLER_METHOD_RE. Whilst TypeScript allows this (values and types occupy separate namespaces), it can be confusing for maintainability and readability.Consider renaming the type to something like
HTTPMethodorHandlerMethodto make the distinction clearer.🔎 Proposed refactor
const HANDLER_METHOD_RE = /\.(get|head|patch|post|put|delete|connect|options|trace)(\.\w+)*$/ -type HANDLER_METHOD_RE = 'get' | 'head' | 'patch' | 'post' | 'put' | 'delete' | 'connect' | 'options' | 'trace' +type HTTPMethod = 'get' | 'head' | 'patch' | 'post' | 'put' | 'delete' | 'connect' | 'options' | 'trace'Then update line 18:
- method: method?.toUpperCase() as Uppercase<HANDLER_METHOD_RE> | undefined, + method: method?.toUpperCase() as Uppercase<HTTPMethod> | undefined,
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/kit/package.jsonis excluded by!**/package.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!pnpm-lock.yaml
📒 Files selected for processing (4)
packages/kit/src/nitro-types.tspackages/kit/src/nitro.tspackages/kit/src/pages.tspackages/kit/src/runtime-config.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Follow standard TypeScript conventions and best practices
Files:
packages/kit/src/pages.tspackages/kit/src/runtime-config.tspackages/kit/src/nitro-types.tspackages/kit/src/nitro.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx,vue}: Use clear, descriptive variable and function names
Add comments only to explain complex logic or non-obvious implementations
Keep functions focused and manageable (generally under 50 lines), and extract complex logic into separate domain-specific files
Remove code that is not used or needed
Use error handling patterns consistently
Files:
packages/kit/src/pages.tspackages/kit/src/runtime-config.tspackages/kit/src/nitro-types.tspackages/kit/src/nitro.ts
🧠 Learnings (1)
📚 Learning: 2024-12-12T12:36:34.871Z
Learnt from: huang-julien
Repo: nuxt/nuxt PR: 29366
File: packages/nuxt/src/app/components/nuxt-root.vue:16-19
Timestamp: 2024-12-12T12:36:34.871Z
Learning: In `packages/nuxt/src/app/components/nuxt-root.vue`, when optimizing bundle size by conditionally importing components based on route metadata, prefer using inline conditional imports like:
```js
const IsolatedPage = route?.meta?.isolate ? defineAsyncComponent(() => import('#build/isolated-page.mjs')) : null
```
instead of wrapping the import in a computed property or importing the component unconditionally.
Applied to files:
packages/kit/src/pages.ts
🧬 Code graph analysis (1)
packages/kit/src/nitro.ts (1)
packages/kit/src/context.ts (1)
useNuxt(30-37)
⏰ 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). (4)
- GitHub Check: code
- GitHub Check: build
- GitHub Check: codeql (javascript-typescript)
- GitHub Check: codeql (actions)
🔇 Additional comments (7)
packages/kit/src/nitro-types.ts (2)
6-10: Conditional type exports look correct.The conditional type pattern correctly provides a unified interface for Nitro types across v2 and v3. Assuming the version detection mechanism is validated, this approach should provide seamless forward compatibility.
1-4: This forward-compatible type detection pattern is intentional and well-designed for bridging Nitro v2 (nitropack) to v3 (nitro) during the framework transition (planned for Nuxt 5). The conditional type'options' extends keyof NitroV2.Nitro ? true : falseis a compile-time TypeScript check, not runtime code, and correctly resolves during type-checking based on which package's types are available. The 'options' property is a legitimate distinguishing feature between the two versions' APIs. Import-time errors for missing dependencies are the expected and appropriate behaviour. This is not a fragile edge case; it's an intentional forward-compatibility bridge with proper error handling.Likely an incorrect or invalid review comment.
packages/kit/src/runtime-config.ts (2)
9-9: Import change aligns with centralized type surface.The import of
NitroRuntimeConfigfrom the localnitro-typesmodule correctly consolidates type dependencies for forward compatibility.
33-33: Type cast for Nitro compatibility is appropriate.The cast to
NitroRuntimeConfigensures type compatibility with the Nitro instance'supdateConfigmethod. SinceruntimeConfigis already typed asRecord<string, unknown>(from the function parameter), this cast provides the necessary type refinement without runtime changes.packages/kit/src/pages.ts (1)
6-6: Import refactoring correctly centralizes type dependencies.The change from
nitropack/typesto the local./nitro-types.tsmodule aligns with the PR's goal of providing forward-compatible type definitions across Nitro versions.packages/kit/src/nitro.ts (2)
5-5: Type imports correctly use centralized module.The import change aligns with the new forward-compatible type surface defined in
nitro-types.ts.
14-22: Uppercase normalization aligns with Nitro v3 behaviour.The change to uppercase HTTP method names correctly prepares for Nitro v3, where uppercase will be the default. The type cast to
Uppercase<HANDLER_METHOD_RE>provides proper type safety for the transformation.
CodSpeed Performance ReportMerging #34036 will not alter performanceComparing Summary
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/kit/src/nitro-types.ts (1)
4-8: Consider adding documentation for the type detection logic.The
___INVALIDkey check guards against unresolved type imports, which is clever but relies on TypeScript's internal behaviour. Consider adding a comment explaining this rationale to help future maintainers understand the purpose of this guard.Additionally, the detection logic assumes only one package is installed. If both
nitroandnitropackare present, it will default to v2. If neither is installed, it defaults to v3, which might produce confusing error messages.📝 Suggested documentation
+// Detect Nitro v2 by checking for the 'options' key in the Nitro type. +// The ___INVALID check guards against unresolved type imports, ensuring +// we don't mistakenly treat a failed import as Nitro v2. type isNitroV2 = 'options' extends keyof NitroV2.Nitro ? '___INVALID' extends keyof NitroV2.Nitro ? false : true : false
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
packages/kit/package.jsonis excluded by!**/package.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!pnpm-lock.yaml
📒 Files selected for processing (1)
packages/kit/src/nitro-types.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,vue}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Follow standard TypeScript conventions and best practices
Files:
packages/kit/src/nitro-types.ts
**/*.{ts,tsx,js,jsx,vue}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx,vue}: Use clear, descriptive variable and function names
Add comments only to explain complex logic or non-obvious implementations
Keep functions focused and manageable (generally under 50 lines), and extract complex logic into separate domain-specific files
Remove code that is not used or needed
Use error handling patterns consistently
Files:
packages/kit/src/nitro-types.ts
🔇 Additional comments (2)
packages/kit/src/nitro-types.ts (2)
10-14: Clean compatibility layer implementation.The conditional type exports provide a clean, unified API that automatically adapts to whichever Nitro version is installed. The pattern is consistent and straightforward, making it easy to maintain and extend if additional types need to be exposed in the future.
1-2: Type imports and version detection are correctly implemented.The imports are properly structured for type-only access, and the versions (nitropack 2.12.9 and nitro 3.0.1-alpha.1) exist with the expected type structures. The
___INVALIDkey detection is a documented TypeScript pattern used to distinguish resolved from unresolved types, making it a valid mechanism for detecting which version is installed. Theoptionskey exists in nitropack v2's Nitro type as expected. No changes required.
🔗 Linked issue
📚 Description
this is a forward-compatible update to kit which automatically prefer types for whichever of nitro/nitropack the user has installed
in addition, we now normalise to uppercase handler methods (as they are normalised safely to lower case here and uppercase will be the new default for nitro v3)