Skip to content
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(): add ip route param #3249

Merged
merged 1 commit into from
Nov 3, 2019
Merged

Conversation

iniel
Copy link

@iniel iniel commented Oct 24, 2019

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Added ip param decorator for HTTP route handlers.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

@coveralls
Copy link

Pull Request Test Coverage Report for Build 5377

  • 3 of 3 (100.0%) changed or added relevant lines in 3 files are covered.
  • 11 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.001%) to 95.292%

Files with Coverage Reduction New Missed Lines %
packages/core/injector/module.ts 11 86.67%
Totals Coverage Status
Change from base Build 5359: -0.001%
Covered Lines: 4372
Relevant Lines: 4588

💛 - Coveralls

Copy link
Member

@BrunnerLivio BrunnerLivio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really nice PR, well done!!!

From my side this feature is good to go. Though @iniel for future contributions, please create an issue before creating a pull request. This workflow is commonly done, so you do not waste time implementing something, which does not fit the core teams vision :)

@kamilmysliwiec
Copy link
Member

kamilmysliwiec commented Oct 31, 2019

Even though I really like this PR, I'm slightly concerned that adding more and more decorators that simply pick properties from the request instance might be redundant :( Like, if one wants to get an IP address and use decorator for that, it can be achieved with one line of code:

export const Ip = createParamDecorator((data, req) => req.ip);

@iniel
Copy link
Author

iniel commented Oct 31, 2019

You are right in this concern. Basically I think most of the fields in request object are too low level to be used in handler business logic (like method, host etc.). And it's better to use them only in specialized services.
But ip is only field left appropriate to use in handler but have no decorator from the box. So my motivation was - not to hardcode lonely local decorator with createParamDecorator but push this to upstream :)

@kamilmysliwiec kamilmysliwiec added this to the 6.9.0 milestone Nov 3, 2019
@kamilmysliwiec
Copy link
Member

Thanks for your input @iniel. Sounds reasonable :)

@kamilmysliwiec kamilmysliwiec merged commit ee20664 into nestjs:master Nov 3, 2019
@kamilmysliwiec kamilmysliwiec mentioned this pull request Nov 3, 2019
3 tasks
@lock
Copy link

lock bot commented Feb 1, 2020

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Feb 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants