fix(http): resolve instance handling #128 and misleading jsDOC issue#145
fix(http): resolve instance handling #128 and misleading jsDOC issue#145visionEye0 wants to merge 1 commit into
Conversation
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
VikramAditya33
left a comment
VikramAditya33
left a comment
There was a problem hiding this comment.
Looks really good to me, well done just fix the prettier formatting
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/http/platform/uws-platform.adapter.ts (2)
438-443:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPrettier check is failing on this declaration.
CI already reports formatting issues in this file; please run Prettier on this method declaration before merge.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/http/platform/uws-platform.adapter.ts` around lines 438 - 443, The addRoute method declaration in uws-platform.adapter.ts is misformatted and failing Prettier; reformat the addRoute signature and surrounding lines to conform to project Prettier rules (spacing, indentation, commas and line breaks) by running Prettier or your editor's format command on the file so the addRoute(method: string, path: string, handler: Function, metadata: RouteMetadata): void { ... } declaration matches the repository style.
438-445:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep a deprecated alias for the renamed public method.
This changes a public API surface (
registerRouteWithMetadata→addRoute) and can break existing consumers at runtime. Please keep a delegating deprecated alias for at least one release (or provide a migration path in this PR).Suggested compatibility shim
+ /** + * `@deprecated` Use addRoute(). Kept temporarily for backward compatibility. + */ + registerRouteWithMetadata( + method: string, + path: string, + handler: Function, + metadata: RouteMetadata + ): void { + this.addRoute(method, path, handler, metadata); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/http/platform/uws-platform.adapter.ts` around lines 438 - 445, The public method addRoute replaced registerRouteWithMetadata and to preserve backward compatibility add a delegating deprecated alias named registerRouteWithMetadata that simply calls addRoute(method, path, handler, metadata); mark it with a JSDoc `@deprecated` notice and keep the same signature so existing callers still work; ensure it delegates to this.routeRegistry.register via addRoute so behavior is identical and add a short comment indicating it will be removed in a future release.
🧹 Nitpick comments (1)
docs/http/Routing.md (1)
41-53: ⚡ Quick winUse class tokens in the advanced example to preserve DI behavior.
The current sample uses
newfor guards/pipes/filters, which bypasses container resolution. Prefer class references so middleware with constructor dependencies works as expected. Also, wording can say this is an adapter API rather than “direct access to internal registry.”Suggested docs update
-For advanced use cases requiring programmatic route creation outside of NestJS controllers, uWestJS provides direct access to its internal registry via `addRoute()`. This allows you to manually register routes while still executing standard NestJS guards, pipes, and filters. +For advanced use cases requiring programmatic route creation outside NestJS controllers, uWestJS exposes an adapter-level `addRoute()` API. It delegates to the internal `RouteRegistry` pipeline and still executes guards, pipes, and filters. adapter.addRoute('GET', '/health', (req, res) => { res.send({ status: 'ok' }); }, { - guards: [new CustomAuthGuard()], - pipes: [new CustomValidationPipe()], - filters: [new CustomExceptionFilter()] + guards: [CustomAuthGuard], + pipes: [CustomValidationPipe], + filters: [CustomExceptionFilter] });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/http/Routing.md` around lines 41 - 53, Update the doc example and wording: change the phrase "direct access to its internal registry" to "adapter API" and in the addRoute example use class tokens (constructor references) instead of instantiating guards/pipes/filters with new so Nest's DI can resolve them; reference the UwsPlatformAdapter.addRoute API and show guards/pipes/filters as CustomAuthGuard, CustomValidationPipe, CustomExceptionFilter class tokens so middleware with constructor dependencies is resolved by the container.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/http/routing/route-registry.ts`:
- Line 682: The single-line ternary return expression using middleware is
causing Prettier/ESLint failures; replace the ternary with an equivalent,
properly formatted conditional (e.g., an if/else that returns middleware.name
when typeof middleware === 'function' otherwise returns
middleware.constructor?.name || 'AnonymousMiddleware') so the line is split and
formatted according to linter rules, targeting the return statement that
references the middleware variable.
---
Outside diff comments:
In `@src/http/platform/uws-platform.adapter.ts`:
- Around line 438-443: The addRoute method declaration in
uws-platform.adapter.ts is misformatted and failing Prettier; reformat the
addRoute signature and surrounding lines to conform to project Prettier rules
(spacing, indentation, commas and line breaks) by running Prettier or your
editor's format command on the file so the addRoute(method: string, path:
string, handler: Function, metadata: RouteMetadata): void { ... } declaration
matches the repository style.
- Around line 438-445: The public method addRoute replaced
registerRouteWithMetadata and to preserve backward compatibility add a
delegating deprecated alias named registerRouteWithMetadata that simply calls
addRoute(method, path, handler, metadata); mark it with a JSDoc `@deprecated`
notice and keep the same signature so existing callers still work; ensure it
delegates to this.routeRegistry.register via addRoute so behavior is identical
and add a short comment indicating it will be removed in a future release.
---
Nitpick comments:
In `@docs/http/Routing.md`:
- Around line 41-53: Update the doc example and wording: change the phrase
"direct access to its internal registry" to "adapter API" and in the addRoute
example use class tokens (constructor references) instead of instantiating
guards/pipes/filters with new so Nest's DI can resolve them; reference the
UwsPlatformAdapter.addRoute API and show guards/pipes/filters as
CustomAuthGuard, CustomValidationPipe, CustomExceptionFilter class tokens so
middleware with constructor dependencies is resolved by the container.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e08a8dbe-fc85-4242-950d-6505aa05bccf
📒 Files selected for processing (3)
docs/http/Routing.mdsrc/http/platform/uws-platform.adapter.tssrc/http/routing/route-registry.ts
| private getMiddlewareName(middleware: Type<unknown> | object): string { | ||
| return typeof middleware === 'function' ? middleware.name : middleware.constructor.name; | ||
| if (!middleware) return 'UnknownMiddleware'; | ||
| return typeof middleware === 'function' ? middleware.name : (middleware.constructor?.name || 'AnonymousMiddleware'); |
There was a problem hiding this comment.
Prettier/ESLint formatting error remains on this ternary.
This exact line is flagged by both ESLint and Prettier checks; please reformat (or run prettier --write) so CI passes.
🧰 Tools
🪛 ESLint
[error] 682-682: Replace ·?·middleware.name·:·(middleware.constructor?.name·||·'AnonymousMiddleware') with ⏎······?·middleware.name⏎······:·middleware.constructor?.name·||·'AnonymousMiddleware'
(prettier/prettier)
🪛 GitHub Check: CodeFactor
[warning] 682-682: src/http/routing/route-registry.ts#L682
Replace ·?·middleware.name·:·(middleware.constructor?.name·||·'AnonymousMiddleware') with ⏎······?·middleware.name⏎······:·middleware.constructor?.name·||·'AnonymousMiddleware' (prettier/prettier)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/http/routing/route-registry.ts` at line 682, The single-line ternary
return expression using middleware is causing Prettier/ESLint failures; replace
the ternary with an equivalent, properly formatted conditional (e.g., an if/else
that returns middleware.name when typeof middleware === 'function' otherwise
returns middleware.constructor?.name || 'AnonymousMiddleware') so the line is
split and formatted according to linter rules, targeting the return statement
that references the middleware variable.
|
@VikramAditya33 i'v fixed the mentioned linting issues |
|
Prettier checks are still failing |
| /** | ||
| * @deprecated This method is deprecated and will be removed in a future release. Use addRoute instead. | ||
| */ | ||
| registerRouteWithMetadata( | ||
| method: string, | ||
| path: string, | ||
| handler: Function, | ||
| metadata?: RouteMetadata | ||
| ): void { | ||
| this.addRoute(method, path, handler, metadata); | ||
| } | ||
|
|
There was a problem hiding this comment.
Why have you kept this as deprecated? Doesn't make sense to me this should be completely removed
There was a problem hiding this comment.
We haven't released v2 yet so no sense of keeping it as deprecated
|
You are right, it must have gotten in there since i gave the same prompt coderabbit gave me, i'v fixed the issue, removed the deprecated code and resolved the prettier liniting issues as well |
|
Please squash and rebase and also properly link the issue in PR description to close/fix it |
… registry usage - Fixed instance handling bug in `RouteRegistry.getMiddlewareName` to support passing plain object instances in route metadata without throwing errors. - Renamed `registerRouteWithMetadata` to `addRoute` to signify programmatic usage outside of the NestJS standard pipeline. - Updated the JSDoc for `addRoute` to clearly identify it as an advanced/manual API. - Documented the dual usage model (NestJS Controller Pipeline vs Direct Registry Registration) in the HTTP Routing documentation.
|
Hey i'v squashed and rebased the branch and also linked the issue in PR description |
2 participants
This PR fixes the issues mentioned in #129
RouteRegistry.getMiddlewareNameto support passing plain object instances in route metadata without throwing errors.registerRouteWithMetadatatoaddRouteto signify programmatic usage outside of the NestJS standard pipeline.addRouteto clearly identify it as an advanced/manual API.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation