-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Description
Discussed in #5582
Originally posted by driimus July 23, 2024
Context
Fastify currently listens on localhost by default, but there are still scenarios where that default isn't propagated to listenOptions.
One example of this is when listening options are provided and the host property is falsy:
require('fastify')().listen({host: undefined, port: 3000})Which works, but produces a deprecation warning due to dns.lookup() not liking falsy hostname values (since node v12 - code):
> (node:156681) [DEP0118] DeprecationWarning: The provided hostname "undefined" is not a valid hostname, and is supported in the dns module solely for compatibility.
at emitInvalidHostnameWarning (node:internal/dns/utils:270:13)
at Object.lookup (node:dns:195:5)
at Object.multipleBindings (/home/driimus/personal/fastify-test/node_modules/.pnpm/fastify@4.28.1/node_modules/fastify/lib/server.js:140:7)
at /home/driimus/personal/fastify-test/node_modules/.pnpm/fastify@4.28.1/node_modules/fastify/lib/server.js:116:30
at new Promise (<anonymous>)
at /home/driimus/personal/fastify-test/node_modules/.pnpm/fastify@4.28.1/node_modules/fastify/lib/server.js:114:16
at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
Proposed change
I think it would be nice to get rid of the warning by re-assigning listenOptions.host even if the property already exists, in
Lines 58 to 60 in 9226a47
| if (Object.prototype.hasOwnProperty.call(listenOptions, 'host') === false) { | |
| listenOptions.host = host | |
| } |
As long as the provided value is nullish, which would more closely match the fallback for
host:Line 54 in 9226a47
| host = listenOptions.host ?? 'localhost' |
Use case
Looking back at the original example, a more likely use case is to override host using an env var or similar variable which is potentially undefined:
require('fastify')().listen({host: process.env.FASTIFY_HOST, port: 3000})
```</div>