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

Underlying HttpServer is of type any #10480

Closed
7 of 15 tasks
royhadad opened this issue Oct 30, 2022 · 8 comments · Fixed by #10481
Closed
7 of 15 tasks

Underlying HttpServer is of type any #10480

royhadad opened this issue Oct 30, 2022 · 8 comments · Fixed by #10481
Labels
needs triage This issue has not been looked into

Comments

@royhadad
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Current behavior

app.listen() and app.getHttpServer() return the underlying HttpServer, but regarding types - the returned type is any.
Is there a way we can improve this type?
Currently, I'm doing this, which feels unsafe:
Screen Shot 2022-10-30 at 17 17 07

Minimum reproduction code

https://codesandbox.io/s/ecstatic-sanderson-xpxxfv

Steps to reproduce

  1. go to the codesandbox link
  2. hover over the server const.
  3. see it's of type any

Expected behavior

I expect server to have a safer type, perhaps { Server } from 'https'?
maybe something else? Open for suggestions

Package

  • I don't know. Or some 3rd-party package
  • @nestjs/common
  • @nestjs/core
  • @nestjs/microservices
  • @nestjs/platform-express
  • @nestjs/platform-fastify
  • @nestjs/platform-socket.io
  • @nestjs/platform-ws
  • @nestjs/testing
  • @nestjs/websockets
  • Other (see below)

Other package

No response

NestJS version

8.0.6

Packages versions

    "@nestjs/core": "8.0.6",

Node.js version

16.14.0

In which operating systems have you tested?

  • macOS
  • Windows
  • Linux

Other

Relevant source code block:

listen(port: number | string, callback?: () => void): Promise<any>;

@royhadad royhadad added the needs triage This issue has not been looked into label Oct 30, 2022
@micalevisk
Copy link
Member

micalevisk commented Oct 30, 2022

Let's use the express adapter as an example:

if (isHttpsEnabled) {
this.httpServer = https.createServer(
options.httpsOptions,
this.getInstance(),
);
return;
}
this.httpServer = http.createServer(this.getInstance());

public listen(port: any, ...args: any[]) {
return this.httpServer.listen(port, ...args);

as you can see above, we can't define the right return type statically for the return of listen because it could be either
import { Server } from 'https' or
import { Server } from 'http'
which is defined at runtime.


What we could do is narrow down that a bit. That method could then have the following return type instead of any:
HttpServer | HttpsServer
What do you think?

@micalevisk
Copy link
Member

micalevisk commented Oct 30, 2022

nvm lol

I just found that the Server is the same regardless of the http module being used:

both of them points to server.listen method from net module, which return net.Server.

So we could replace that any with net.Server since
server instanceof net.Server === true.

@kamilmysliwiec
Copy link
Member

Let's track this here #10481

@royhadad
Copy link
Author

Great! do we want to make the same adjustment in other places as well? (e.g. fastify)

@micalevisk
Copy link
Member

micalevisk commented Oct 31, 2022

I thought that the listen on fastify adapter always returns undefined due to the types defined below

public listen(port: string | number, callback?: () => void): void;
public listen(
port: string | number,
hostname: string,
callback?: () => void,
): void;
public listen(port: string | number, ...args: any[]): void {

but I just tested it and it returns the same Server instance. I'll update that too then.

image

@micalevisk
Copy link
Member

micalevisk commented Oct 31, 2022

I'm getting Type 'void' is not assignable to type 'Server' because instance.listen returns void even tho await app.listen(port, () => {}) returns Server 🤔 I might be missing something

return this.instance.listen(options, callback);

@royhadad
Copy link
Author

royhadad commented Nov 1, 2022

Well, as you've stated yourself, the actual object is different from the type definition, it makes sense that you get a type error. Using the as keyword could potentially solve this, I guess.

@micalevisk
Copy link
Member

yeah, I need to figure out why the type of this.instance.listen() is void. Feel free to investigate that please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs triage This issue has not been looked into
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants