Deprecate variadic listen method (closes #3652)

[jsumners]

Resolves #3652.

As I am going through the process of getting this PR together, I am more convinced that this is the right path. I have had to jump through several hoops to cover cases of "was this signature provided? That one? This one?", and deal with adding/removing coverage from tests. Once we get past the deprecation phase, the listen code will be simplified by virtue of completely removing that normalizeListenArgs function and the ceremony around feeding it.

Technically, as implemented, the signature is still variadic since it accepts a second, optional, parameter for the callback. Should we require the callback in the options object? This would be slightly odd in that it isn't a typical pattern. But we already internally support (and actually require) listenOptions.cb be defined when a callback is to be used.

Checklist

• run npm run test and npm run benchmark
• tests and/or benchmarks are included
• documentation is changed or added
• commit message and code follows the Developer's Certification of Origin
  and the Code of conduct

[zekth]

Should we require the callback in the options object

On one hand it makes totally a good point to do it this way, on the other this is a slight change of common use from the javascript ecosystem; this will potentially raise Developer questions.

I'm +1 on this approach.

[mcollina]

Regarding the callback: let's keep it as a separate argument.

---

I'm not really convinced that we ready for a deprecation cycle warning on this. I fear that the churn for our users and the uproar on our repo would be too high.

However I support the change to a single object everywhere including our docs.

How about:

1. we stop using the variadic signature ourselves, including in our docs. Mark it as deprecated in docs and typescript.
2. runtime deprecate it in v5
3. remove in v6

Node has a --pending-deprecation, we might look into hooking into the same logic.

[jsumners]

I fear that the churn for our users and the uproar on our repo would be too high.

Can't we make the same argument for every release?

• runtime deprecate it in v5
• remove in v6

Are we planning for a more aggressive release cycle? I think we should try to track core's LTS cycle.

Node has a --pending-deprecation,

This is the first I am learning about this flag. I bet not many outside of core know about it.

[mcollina]

I would target a one-release-per-year cycle. v4 has been dragging way too long.

[jsumners]

Agreed. That timeline matches with tracking core LTS. Which means we would end up maintaining this code block for two more years instead of one. What other churn in v4 are we saying will cause this one to be too much?

[Eomm]

The code: LGMT but this PR requires a follow-up PR for documentation.

I would release it in v4 after updating the docs as well.

[jsumners]

What follow up for docs? All docs are updated in this PR.

[RafaelGSS]

LGTM.

[RafaelGSS]

Actually, I think is missing to update:

• The examples folder - e.g: https://github.com/fastify/fastify/blob/main/examples/simple.js#L28
• The type definition of .listen
• The documentation/guides - e.g: https://github.com/fastify/fastify/blob/2b6974a39905ce654d9fbf85165e68e2cd230a6c/docs/Guides/Getting-Started.md#your-first-server

[Eomm]

What follow up for docs? All docs are updated in this PR.

I see only js and json files' changes.

I mean the README for example, that uses the .listen(3000) signature

[jsumners]

I see only js and json files' changes.

I mean the README for example, that uses the .listen(3000) signature

A push got missed. Probably forgot to use --force. But I did miss the main Readme. So I'll have to fix that.

[jsumners]

Readme and examples have been updated.

What other churn in v4 are we saying will cause this one to be too much?

@mcollina can you address this question?

[darkgl0w]

Concerning the typescript definitions, the missing-types tag is not needed.
It is just a matter of adding a deprecated notice or this before lines 85, 86 and 87:

A PR targeting this one would be welcome. But I can say that the listenOptions object has more properties than I see in your comments.

After my zoom conference (it should end in less than one hour) I will submit one with all these options and a proper deprecation message ^^
Should I target main branch or directly this issue-3652 branch ?

[jsumners]

Should I target main branch or directly this issue-3652 branch ?

It should target the branch of this PR.

[mcollina]

Why did you remove the backlog parameter?

[jsumners]

Specifying it no longer matches with the lead-in text.

[darkgl0w]

The PR adding typescript definitions for this change is ready to be reviewed here.

While digging in the code to write the types a few points raised my attention :

1. technically fastify.listen() is valid as it falls back to listening on localhost and the first random open port on both ipv4 and ipv6, but it currently displays the deprecation warning (this conditional case is not covered). Is it a wanted behavior ?

2. same goes for the fastify.listen(() => {}) call that is technically valid (no options, just a callback), same question here.

3. the state of the callback argument is not clear to me, at least in the discussions. Code wise the callback argument will trigger the deprecation warning. Should I mark all the types with a callback argument as deprecated (point 2 included)?

[jsumners]

1. technically fastify.listen() is valid as it falls back to listening on localhost and the first random open port on both ipv4 and ipv6, but it currently displays the deprecation warning (this conditional case is not covered). Is it a wanted behavior ?

No.

2. same goes for the fastify.listen(() => {}) call that is technically valid (no options, just a callback), same question here.

Same bug.

3. the state of the callback argument is not clear to me, at least in the discussions. Code wise the callback argument will trigger the deprecation warning. Should I mark all the types with a callback argument as deprecated (point 2 included)?

No.

[mcollina]

lgtm,

could you add this to the release notes (google docs) too?

[jsumners]

@darkgl0w I have addressed the bugs you found.

@mcollina please forward that link to me again.

[delvedor]

LGTM

[mcollina]

@jsumners what link?

[jsumners]

@jsumners what link?

The Google doc. @RafaelGSS got it to me.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.