dommilosz / minecraft-auth

29 stars 3 forks source link

Add callbacks to listenForCode/createServer function #21

Closed dommilosz closed 9 months ago

dommilosz commented 9 months ago

See #20

dommilosz commented 9 months ago

I think the onstart callback location doesn't really matter that much. I merged your changes.

CordonZeus22 commented 9 months ago

If the server can't start, for example the port is already in use, put the call of onstart after the call of listen will call onstart, but put onstart in the callback of listen will not call it. You can try with

import http from "http";

const server1 = http.createServer();
const server2 = http.createServer();

server1.listen(10, "localhost", function() {
    console.log("Server 1 started. Listen callback.");
});

console.log("Server 1 started.");

server2.listen(10, "localhost", function() {
    console.log("Server 2 started. Listen callback.");
});

console.log("Server 2 started.");
dommilosz commented 9 months ago

Try wrapping everything in async function and await the listens as is the case in library code

On Thu, 28 Sep 2023, 11:40 CordonZeus22, @.***> wrote:

If the server can't start, for example the port is already in use, put the call of onstart after the call of listen will call onstart, but put onstart in the callback of listen will not call it. You can try with

import http from "http"; const server1 = http.createServer();const server2 = http.createServer(); server1.listen(10, "localhost", function() { console.log("Server 1 started. Listen callback.");}); console.log("Server 1 started."); server2.listen(10, "localhost", function() { console.log("Server 2 started. Listen callback.");}); console.log("Server 2 started.");

— Reply to this email directly, view it on GitHub https://github.com/dommilosz/minecraft-auth/pull/21#issuecomment-1738819220, or unsubscribe https://github.com/notifications/unsubscribe-auth/AM7D32REXSH2FMCIKDTD2W3X4VA2TANCNFSM6AAAAAA5HZJNJQ . You are receiving this because you authored the thread.Message ID: @.***>

CordonZeus22 commented 9 months ago

It doesn't change anything because the listen method doesn't return a Promise. I just tried to be sure.

dommilosz commented 9 months ago

Okay. I see I might've used it as express webserver in that listen actually returns promise. I've added test for that and handling errors generated by the server.

CordonZeus22 commented 9 months ago

The createServer function now returns a Promise that resolves when the server close or rejects when an error occurred. I think you want to put the resolve in the listen callback because the listenForCode function await the createServer function.

dommilosz commented 9 months ago

Yeah good point. Only if I had ran all the tests

CordonZeus22 commented 9 months ago

Also I think the createServer needs to return the server not undefined. It's better to not use any, use unknown if any type can be accepted.

CordonZeus22 commented 9 months ago

Why returns the server when the server closes ?

dommilosz commented 9 months ago

Do you have any more suggestions or can I merge?

CordonZeus22 commented 9 months ago

I think it's ok for me, you can merge. One thing maybe, when you update the package.json run npm i for also update the package-lock.json