feature(core): allow status override in interceptors

[toonvanstrijp]

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

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?

Currently the only way to set a custom http status code is to use the @res() or @httpcode(). By default Nest is returning 200 or 201. Right now I'm trying to implement a interceptor that sets the status code but because Nest is overriding this in the reply phase it's not possible to set it. Or yeah it is but Nest overrides it.

Issue Number: #1869

What is the new behavior?

I've replaces the code where nest is setting the HttpStatus, this way the status is set before nest is running trough all the interceptors. Because of this we now can override the status by doing res.status(404) or we could read the current status by doing: res.statusCode.

Does this PR introduce a breaking change?

[ ] Yes
[X] No

Other information

[coveralls]

Pull Request Test Coverage Report for Build 2204

• 10 of 10 (100.0%) changed or added relevant lines in 3 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.006%) to 93.552%

---

Totals
Change from base Build 2200:	0.006%
Covered Lines:	3090
Relevant Lines:	3245

---

💛 - Coveralls

[coveralls]

Pull Request Test Coverage Report for Build 2276

• 8 of 8 (100.0%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.02%) to 93.569%

---

Totals
Change from base Build 2200:	0.02%
Covered Lines:	3087
Relevant Lines:	3242

---

💛 - Coveralls

[kamilmysliwiec]

What about setting status conditionally inside the reply() method (and making statusCode argument optional)? That would allow moving forward with this enhancement without breaking API, tests, exception filter changes, etc.

And PS. thanks for your contribution! :)

[toonvanstrijp]

@kamilmysliwiec we could do that, but since It's hard to measure when we need to set it conditionally. And if an interceptor wants to read the current status it possible by doing it my way.

Also if express or fastify changes their default response then the condition check won't work properly anymore. But setting it up front we make more things possible.

[kamilmysliwiec]

@kamilmysliwiec we could do that, but since It's hard to measure when we need to set it conditionally.

Your current solution is fine. I'm just saying that you can leave reply signature as before (with statusCode) and make status optional. Then, simply check if (statusCode) :) Thus, you won't be forced to perform any changes in tests, exception filters, and the API would remain the same. The rest is great :D

[toonvanstrijp]

@kamilmysliwiec ahh like that ;) Of course will change it in a minute. One other thing what we could improve is also set the headers before running the interceptors?

[kamilmysliwiec]

So I assume that you basically want to give an ability to override headers set through @Header() decorator inside the interceptor?

[toonvanstrijp]

@kamilmysliwiec yes exactly ;), let me know if we want that, then I'll create a new pull request with this feature in it.

[kamilmysliwiec]

Well, why not :) Sounds good!

[toonvanstrijp]

@kamilmysliwiec alright I've reverted the code so that the signature of reply didn't change only made the statusCode optional and check for it in the reply. Let me know if any other changes are needed.

Also should I change the header feature in this pull request? Otherwise we get merge conflicts later on.

[toonvanstrijp]

@kamilmysliwiec is there any documentation that needs to be changes?

[kamilmysliwiec]

Also should I change the header feature in this pull request? Otherwise we get merge conflicts later on.

You can push headers feature here too.

@kamilmysliwiec is there any documentation that needs to be changes?

Very likely no :)

[toonvanstrijp]

@kamilmysliwiec I think there is something wrong with the travis config, cause at my pc it's running fine and all the tests pass. But somehow node 8 has troubles with typeorm?

[kamilmysliwiec]

There is a sort of race condition that I have to fix shortly. No worries (triggering another build should help)

[toonvanstrijp]

@kamilmysliwiec how do I trigger an other build? :)

[kamilmysliwiec]

No worries, I'll fix that! Great contribution, thanks @toonvanstrijp :)

[lock]

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.