feat: Type narrowing on status().send() chains

[aadito123]

Added type narrowing on chained .status(xxx).send(xxx) calls.
Closes #4755

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]

lgtm

[climba03003]

Personally, I would against it.

This changes means everyone using schema should provide all possible return.
I would imagine it turns out would be casting any everywhere for the payload.

[joeyballentine]

This changes means everyone using schema should provide all possible return.

This is bad why? Why would you want your schema to be mismatched from what actually happens in your code?

[aadito123]

Personally, I would against it.

This changes means everyone using schema should provide all possible return. I would imagine it turns out would be casting any everywhere for the payload.

If someone intentionally describes a schema for the response payload, then it would make sense that they would want this security enforced at the type level. Furthermore, if they don't want to cast as any, they can simply decouple the calls, instead of chaining them.

I also think it is important that the schema reflect all possible return types to ensure consistency and if there are several return types, the schema can just have a broader definition. i.e. Type.String() instead of Type.Union([Type.Literal('a'), Type.Literal('b'), ..., Type.Literal('z')])

[climba03003]

This is bad why? Why would you want your schema to be mismatched from what actually happens in your code?

It is bad because how can you send with Buffer and stream that contain the correct data.

The next question is you should also consider the content-type problem.
statusCode, Content-Type and payload is correlated, you should not ignore one of them.

Furthermore, if they don't want to cast as any, they can simply decouple the calls, instead of chaining them.

I would say a no on this solution. You are forcing people coding styles just because the types doesn't match.
So, I would consider it as a breaking change and have too much impact.

Anyway, it just a personal feeling. I do not block this to land.
If you want to discuss more, I will comment inside the issue instead of here.

[sinclairzx81]

@aadito123 Hi! Cool PR! :)

Hey, just have a quick read over the original PR for typed reply types, specifically this comment #3524 (comment). Narrowing on the status code was previously considered, however the generic argument (as implemented in v3/v4) wasn't (at the time) conducive to supporting it (as the type provider implementation was specifically written to match capabilities of the raw generics). The following is the raw generics (as i remember them)

server.get<{
   Reply: string | number // { should be Reply: { 200: string, 500: number }
}>('/', {
   schema: {
      response: {
          200: { type: 'string' },
          500: { type: 'number' }
      }
   }
}, (_, reply) => { ... })

It would be possible to support narrowing through the type providers, but would be worth considering the Reply generics (as type narrowing should work the same for both, and help simplify some of the inference strategies).

@RafaelGSS would be good to revisit this aspect.

[aadito123]

would be worth considering the Reply generics

I am not sure if the generic pattern is suitable when a schema is defined. There would need to be some reconciliation to make sure that either the type in the generic is respected in the schema or if the schema is respected by the generic since a type would essentially be defined in two places. However, I totally agree, that without a type provider, the generics would be the way to go.

Edit: just wanted to add, the generics could also solve @climba03003 concern about Buffer and stream payloads.

[sinclairzx81]

@aadito123 Hey, Just have a quick review of the implementation below. Note that it's not currently possible to specify status codes on the Generic Reply argument in a way that matches how one would specify the schema type (noting the code below would be a requirement to support Generics + status code narrowing, afaik)

TypeScript Link Here

import Fastify from 'fastify'

const fastify = Fastify()

fastify.get<{
  Reply: {    // note: this is a requirement for status code narrowing with generics
    200: string, 
    500: number 
  }
}>('/', (req, res) => {
  res.send('')                  // expect: number | string
  res.status(200).send('')      // expect: string
  res.status(500).send(1)       // expect: number
})

I do think both Generic and Type Provider inference should probably work the same here in terms of status code narrowing. However without first updating the Reply generic type to { [status: number]: T }, only the Type Providers can reasonably narrow on the status code. So just need to be mindful of this aspect.

