types: added http header types to reply

[skwee357]

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

[mcollina]

Custom http headers are very common. This should also allow any string.

[Uzlopak]

Does this even work????????

type name = 'matteo' | 'eomm' | string

Is afaik because of type widening the same as
type name = string

So i doubt that this has any benefit.

[Uzlopak]

Blocking because type widening makes this pointless.

[mcollina]

Blocking because type widening makes this pointless.

This is incorrect. It improves autocompletion.

[Uzlopak]

@mcollina

Please check yourself

https://www.typescriptlang.org/play?#code/C4TwDgpgBAdghgWwgZygXigcgXYwID2mUAPloQgsWcsAE4CWMA5gLABQHAxgTLVBAAeiMABsIALliIU6LJg5A

There is no autocompletion possible.

[Eomm]

For reference:

[skwee357]

A lot of comments, I'll try to address them all:

• I took the list of headers from Rust http package (as I was porting some Rust code to Node) → https://github.com/hyperium/http/blob/master/src/header/name.rs
• My PR does allow for arbitrary header name, as it allows string
• The motivation behind my PR, is to improve auto-complete and avoid stupid mistakes like contest-location instead of content-location. I understand it's not 100% bulletproof, but fastify does provide types for request headers → https://github.com/fastify/fastify/blob/main/types/request.d.ts#L59

I'm open for suggestions on how to improve it, but I think typing is necessary.

[Uzlopak]

I use vscode (which uses monaco like typescript playground) as reference. And in vscode this change has no effect.

[sergburn]

Fastify's header name constants come from @types/node, so maybe this is the way they should be defined properly, because as Eomm shows, code completion doesn't work this way.
Code completion would look like this:

However. Node types do not define response header names, maybe for a reason...

[skwee357]

@sergburn thanks. I agree that it's better to take the header names from @types/node. I wonder why they don't define reply headers, though.

[skwee357]

Started a discussion in @types/node. Let's see what they will answer and proceed based on their response DefinitelyTyped/DefinitelyTyped#66753

[Semigradsky]

This is incorrect. It improves autocompletion.

For reference:

You have 'eonm' in proposals because you are in the same file where you already typed 'eonm'. Move to another file, import type and you won't have autocomplete.

Unfortunately, the proposed changes do nothing.

[DemonHa]

if we do something like this:

type headers = 'mateo' | 'eonm' | (string & {});

it will give us the autocompletion but also we can define general strings

@Semigradsky @Eomm @mcollina @Uzlopak

[mcollina]

type headers = 'mateo' | 'eonm' | (string & {}); is supported syntax and it works.

Good job.

@skwee357 could you update the PR?

[Uzlopak]

Nice trick. Is there an explaination?

[mcollina]

https://x.com/matteocollina/status/1706595817409093698?s=20

[climba03003]

Reading through the threads, it may suffer from another issue.
microsoft/TypeScript#29729 (comment)

There will be problems on object index. The return of getHeaders

[metcoder95]

if we do something like this:

type headers = 'mateo' | 'eonm' | (string & {});

it will give us the autocompletion but also we can define general strings

This past week has been full of new learnings. First split with the extra arg, and now this 😅 🪄

[Uzlopak]

I patched it now according to @DemonHa suggestion.

Now the question is, is it safe to merge @climba03003 or do you recommend a hard no?

[DemonHa]

Nice trick. Is there an explaination?

@Uzlopak
There is a great explanation here

[skwee357]

type headers = 'mateo' | 'eonm' | (string & {}); is supported syntax and it works.

Good job.

@skwee357 could you update the PR?

I'm working on adding the types to @types/node instead, see here DefinitelyTyped/DefinitelyTyped#66783

Once it will be merged, I'll update this PR to use the types from that package, rather that having fastify maintain its own types

I updated it

[Uzlopak]

@DemonHa
@skwee357
@mcollina
@Eomm
@metcoder95
@Semigradsky
@climba03003

PTAL

[mcollina]

lgtm

[Uzlopak]

I followed the link provided by @climba03003 and saw that we can do Record<never, never> instead of {}. So to avoid that unnessary eslint ignore, i used that.