Swagger compatibility

[mhavelant]

I'm submitting a...

[ ] Regression 
[ ] Bug report
[?] Feature request
[x] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead post your question on Stack Overflow.

Current behavior

There's no documented way for enabling swagger integration for the health endpoints.

Expected behavior

Documentation for enabling swagger integration for the health endpoints.

Minimal reproduction of the problem with instructions

What is the motivation / use case for changing the behavior?

I have swagger enabled so I can benefit from the autogenerated documentation for my API. When enabling terminus, its endpoints (e.g /health/readiness) do not show up on the list.

I can add a custom controller with the same endpoint (AppController / @get('health/readiness') ) and then it is registered and the terminus-code is executed, but that isn't a clean way of doing this.

Environment

Nest version: 5.5.0
Nest terminus version: 5.5.2

 
For Tooling issues:
- Node version: v10.15.0  
- Platform: Linux / Docker 

Others:

[BrunnerLivio]

Thank you for reporting!

@kamilmysliwiec
Terminus does directly modify the underlying http server instance. That is why @nestjs/swagger does not recognize the health check routes.

Also related #33

Some possible thoughts on how we should proceed:

1. Drop the of the underlying @godaddy/terminus package and maintain the healtchecks handling ourselves

• Pro: Better control how the healthchecks are being implemented (e.g. use internal router API instead of HTTP Server)
• Pro: No waiting for PR acceptance
• Contra: Having to maintain it ourselves

2. Create a PR for custom registration of the HTTP listener on @godaddy/terminus

I tend to option use option 1. The @goddady/terminus lib is really small and the benefit of using it is rather small.

[mhavelant]

Creating a PR for @godaddy/terminus would allow the feature to be used by people who are not using nest, too, even if the "issue -> pr -> wait for accept" is a slow process.
However, this seems like a niche use case for terminus, they might not prioritize merging it.

So unless upstream is unwilling to merge it, I'd vote 2.

[BrunnerLivio]

@mhavelant you're definitely right on that point.

After some digging, maybe we don't even have to modify @godaddy/terminus. I am not quite sure how the internal routes API works in NestJS, but I am currently trying to get it running:

https://github.com/nestjs/terminus/pull/34/files#diff-b93d1fe3b1adefa429e1a750bd1e3abaR137

Maybe @kamilmysliwiec can give me a hint how I should proceed?

[gperdomor]

any progress here?

[cjancsar]

@BrunnerLivio did you implement a solution?

[BrunnerLivio]

It is coming in version 7! Stay tuned :)