feat: express decorators for rest apis and websocket#6818
feat: express decorators for rest apis and websocket#6818sriramveeraghanta merged 4 commits intopreviewfrom
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThis pull request introduces a new package, Changes
Sequence Diagram(s)sequenceDiagram
participant Router
participant ControllerInstance
participant ReflectAPI
Router->>ControllerInstance: Invoke registerControllers()
ControllerInstance->>ReflectAPI: Retrieve metadata (base route, HTTP method, etc.)
ReflectAPI-->>ControllerInstance: Return metadata
ControllerInstance->>Router: Register route with handler
sequenceDiagram
participant Router
participant WSControllerInstance
participant ReflectAPI
Router->>WSControllerInstance: Invoke registerWebSocketControllers()
WSControllerInstance->>ReflectAPI: Retrieve WebSocket metadata
ReflectAPI-->>WSControllerInstance: Return metadata
WSControllerInstance->>Router: Register WebSocket route with handler
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🪧 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 (
|
Palanikannan1437
force-pushed
the
feat/decorators
branch
3 times, most recently
from
2660a11 to
f4d8103
Compare
Palanikannan1437
force-pushed
the
feat/decorators
branch
from
f4d8103 to
ea7ebe6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (11)
packages/decorators/tsconfig.json (1)
9-9: Remove trailing comma in object literal.There's a trailing comma after the closing bracket of the "lib" array in the compilerOptions object. While this is valid JSON in some contexts, it's technically not valid in the strictest JSON parsing and might cause issues with some tools.
"compilerOptions": { "jsx": "react", "lib": [ "esnext", "dom" ], - }, + }packages/decorators/src/websocket.ts (3)
3-7: Complete the JSDoc comment with return type description.The JSDoc comment for the WebSocket decorator is incomplete. It lists the route parameter but doesn't describe what the function returns.
/** * WebSocket method decorator * @param route + * @returns MethodDecorator - A decorator that can be applied to class methods */
8-17: Add route parameter validation.The WebSocket decorator doesn't validate the route parameter. Adding validation would help catch potential issues early and provide better error messages.
export function WebSocket(route: string): MethodDecorator { + if (!route || typeof route !== 'string') { + throw new Error('WebSocket decorator requires a valid route string'); + } + return function ( target: object, propertyKey: string | symbol, descriptor: PropertyDescriptor, ) { Reflect.defineMetadata("method", "ws", target, propertyKey); Reflect.defineMetadata("route", route, target, propertyKey); }; }
14-15: Consider using constants for metadata keys.Using string literals directly for metadata keys could lead to inconsistencies if the same keys are used elsewhere. Consider defining constants for these keys.
+const METHOD_METADATA_KEY = "method"; +const ROUTE_METADATA_KEY = "route"; +const WS_METHOD_TYPE = "ws"; export function WebSocket(route: string): MethodDecorator { return function ( target: object, propertyKey: string | symbol, descriptor: PropertyDescriptor, ) { - Reflect.defineMetadata("method", "ws", target, propertyKey); - Reflect.defineMetadata("route", route, target, propertyKey); + Reflect.defineMetadata(METHOD_METADATA_KEY, WS_METHOD_TYPE, target, propertyKey); + Reflect.defineMetadata(ROUTE_METADATA_KEY, route, target, propertyKey); }; }packages/decorators/README.md (1)
1-100: README provides good documentation but needs clarification on setup requirements.The documentation is well-structured with clear examples of both REST and WebSocket controller usage. However, there are some areas that could be improved:
The "No build step required" feature (line 11) may be misleading since TypeScript files still need transpilation.
The WebSocket example (lines 74-76) uses
express-wswithout showing how to set it up:// Register WebSocket routes const router = require("express-ws")(app).router; const chatController = new ChatController(); chatController.registerWebSocketRoutes(router);Consider adding:
- Prerequisites section mentioning the need for TypeScript configuration
- Complete setup example including express-ws initialization
- Instructions for enabling decorator metadata in tsconfig.json
packages/decorators/src/index.ts (1)
13-14: Consider consistent naming convention for namespaces.The namespace naming is inconsistent:
Restis used for REST decoratorsWebSocketNSis used for WebSocket decorators (with an "NS" suffix)For better consistency, consider either:
-export const Rest = RestDecorators; -export const WebSocketNS = WebSocketDecorators; +export const RestNS = RestDecorators; +export const WebSocketNS = WebSocketDecorators;Or:
-export const Rest = RestDecorators; -export const WebSocketNS = WebSocketDecorators; +export const Rest = RestDecorators; +export const WebSocket = WebSocketDecorators;packages/decorators/src/controller.ts (2)
18-21: Refine constructor parameter typing.
Currently,new (...args: any[])uses very broadanytyping. This makes the code less self-documenting and can mask potential type mismatch errors. Consider introducing a generic typed constructor signature or a more descriptive interface to improve maintainability.
23-61: Validate consistency between REST and WebSocket registration.
This function explicitly skips"ws"methods. Ensure that all relevant WebSocket routes are registered and tested by the dedicatedregisterWebSocketControllersfunction, or consider unifying the approach if you'd prefer to manage both protocols in a single method.packages/decorators/src/rest.ts (1)
20-93: Consolidate repetitive decorator logic.
TheGet,Post,Patch,Put, andDeletedecorators use nearly identical logic. Consider introducing a higher-order function that takes the HTTP method as an argument to reduce duplication. This can help you easily add new methods and keep the code consistent.packages/decorators/src/websocket-controller.ts (2)
43-55: Refine error handling for WebSocket exceptions.
Currently, errors are logged to the console and the connection is closed with code1011. You might enhance user feedback by implementing a structured error logger or returning more contextual error messages to the client when appropriate.
63-85: Review instantiation of the router in BaseWebSocketController.
You create a freshRouter()instance in the constructor. Confirm whether you intend to attach WebSocket routes to this controller-specific router or a shared application router, to avoid losing track of your WebSocket endpoints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (11)
packages/decorators/.eslintignore(1 hunks)packages/decorators/.eslintrc.js(1 hunks)packages/decorators/README.md(1 hunks)packages/decorators/package.json(1 hunks)packages/decorators/src/controller.ts(1 hunks)packages/decorators/src/index.ts(1 hunks)packages/decorators/src/rest.ts(1 hunks)packages/decorators/src/websocket-controller.ts(1 hunks)packages/decorators/src/websocket.ts(1 hunks)packages/decorators/tsconfig.json(1 hunks)packages/decorators/tsup.config.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
packages/decorators/src/websocket-controller.ts (3)
packages/decorators/src/index.ts (3)
registerWebSocketControllers(6-6)Controller(2-2)WebSocket(4-4)packages/decorators/src/rest.ts (1)
Controller(9-13)packages/decorators/src/websocket.ts (1)
WebSocket(8-17)
🪛 Biome (1.9.4)
packages/decorators/src/rest.ts
[error] 10-10: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
🔇 Additional comments (8)
packages/decorators/tsconfig.json (1)
1-18: TypeScript configuration appears appropriate, but verify React dependency.The TypeScript configuration properly extends from the base configuration and sets up the compiler options. However, it includes React-specific settings (
"jsx": "react") which raises a question - is this package intended to be used with React components, or is it strictly for Express decorators as suggested by the PR title?Please verify if the React JSX setting is necessary for this package. If this package doesn't contain React components but is only for Express decorators, you might consider removing the JSX setting.
packages/decorators/.eslintignore (1)
1-4: LGTM! Standard ESLint ignore patterns.The ESLint ignore file correctly excludes common directories that shouldn't be linted: node_modules, build artifacts, and distribution files.
packages/decorators/.eslintrc.js (1)
1-9: LGTM! Standard ESLint configuration for TypeScript.The ESLint configuration is well set up for a TypeScript library. It properly:
- Marks this as the root ESLint configuration
- Extends from a shared configuration
- Uses the TypeScript parser
- Sets up parser options for TypeScript
packages/decorators/tsup.config.ts (1)
1-12: Build configuration is well-structured and follows best practices.The tsup configuration is properly set up with:
- Both ESM and CommonJS output formats for maximum compatibility
- TypeScript declaration files enabled
- Source maps included for debugging
- External dependencies correctly specified
- Tree-shaking enabled for optimized bundle size
packages/decorators/package.json (1)
1-42: Package configuration follows best practices.The package.json is well-structured with:
- Clear package information and description
- Properly defined entry points for CommonJS, ESM, and TypeScript
- Appropriate scripts for building and linting
- Required dependencies clearly specified
The dependencies are handled correctly:
reflect-metadatais a direct dependency (needed for decorators)expressandwsare properly listed as peer dependencieswsis correctly marked as optional for users who don't need WebSocket functionalitypackages/decorators/src/index.ts (1)
1-16: Exports are well-organized with both direct and namespaced options.The index file properly exports all decorators and utilities, providing flexibility through:
- Individual exports for direct imports of specific decorators
- Namespaced exports (Rest, WebSocketNS) for better organization
packages/decorators/src/controller.ts (1)
4-12: Clarify inclusion of "ws" in HttpMethod.
You include"ws"as a possible HTTP method but specifically ignore it later in the code. Verify that this is intended and that you're handling WebSocket routes elsewhere.packages/decorators/src/rest.ts (1)
100-111: Decorators and middleware chaining look good.
Attaching multiple middleware functions via metadata is an elegant approach. This logic is straightforward and should integrate well with Express’s request handling pipeline.
| if ( | ||
| typeof handler === "function" && | ||
| typeof (router as any).ws === "function" | ||
| ) { | ||
| (router as any).ws( | ||
| `${baseRoute}${route}`, | ||
| (ws: WebSocket, req: Request) => { | ||
| try { | ||
| handler.call(instance, ws, req); | ||
| } catch (error) { | ||
| console.error( | ||
| `WebSocket error in ${Controller.name}.${methodName}`, | ||
| error, | ||
| ); | ||
| ws.close( | ||
| 1011, | ||
| error instanceof Error | ||
| ? error.message | ||
| : "Internal server error", | ||
| ); | ||
| } | ||
| }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Type the WebSocket registration to avoid using (router as any).
It appears you rely on express-ws or a similar library that adds .ws to the router. Consider declaring a compatible type instead of casting to any, to ensure better type safety and autocompletion.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/typescript-config/package.json (1)
9-10: Addition of Node.js Library Configuration File
The addition of"node-library.json"in the "files" array clearly indicates that a new configuration file for Node.js library environments has been introduced. Please verify that thenode-library.jsonfile is maintained with consistent standards and documented appropriately for users who reference these configurations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/decorators/package.json(1 hunks)packages/decorators/src/websocket-controller.ts(1 hunks)packages/decorators/tsconfig.json(1 hunks)packages/typescript-config/node-library.json(1 hunks)packages/typescript-config/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/typescript-config/node-library.json
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/decorators/package.json
- packages/decorators/src/websocket-controller.ts
🔇 Additional comments (1)
packages/decorators/tsconfig.json (1)
1-22: Solid TypeScript Configuration for Decorators Package
This newtsconfig.jsonfile is well-structured. It extends from@plane/typescript-config/node-library.jsonand appropriately enables experimental decorators and metadata emission. The inclusion of compiler options such as"lib": ["ES2020"], clear path mappings, and designated include/exclude arrays ensures consistency and proper module resolution for the decorators package.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/decorators/src/rest.ts (1)
7-16:⚠️ Potential issueFix the 'Function' type usage in the Controller decorator.
The static analysis tool correctly flags the use of
Functiontype on line 13, which can be error-prone. This issue was previously addressed in commit 8a9cdc6 but appears to have been reintroduced.Replace the general
Functiontype with a more specific constructor signature:export function Controller(baseRoute: string = ""): ClassDecorator { - return function (target: Function) { + return function <T extends { new (...args: any[]): {} }>(target: T) { Reflect.defineMetadata("baseRoute", baseRoute, target); }; }This change provides better type safety by explicitly defining the function shape.
🧰 Tools
🪛 Biome (1.9.4)
[error] 13-13: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
🧹 Nitpick comments (2)
packages/decorators/src/rest.ts (2)
7-11: Complete the JSDoc comments for better documentation.The JSDoc comments for the
Controllerdecorator are incomplete - they have@paramand@returnstags but no descriptions.Improve the JSDoc by adding descriptions:
/** * Controller decorator - * @param baseRoute - * @returns + * @param baseRoute - Base route prefix for all methods in the controller + * @returns A class decorator that adds route metadata to the controller */
45-49: Complete the JSDoc comments for the Middleware decorator.Similar to the Controller decorator, the JSDoc comments for the
Middlewaredecorator are incomplete.Improve the documentation:
/** * Middleware decorator - * @param middleware - * @returns + * @param middleware - Express middleware function to be applied to the route handler + * @returns A method decorator that adds middleware metadata to the handler method */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/decorators/src/rest.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
packages/decorators/src/rest.ts (1)
packages/decorators/src/index.ts (7)
Controller(2-2)Get(3-3)Post(3-3)Put(3-3)Patch(3-3)Delete(3-3)Middleware(2-2)
🪛 Biome (1.9.4)
packages/decorators/src/rest.ts
[error] 13-13: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
🔇 Additional comments (4)
packages/decorators/src/rest.ts (4)
18-36: Good implementation of a factory pattern for HTTP method decorators.The
createHttpMethodDecoratorfactory function is well implemented, reducing code duplication while maintaining a clean API. The function creates decorators for different HTTP methods with consistent behavior.
38-43: Clean export of HTTP method decorators.The HTTP method decorators are created using the factory function and exported with clear, concise names that follow REST API conventions.
50-61: Well-implemented Middleware decorator.The
Middlewaredecorator correctly retrieves existing middlewares, appends new ones, and updates the metadata. This approach allows for multiple middleware functions to be applied to a single handler method.
1-5: Good type definition for HTTP methods.The
RestMethodtype definition clearly defines the valid HTTP methods that can be used with the decorators, which helps with type safety.
* feat: express decorators for rest apis and websocket * fix: added package dependency * fix: refactor decorators
3 participants
Description
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Documentation
Chores