The change to update the Reply to {[status: number]: T } should be relatively straight forward (and would allow Generics to narrow on status code as well), however changing the Reply type would constitute a breaking change in the type definitions (there's some logistics to consider here)

I'm actually VERY keen to see your PR implemented, but I do think it's important to weigh whether going out with this functionality now is the right call (knowing it may introduce a varying behavior for Generic and Type Provider inference), or whether it's best to wait for the Reply type to be updated first (in which case both Generics and Type Providers can benefit from this feature, and perhaps lead to a more unified inference strategy). So, just some things to consider.

[aadito123]

@sinclairzx81 I agree that updating the generics with the type providers would be the better move. However, as you mentioned, setting a constraint on the generic to be {[status: HttpCodes]: T} would be a breaking change.

update the Reply to {[status: number]: T }

This might be a nitpick but I think number is both too wide and too narrow of a type for the key since it would allow invalid status codes and exclude wildcards like '2xx'. I have already written a couple type helpers that might serve us better for the key type.

Nonetheless, I do not see my PR becoming an source of conflict for when the Reply generic is constrained since the tests already specify that the generic takes precedence over the type provider. As such, I do not think compatibility between the two is entirely necessary (as long as the priority for the source of inference is clarified).

fastify/test/types/type-provider.test-d.ts

Lines 51 to 66 in d0d3696

	expectAssignable(server.withTypeProvider<OverriddenProvider>().get<{ Body: 'override' }>(
	'/',
	{
	schema: {
	body: Type.Object({
	x: Type.Number(),
	y: Type.Number(),
	z: Type.Number()
	})
	}
	},
	(req) => {
	expectType<'override'>(req.body)
	}
	))

[sinclairzx81]

This might be a nitpick but I think number is both too wide and too narrow of a type for the key since it would allow invalid status codes and exclude wildcards like '2xx'. I have already written a couple type helpers that might serve us better for the key type.

Yeah, I saw the status code mapping, looks good.

I agree that updating the generics with the type providers would be the better move. However, as you mentioned, setting a constraint on the generic to be {[status: HttpCodes]: T} would be a breaking change.

Yeah, it's just something to be mindful of more than anything. The original implementation of reply type inference held back on status code narrowing (for the reasons noted above). However, this was back in December 2021 (obviously, a very long time ago). I do still think it's worth considering the Generics side of things (and to put a spotlight on the current Reply type definition as it relates to this feature). But it's best for the Fastify TS team to weigh the logistics overall.

Good work! :)

[aadito123]

best for the Fastify TS team

That is fair. As useful as I find this to be in my own code base, if the team feels it might be better to hold off until the generics are also ready, that is fine.

[climba03003]

But it's best for the Fastify TS team to weigh the logistics overall.

You can always jot some notes on #4081
I would like anyone who experienced in TypeScript to lend a hand on how to structure the types further.

[joeyballentine]

Just wanted to chime in and say that the Reply type narrowing was the only thing I considered missing from Fastify. I have been messing around with it the past few days as I've been considering switching from Express to Fastify for work-related projects. The fact that I can define a schema in one place and have it both generate TS types and OAS docs is extremely powerful and time saving, it's just missing the ability to type-check the responses accurately. So to me this is a very welcomed addition.

[climba03003]

Yes, it would be a good addition for type-check to exist.
But I don't want to land it and then pop up a usage regression and revert again.

The use-case is more than you though, restricting payload also impact a lot.
Anyone who are reviewing should be very careful on this changes.

it's too deep for me to review

[fox1t]

First of all, thank you for the PR, @aadito123! Amazing stuff.
After reviewing the discussion, I agree with @climba03003 and @sinclairzx81: we should consider modifying the Reply type to support generics. Landing this PR now will make us change it again in the future and ship another breaking change at the type level. Let's consider the effort to implement the Reoly refactor and type providers too. What do you think?

[aadito123]

@fox1t I suppose that is fair. My only concern with it would be that now the schema and the Reply argument may not have matching types, although perhaps some sort of inference magic can make it work. Nonetheless, I think people might end up having to define types twice (once in the generic, and once in the schema) or having to change up their code style a bit to avoid that.

I would be happy to work on a PR for the Reply generic if that is an implementation the typescript team can get behind. Personally, I really want the type narrowing, so I don't mind if I change my code a little to do it.

[fox1t]

I would be happy to work on a PR for the Reply generic if that is an implementation the typescript team can get behind. Personally, I really want the type narrowing, so I don't mind if I change my code a little to do it.

I don't understand this sentence. Could you rephrase it?

[aadito123]

@fox1t what I meant was that if the Typescript team is okay with using the Reply generic, and will merge the PR (assuming implementation is good), then I am ready to work on it. Basically, I don't want to make a PR for it and realize that it could still be a breaking change.

[fox1t]

Oh! Thanks for clarifying! @fastify/typescript, do we agree on this? My vote is yes. Let’s do it.

[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.