Fix: typescript annotation for server

[sviande]

PR Checklist

• [ X] I have run npm test locally and all tests are passing.
• [ X] I have added/updated tests for any new behavior.
• [ X] If this is a significant change, an issue has already been created where the problem / solution was discussed: [N/A, or add link to issue here]

PR Description

Fix typescript annotation accordingly to the current behavior

[nulltoken]

@sviande Can you please elaborate on that proposed change? What doesn't work for you with the current signature?

[nulltoken]

A Server is currently returned by both promises, I'm not sure to really understand the proposed fix.

Closing this for now.

Feel free to comment would have I overlooked the intent of the PR and let's revisit it together.

Cheers!

[sviande]

This change fix the signature because it's misleading, the code start func doesn't return a server.

As you can see in the comment @returns {Promise<void>} A promise that resolves when the server has been started.
Promises only resolve when the server is ready but does'nt return any value

This code resolve without providing the server

/**
   * Starts the server.
   * @param {number} [port] Port number. If omitted, it will be assigned by the operating system.
   * @param {string} [host] Host name.
   * @returns {Promise<void>} A promise that resolves when the server has been started.
   */
  async start(port?: number, host?: string): Promise<void> {
    if (this.listening) {
      throw new Error('Server has already been started.');
    }

    return new Promise((resolve, reject) => {
      this.#server
        .listen(port, host)
        .on('listening', resolve)
        .on('error', reject);
    });
  }

[nulltoken]

Oh, you're right!

So, that's a documentation issue then, as the implementation indeed returns a Server.
Would you be open to turn this Pull Request into a doco fix?

Hmmm. That's a little more complicated than that.

start() in oauth2-server.ts indeed returns a server so in that case the doco is wrong and the type definition is ok.

start() in http-server.ts doesn't return anything. In that case the doco is right and the type definition is wrong.

Duh :-/

Would you still be up to clean up that mess? ;-)

[nulltoken]

Two more possible options would also be

• to update start() in http-server.ts so that it also returns a Server
• or to patch start() in oauth2-server.ts so that it doesn't return a Server anymore

The slight issue here is that there's a mix of possible fixes and all of them could potentially be seen as breaking changes from the contract standpoint.

I'm not actually sure there's a lot of usage regarding the returned parameter of those start() methods, considering that in order to call it, you first need a Server to begin with...

@poveden Thoughts? What path do you prefer AND how should we release this? End of support for Node 18 (and potential window for a new major version) is end of April 2025, four months from now.

Note: FWIW, my current preferred option would indeed to return a Promise<void> considering the code patterns.