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

Compatibility with FastifyAdapter #24

Closed
mhavelant opened this issue Dec 20, 2018 · 3 comments
Closed

Compatibility with FastifyAdapter #24

mhavelant opened this issue Dec 20, 2018 · 3 comments
Assignees
Milestone

Comments

@mhavelant
Copy link

I'm submitting a...


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

Current behavior

Using the "FastifyAdapter" the server fails to start with the message

UnhandledPromiseRejectionWarning: TypeError: Cannot read property 'listeners' of undefined

Expected behavior

The server starts and the added health check URLs are available.

Minimal reproduction of the problem with instructions

App:

  • Basic setup of nest from the first-steps docs
    • index.ts with app = nestfactory.create() and app.listen,
    • app.module;
      • controllers: app.controller, which has a single @get(/hello) route
      • imports: HealthModule, that's the same as the one in the README, but DatabaseHealthIndicator replaced with DNSHealthIndicator which pings google

This start like expected:
const app = await NestFactory.create(AppModule); await app.listen(20000);

This fails with the mentioned error:
const app = await NestFactory.create(AppModule, new FastifyAdapter()); await app.listen(20000);

Note:

  • When using fastify/fastify with godaddy/terminus it works with this: createTerminus(fastify.server, options); (everything else is just copy/paste from their respective READMEs)

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

Nest provides FastifyAdapter by default, nestjs/terminus should be compatible with it.

Environment


Nest version: 5.5.1

 
For Tooling issues:
- Node version: 10.14.2  
- Platform: Linux running docker, image: 10.14.2-alpine ; but also Linux running 10.13.0 directly

Others:

@mhavelant
Copy link
Author

Small note: Using createTerminus(app.getHttpAdapter().getHttpServer(), terminusOptions); will register the routes for the server (but not for nest, so it's kinda pointless, but at least more info for the maintainers).

@BrunnerLivio
Copy link
Member

BrunnerLivio commented Dec 20, 2018

Thanks a lot for reporting!

getHttpServer() won't work as far as I know in a TestModuleContext. nestjs/nest#1282

Therefore I needed to change to ApplicationReferenceHost.
Seems like ApplicationReferenceHost provided by @nestjs/core does not provide a HttpServer when using Fastify.

I was able to reproduce the error by adding a FastifyAdapter e2e test case.
Travis fails with: Cannot read property 'listeners' of undefined, therefore ApplicationReferenceHost.applicationRef.httpServer must be null

https://travis-ci.org/nestjs/terminus/builds/470600392#L717

this.httpServer = this.refHost.applicationRef.httpServer;

I created an issue on the nestjs repository
nestjs/nest#1391

@BrunnerLivio BrunnerLivio added this to the 5.5.2 milestone Dec 20, 2018
@BrunnerLivio BrunnerLivio self-assigned this Dec 20, 2018
@BrunnerLivio
Copy link
Member

Fixed with 5.5.2

nartc pushed a commit to nartc/terminus that referenced this issue Jan 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants