fix: ReplyTypeConstrainer array type inference by pedroescumalha · Pull Request #4885 · fastify/fastify

pedroescumalha · 2023-07-07T10:26:56Z

As @aadito123 mentioned on the issue, there's a pitfall here: if someone wants the Reply type to be an object with keys as http codes like { 401: string, 409: string }, the inference of the return type will be string. A possible workaround for this case would be { '4xx': { 401: string, 409: string } }.

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

Make sure the reply type is a valid object with only http codes as keys

pedroescumalha · 2023-07-08T10:59:55Z

Hey @mcollina could you take a look again? Found an issue with the previous type definition:

import fastify from '../fastify'

const server = fastify()

type TestReply = {
    200: string;
    10000: Date;
};

server.get<{ Reply: TestReply }>(
  '/ping',
  { schema: {} },
  async (request, reply) => {
    reply.code(10000).send() // type is inferred to a Date
  }
)

The type has an invalid status code, and code().send() was still inferring to TestResponse[code].
In this new implementation we make sure that if the reply object consists of only valid http status codes we infer to TestReply[code], otherwise it's infered to TestReply.

mcollina

lgtm

Uzlopak · 2023-07-08T19:03:03Z

I have to be honest:
This PR is quiete complicated. The typings test imply that the underlying issue is fixed. But does it have other effects? I dont know.
Unfortunately the typings tests are actually not good enough, as we actually needed to determine what the first Parameter of .send exactly is. We currently implicitly check that. This could mean that .send() parameter is implicitly any or so, and that is why maybe other cases, which were before stricter, are now less strict and thus dont throw error in tsd.

I personally dont know, if this PR means regressions in other cases.

mcollina · 2023-07-09T14:05:04Z

@climba03003 @fox1t wdyt?

climba03003 · 2023-07-10T16:55:40Z

types/reply.d.ts

+
+type ReplyTypeConstrainer<RouteGenericReply, Code extends ReplyKeysToCodes<keyof RouteGenericReply>> =
+  RouteGenericReply extends HttpCodesReplyType & Record<Exclude<keyof RouteGenericReply, keyof HttpCodesReplyType>, never> ?
+    Code extends keyof RouteGenericReply ? Required<RouteGenericReply[Code]> :


I am not sure if Required is needed.
It is actually changing the original generic to all property must exist, doesn't it?

Yes - https://www.typescriptlang.org/docs/handbook/utility-types.html#requiredtype

True, it's not needed at all. Just removed it.

github-actions · 2024-07-26T00:23:44Z

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.

fixed ReplyTypeConstrainer array type inference

a22e41a

github-actions bot added the typescript TypeScript related label Jul 7, 2023

mcollina approved these changes Jul 8, 2023

View reviewed changes

Fdawgs requested a review from a team July 8, 2023 08:09

Fix ReplyTypeConstrainer

e92d5df

Make sure the reply type is a valid object with only http codes as keys

mcollina approved these changes Jul 8, 2023

View reviewed changes

mcollina requested review from climba03003 and fox1t July 9, 2023 14:04

climba03003 reviewed Jul 10, 2023

View reviewed changes

remove required constraint

c31ee3e

mcollina merged commit 9d2da75 into fastify:main Jul 26, 2023

github-actions bot locked as resolved and limited conversation to collaborators Jul 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: ReplyTypeConstrainer array type inference#4885

fix: ReplyTypeConstrainer array type inference#4885
mcollina merged 3 commits intofastify:mainfrom
pedroescumalha:fix-reply-type

pedroescumalha commented Jul 7, 2023 •

edited

Loading

Uh oh!

mcollina left a comment

Uh oh!

pedroescumalha commented Jul 8, 2023

Uh oh!

mcollina left a comment

Uh oh!

Uzlopak commented Jul 8, 2023

Uh oh!

mcollina commented Jul 9, 2023

Uh oh!

climba03003 Jul 10, 2023

Uh oh!

metcoder95 Jul 11, 2023

Uh oh!

pedroescumalha Jul 11, 2023

Uh oh!

github-actions bot commented Jul 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

pedroescumalha commented Jul 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

pedroescumalha commented Jul 8, 2023

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uzlopak commented Jul 8, 2023

Uh oh!

mcollina commented Jul 9, 2023

Uh oh!

climba03003 Jul 10, 2023

Choose a reason for hiding this comment

Uh oh!

metcoder95 Jul 11, 2023

Choose a reason for hiding this comment

Uh oh!

pedroescumalha Jul 11, 2023

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jul 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pedroescumalha commented Jul 7, 2023 •

edited

Loading