-
-
Notifications
You must be signed in to change notification settings - Fork 7.7k
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
feat(fastify): supporting fastify route config #11992
feat(fastify): supporting fastify route config #11992
Conversation
Pull Request Test Coverage Report for Build f42754e6-8e3c-4930-98b6-09e98c21196b
💛 - Coveralls |
Docs PR: nestjs/docs.nestjs.com#2790 |
Note: there wasn't an existing test case for |
Could you create a draft PR to the docs describing this feature? :) A short section just for demonstration purposes would be sufficient |
361c208
to
81ce478
Compare
Is the change in this PR good or do you want me to elaborate? https://github.com/nestjs/docs.nestjs.com/pull/2790/files |
@rich-w-lee ahh I must have missed that PR, apologies! Looks good |
import { SetMetadata } from '@nestjs/common'; | ||
import { FASTIFY_ROUTE_CONFIG_METADATA } from '../constants'; | ||
|
||
export const RouteConfig = (config: any) => |
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.
can we get ride of the any
here?
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.
So I checked in the fastify types, and they don't have a type requirement for it, so I want to check what you prefer instead of any:
- Using
unknown
since it's defined by the client - Using a generic
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.
to me, generic type arg is almost the same as having any in terms of type safety
unknown
would be better but cumbersome to maintain
so we can keep any
until they release a type for that config.
But we can have a jsdocs on that like this:
/**
* @param config See {@link https://fastify.dev/docs/latest/Reference/Routes/#config}
*/
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.
Cool, I can add that in 👍
81ce478
to
ca4a48e
Compare
lgtm |
Resolves #11815
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: #11815
What is the new behavior?
Users can use the
@RouteConfig()
decorator to specify a value for the route config for fastify endpointsDoes this PR introduce a breaking change?
Other information