-
-
Notifications
You must be signed in to change notification settings - Fork 7.6k
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(core/common): Add @Search decorator to the available HTTP route handlers #10533
Conversation
Pull Request Test Coverage Report for Build 7ebb1617-a886-4ce7-8c0b-7e619c5abc22
💛 - Coveralls |
public search(port: string | number, callback?: () => void); | ||
public search(port: string | number, hostname: string, callback?: () => void); | ||
public search(port: any, hostname?: any, callback?: any) { | ||
return this.instance.search(port, hostname, callback); | ||
} |
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 move this to right below public all
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.
sure thing!
The search method is going to be anexed with the @Search decorator and going to be used to reference a SEARCH request
This decorator uses the SEARCH method to construct a decorator for a handler to a Search http request
3b37004
to
4ca8ccf
Compare
🤔 force-push it again @Gustrb |
This update was really simple, because we dont need any kind of aliasing due to fastify and express already supporting the search method for Application
4ca8ccf
to
2f69e96
Compare
Hmmm, did it and the issue seem to continue. |
nvm that's due to this: #10545 (comment) |
oh, makes sense |
btw, should I do something to fix it or should I wait the other issue? |
I notice that this PR introduces a breaking change as the I feel that we could find a better way to add more route decorators... Express supports others routing methods that we won't bring to the core but we could have a way to let the consumer extending it if they want to. An ideia: having a class with factory method that will use |
There's a way - you can inject the HTTP adapter and register the route manually, instead of using a dedicated decorator. |
yeah... I'm thinking on how we could allow the usage of such new http method at controller's class as well. @kamilmysliwiec from client POV, does having yet another decorator factory to create any http route sounds bad to you? something like this: const Search = HttpRoute('SEARCH')
class FooController {
@HttpRoute('GET')('/')
getHello() {}
@Search()
bar() {}
} then all the native ones would be just an alias to that. Seems overkill & too flexbile but I don't see any other way. I could try to implement that while still supporting |
I just notice that in the past Nest had the |
I don't find any reference, I think this maybe the |
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: #10420
What is the new behavior?
This PR adds the new Search method decorator to be a valid handler to routes.
This change was rather simple, since both express and fastify already support this little akward and rather new http method
Does this PR introduce a breaking change?
Other information