fix(schema): fix typing for operation decorators #2973
Conversation
WalkthroughThis pull request focuses on refining TypeScript decorators across multiple files in the Changes
Possibly related PRs
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (3)
packages/specs/schema/src/decorators/operations/summary.ts (1)
Line range hint
1-23: Fix incorrect example in documentation.The JSDoc example shows the
@Summarydecorator being used on a property:class Model { @Summary("summary") id: string; }However, the implementation only supports method decoration. Please update the example to show correct method decorator usage:
class Controller { @Summary("summary") method() {} }packages/specs/schema/src/decorators/operations/consumes.ts (1)
Line range hint
8-14: Fix incorrect example in documentation.The example shows the decorator being used on a property, but the implementation only supports class and method usage.
* ## Examples * ```typescript * class Model { - * @Consumes("application/x-www-form-urlencoded") - * id: string; + * @Consumes("application/x-www-form-urlencoded") + * submit() { + * // method implementation + * } * } * ```packages/specs/schema/src/decorators/operations/operationPath.ts (1)
Line range hint
24-24: Consider enhancing type safety for path parameter.The
pathparameter could benefit from a more specific type thanstring | RegExp.-export function OperationPath(method: OperationVerbs | string, path: string | RegExp = "/") { +export function OperationPath(method: OperationVerbs | string, path: `/${string}` | RegExp = "/") {This would ensure paths always start with a forward slash when using string literals.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (19)
packages/specs/schema/src/decorators/generics/generics.ts(1 hunks)packages/specs/schema/src/decorators/operations/acceptMime.ts(1 hunks)packages/specs/schema/src/decorators/operations/consumes.spec.ts(1 hunks)packages/specs/schema/src/decorators/operations/consumes.ts(1 hunks)packages/specs/schema/src/decorators/operations/header.ts(1 hunks)packages/specs/schema/src/decorators/operations/operationId.spec.ts(1 hunks)packages/specs/schema/src/decorators/operations/operationId.ts(1 hunks)packages/specs/schema/src/decorators/operations/operationPath.ts(1 hunks)packages/specs/schema/src/decorators/operations/path.spec.ts(1 hunks)packages/specs/schema/src/decorators/operations/path.ts(1 hunks)packages/specs/schema/src/decorators/operations/produces.spec.ts(1 hunks)packages/specs/schema/src/decorators/operations/produces.ts(1 hunks)packages/specs/schema/src/decorators/operations/redirect.ts(1 hunks)packages/specs/schema/src/decorators/operations/security.spec.ts(1 hunks)packages/specs/schema/src/decorators/operations/security.ts(2 hunks)packages/specs/schema/src/decorators/operations/summary.spec.ts(1 hunks)packages/specs/schema/src/decorators/operations/summary.ts(1 hunks)packages/specs/schema/src/decorators/operations/tags.spec.ts(1 hunks)packages/specs/schema/src/decorators/operations/tags.ts(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test-specs (20.12.2)
🔇 Additional comments (16)
packages/specs/schema/src/decorators/operations/operationId.ts (1)
14-14: LGTM! Improved type safety with explicit decorator return type.The explicit
MethodDecoratorreturn type enhances type safety and makes it clear that this decorator can only be used on methods.packages/specs/schema/src/decorators/operations/operationId.spec.ts (1)
36-37: LGTM! Good error handling test coverage.The test appropriately validates that the decorator cannot be used on classes. The use of
@ts-ignoreis justified here as it's testing runtime behavior that TypeScript would normally prevent.packages/specs/schema/src/decorators/operations/summary.ts (1)
24-24: LGTM! Explicit return type improves type safety.The explicit
MethodDecoratorreturn type annotation correctly enforces that this decorator can only be used on methods.packages/specs/schema/src/decorators/operations/summary.spec.ts (1)
38-38: LGTM! Good error handling test.The
@ts-ignoreis appropriately used here to test runtime error handling for incorrect decorator usage that TypeScript would normally catch at compile time.packages/specs/schema/src/decorators/generics/generics.ts (1)
16-16: LGTM! The explicit ClassDecorator return type improves type safety.The change correctly specifies that this decorator can only be used on classes, which aligns with its purpose of defining generics at the class level.
Let's verify that this decorator is only used on classes in the codebase:
✅ Verification successful
Verified: @generics decorator is correctly used only on classes throughout the codebase
The explicit ClassDecorator return type accurately reflects the decorator's usage pattern, as evidenced by its consistent application on generic class declarations across test files, documentation, and source code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any potentially incorrect usage of @Generics decorator ast-grep --pattern 'class $_ { $$$ @Generics($$$) $_($$$) { $$$ } $$$ }'Length of output: 88
Script:
#!/bin/bash # Search for all usages of @Generics decorator echo "=== Searching for @Generics decorator usage ===" rg "@Generics" -B 1 -A 1 echo -e "\n=== Searching for Generics imports ===" rg "import.*Generics.*from"Length of output: 25276
packages/specs/schema/src/decorators/operations/path.ts (1)
19-19: LGTM! The explicit ClassDecorator return type matches runtime behavior.The type annotation correctly reflects that this decorator can only be used on classes, which is already enforced by the runtime check.
packages/specs/schema/src/decorators/operations/consumes.ts (1)
23-23: LGTM! The return type matches the implementation's behavior.The type annotation correctly reflects that this decorator can be used on both classes and methods, which aligns with the runtime switch statement.
packages/specs/schema/src/decorators/operations/produces.ts (1)
30-30: LGTM! Type safety improvement.The explicit return type
ClassDecorator & MethodDecoratoraccurately reflects the dual nature of this decorator and matches both the implementation and JSDoc documentation.packages/specs/schema/src/decorators/operations/path.spec.ts (1)
40-41: LGTM! Good test coverage.The test case properly verifies error handling for unsupported parameter decorator usage. The
@ts-ignoreis appropriately used here to test the runtime error.packages/specs/schema/src/decorators/operations/tags.ts (2)
Line range hint
19-39: LGTM! Documentation improvement.The updated documentation now properly reflects that the decorator can be used on both classes and methods, with clear examples for each use case.
48-48: LGTM! Type safety improvement.The explicit return type
ClassDecorator & MethodDecoratoraccurately reflects the dual nature of this decorator and matches the implementation.packages/specs/schema/src/decorators/operations/produces.spec.ts (1)
82-82: Consistent error handling test pattern.The test case follows the established pattern for decorator error handling, which is good for maintainability.
packages/specs/schema/src/decorators/operations/tags.spec.ts (1)
106-106: Completes error handling test coverage.The test case maintains consistency with other decorator tests and ensures proper error handling for invalid usage.
packages/specs/schema/src/decorators/operations/redirect.ts (1)
56-58: LGTM! Improved type safety withMethodDecorator.The change from
FunctiontoMethodDecoratorcorrectly restricts the decorator to method-only usage, preventing potential misuse on classes or parameters.packages/specs/schema/src/decorators/operations/security.spec.ts (1)
155-155: LGTM! Good error handling test coverage.The test appropriately validates that the
Securitydecorator cannot be used as a parameter decorator, strengthening type safety guarantees.packages/specs/schema/src/decorators/operations/operationPath.ts (1)
12-12: LGTM! Documentation grammar fix.Fixed typo in documentation ("theses" → "these").
| @@ -20,6 +20,6 @@ import {Produces} from "./produces.js"; | |||
| * @operation | |||
| * @response | |||
| */ | |||
| export function AcceptMime(...mimes: string[]): Function { | |||
| export function AcceptMime(...mimes: string[]): ClassDecorator & MethodDecorator { | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM! Consider adding @methodDecorator to JSDoc.
The return type correctly allows both class and method usage. Consider adding @methodDecorator to the JSDoc tags to match the implementation.
* @decorator
* @operation
* @response
+ * @methodDecorator
*/Committable suggestion skipped: line range outside the PR's diff.
| @@ -50,7 +50,7 @@ import {Returns} from "./returns.js"; | |||
| * @operation | |||
| * @response | |||
| */ | |||
| export function Header(headers: string | number | JsonHeaders, value?: string | number | JsonHeader): Function { | |||
| export function Header(headers: string | number | JsonHeaders, value?: string | number | JsonHeader) { | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add explicit decorator return type.
While removing the generic Function return type is good, we should specify the exact decorator type for consistency with other decorators in the codebase.
-export function Header(headers: string | number | JsonHeaders, value?: string | number | JsonHeader) {
+export function Header(headers: string | number | JsonHeaders, value?: string | number | JsonHeader): MethodDecorator {The MethodDecorator type is appropriate here as this decorator is only used on methods, as shown in the examples in the JSDoc.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function Header(headers: string | number | JsonHeaders, value?: string | number | JsonHeader) { | |
| export function Header(headers: string | number | JsonHeaders, value?: string | number | JsonHeader): MethodDecorator { |
| @@ -79,6 +79,7 @@ describe("Consumes", () => { | |||
|
|
|||
| let actualError: any; | |||
| try { | |||
| // @ts-ignore | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding type annotation for better clarity.
The test case effectively validates the error handling. Consider adding a type annotation to make the intention clearer:
-// @ts-ignore
+// @ts-ignore - Testing invalid decorator usage on parameter📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // @ts-ignore | |
| // @ts-ignore - Testing invalid decorator usage on parameter |
| @@ -25,7 +25,7 @@ import {JsonEntityFn} from "../common/jsonEntityFn.js"; | |||
| * @classDecorator | |||
| * @operation | |||
| */ | |||
| export function Security(name: string, ...scopes: string[]): Function; | |||
| export function Security(name: string, ...scopes: string[]): ClassDecorator & MethodDecorator; | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Type declarations accurately reflect decorator usage.
The return type ClassDecorator & MethodDecorator properly constrains the decorator to class and method usage, improving type safety and preventing misuse at compile time.
Also applies to: 60-61
| export function Redirect(...args: any[]): Function { | ||
| export function Redirect(url: string, meta?: JsonHeader): MethodDecorator; | ||
| export function Redirect(status: number, url: string, meta?: JsonHeader): MethodDecorator; | ||
| export function Redirect(...args: any[]): MethodDecorator { |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding type safety for the spread arguments.
The spread operator with any[] could be more strictly typed.
-export function Redirect(...args: any[]): MethodDecorator {
+export function Redirect(...args: Array<string | number | JsonHeader>): MethodDecorator {Committable suggestion skipped: line range outside the PR's diff.
|
🎉 This PR is included in version 8.4.5 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Information
Todos
Summary by CodeRabbit
Type Safety Improvements
Generics,AcceptMime,Consumes,Produces,Security, and othersError Handling
Documentation