feat: Type narrow on status().send() chains with Reply generics by aadito123 · Pull Request #4823 · fastify/fastify

aadito123 · 2023-06-14T03:30:24Z

Added type narrowing on chained .status(xxx).send(xxx) calls only using reply generics.
Previously tried doing this using the schema but it seems the typescript team preferred doing it through generics instead
as noted here.

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

metcoder95

LGTM, nice work 🚀
Have nothing to add, but maybe ask if we could add this new feature to the docs, under the generics section maybe?

Having the generics well explained + an example can be of much help

aadito123 · 2023-06-19T02:03:19Z

could add this new feature to the docs, under the generics section maybe?

Having the generics well explained + an example can be of much help

Fair enough. Made a short addition to the docs with an example. I did not think it was necessary to mention the non-type-narrowed use case but I don't mind adding it if it seems like a reasonable inclusion.

metcoder95

LGTM 👍

test/types/reply.test-d.ts

Uzlopak · 2023-06-25T09:15:34Z

I saw this PR just now. Can I have a day to think about it, please?

Co-authored-by: Uzlopak <aras.abbasi@googlemail.com>

voxpelli

Some feedback on how this could be simplified and/or reworded

types/reply.d.ts

types/utils.d.ts

types/reply.d.ts

aadito123 · 2023-06-27T05:40:42Z

Some feedback on how this could be simplified and/or reworded

Thanks a lot for going through my PR, @voxpelli

You made a lot of suggestions so I thought I would address them in a reply.

types/utils.d.ts
I agree. There is room to simplify although I don't like that we lose the wildcard key inference. I think I came to a decent compromise that keeps that inference, and loses some of the strictness around the validity of the codes without allowing anything too weird.

type StringAsNumber<T extends string> = T extends `${infer N extends number}` ? N : never;
type Digit = 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9;
type HttpCodes = StringAsNumber<`${Digit}${Digit | 0}${Digit | 0}`>;
type HttpKeys = HttpCodes | `${Digit}xx`;
export type StatusCodeReply = {
  // eslint-disable-next-line no-unused-vars
  [Key in HttpKeys]?: unknown;
};

// weird TS quirk: https://stackoverflow.com/questions/58977876/generic-conditional-type-resolves-to-never-when-the-generic-type-is-set-to-never
export type HttpCodesCovered<Key> = [Key] extends [never] ? number :
  Key extends HttpCodes ? Key :
    Key extends `${infer X extends Digit}xx` ?
      StringAsNumber<`${X}${Digit | 0}${Digit | 0}`> : number;

// eslint-disable-next-line @typescript-eslint/no-unused-vars
// eslint-disable-next-line no-unused-vars
export type CodeToReplyKey<Code extends HttpKeys> = `${Code}` extends `${infer FirstDigit extends Digit}${infer RemainingDigits}`
  ? `${FirstDigit}xx`
  : never;

I think this is sufficiently terse without compromising functionality.

types/reply.d.ts

You're right, CodeConstrainer can be removed entirely, and it makes sense to pass the Reply type instead of the entire RouteGeneric.

export type ReplyTypeConstrainer<RouteGenericReply, Code extends HttpCodesCovered<keyof RouteGenericReply>> =
  RouteGenericReply extends StatusCodeReply ?
    RouteGenericReply[Code extends keyof RouteGenericReply ? Code : Code extends keyof StatusCodeReply ? CodeToReplyKey<Code> : never] :
    unknown;

code<Code extends HttpCodesCovered<keyof RouteGeneric['Reply']>>(statusCode: Code): FastifyReply<RawServer, RawRequest, RawReply, RouteGeneric, ContextConfig, SchemaCompiler, TypeProvider, ReplyTypeConstrainer<RouteGeneric['Reply'], Code>>;
status<Code extends HttpCodesCovered<keyof RouteGeneric['Reply']>>(statusCode: Code): FastifyReply<RawServer, RawRequest, RawReply, RouteGeneric, ContextConfig, SchemaCompiler, TypeProvider, ReplyTypeConstrainer<RouteGeneric['Reply'], Code>>;

What do you think? I can push a commit for these changes if we're aligned.

voxpelli · 2023-06-27T09:57:00Z

@aadito123

Some feedback:

I would still suggest renaming HttpCodesCovered and CodeToReplyKey to WildCardsToCode and CodeToWildCard as I found it quite hard to figure out the purpose of them initially, especially HttpCodesCovered, until I figured that it was a mechanism for converting from and to wildcards.
I would change CodeToReplyKey<Code extends HttpKeys> to CodeToReplyKey<Code extends numbers>, there is no need to limit it to a certain set of keys, all it should do is convert the given number to a wildcard style string and that it can do with whatever number?

I'm a bit scared of HttpCodes as that's a union of 900 numbers (why we need ranges in TS and can't rely on unions for all use cases Suggestion: Range as Number type microsoft/TypeScript#15480), but maybe we could at least limit them to 500?

type HttpCodeClasses = 1 | 2 | 3 | 4 | 5;
type Digit = 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9;
type HttpCodes = StringAsNumber<`${HttpCodeClasses}${Digit}${Digit}`>;

Or actually go with your original solution for the WildCardsToCode/HttpCodesCovered? As they are way fewer than 500 numbers and as such is likely to be a much more performant version?

type ResponseCodes100 = 100 | 101 | 102 | 103 | 122;
type ResponseCodes200 = 200 | 201 | 202 | 203 | 204 | 205 | 206 | 207 | 208 | 226;
type ResponseCodes300 = 300 | 301 | 302 | 303 | 304 | 305 | 306 | 307 | 308;
type ResponseCodes400 = 400 | 401 | 402 | 403 | 404 | 405 | 406 | 407 | 408 | 409 | 410 | 411 | 412 | 413 | 414 | 415 | 416 | 417 | 418 | 421 | 422 | 423 | 424 | 425 | 426 | 428 | 429 | 431 | 451;
type ResponseCodes500 = 500 | 501 | 502 | 503 | 504 | 505 | 506 | 507 | 508 | 510 | 511;

And I'm quite okay with WildCardsToCode/HttpCodesCovered being limited to a set of predefined codes as long as CodeToWildCard/CodeToReplyKey isn't

If we change of CodeToReplyKey<Code extends HttpKeys> to CodeToReplyKey<Code extends numbers> I would still suggest this setup for ReplyTypeConstrainer, removes the need for StatusCodeReply and HttpKeys + I think it makes it very easy to read, "Is `Code? an actual reply code? Go with that! Does it map to a wildcard? Then go with that! Else? Unknown!":

export type ReplyTypeConstrainer<Reply, Code extends HttpCodesCovered<keyof Reply>> =
  Code extends keyof Reply ? Reply[Code] :
    CodeToReplyKey<Code> extends keyof Reply ? Reply[CodeToReplyKey<Code>] :
      unknown;

aadito123 · 2023-06-27T10:36:43Z

@voxpelli Agree with basically all your feedback and made the changes. I limited the HttpCodes up to 500s to limit the size of unions. Typescript's union limit is 100K so I don't think performance should be too concerning at a couple hundred.

However, I don't agree with the renaming. HttpCodesCovered does not just convert the wildcards to codes. It takes a union of codes and wildcards, and then spits out the union of codes. How about something like ConvertReplyKeysToCodes?
CodeToReplyKey also takes a code and will either return a code back, or it will return a wildcard so naming it CodeToWildCard is a bit misleading. I thought the original name was more accurate. Perhaps some comments explaining them would be warranted?

aadito123 · 2023-06-28T08:57:42Z

I forgot to pass the Reply type in the tests. Fixed!

aadito123 · 2023-06-28T15:45:23Z

Any advice on why this test might be failing? My changes only affected the types.

kibertoad · 2023-07-01T20:23:28Z

@aadito123 I think we broke something here.

In my ErrorHandler instance I'm getting an error { my response object} is not assignable to parameter of type 'undefined', which seems clearly wrong, why would it expect undefined?

My ErrorHandler type looks like this:

export const errorHandler = function (
  this: FastifyInstance,
  error: Record<string, unknown>,
  request: FastifyRequest,
  reply: FastifyReply,
) {
   // responseObject resolution code that doesn't matter
   //
   // next line doesn't compile anymore
   void reply.status(responseObject.statusCode).send(responseObject.payload)
}

We should adjust default type to be more permissive, to avoid the change being a breaking change.

Can you also please add documentation on how this case should be handled? There is no Server type in this case.

aadito123 · 2023-07-02T00:33:36Z

@kibertoad this change was supposed to return unknown for the default types but it seems I missed a case. Will push a PR to fix it immediately, with a test case.
Edit: In the meantime, you could type-narrow responseObject.statusCode to the union of accepted codes (basically all integers from 100 to 599).

This reverts commit 4c532d7.

…cs (#4823)" This reverts commit 7d02f76.

github-actions · 2024-07-02T00:23:14Z

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.

aadito123 and others added 3 commits June 14, 2023 09:52

initial type narrowing functionality

0da9be9

type narrow on wildcards

88169c5

Merge branch 'main' into main

55cca08

github-actions bot added the typescript TypeScript related label Jun 14, 2023

Merge branch 'main' into main

10a1573

mcollina requested review from Uzlopak, climba03003 and metcoder95 June 16, 2023 13:26

mcollina approved these changes Jun 16, 2023

View reviewed changes

aadito123 added 2 commits June 17, 2023 09:33

Merge branch 'main' into main

e90e63f

Merge branch 'main' into main

a7b5f33

metcoder95 reviewed Jun 18, 2023

View reviewed changes

fix typo in tests and add docs

2d71210

aadito123 requested a review from metcoder95 June 20, 2023 02:25

metcoder95 approved these changes Jun 20, 2023

View reviewed changes

aadito123 added 3 commits June 21, 2023 07:13

Merge branch 'main' into main

0db5353

Merge branch 'main' into main

0629835

Merge branch 'main' into main

31e8bd5

Uzlopak reviewed Jun 25, 2023

View reviewed changes

test/types/reply.test-d.ts Outdated Show resolved Hide resolved

aadito123 and others added 5 commits June 25, 2023 20:02

fix typo in test/types/reply.test-d.ts

5e9e60e

Co-authored-by: Uzlopak <aras.abbasi@googlemail.com>

Merge branch 'main' into main

b77f125

Fix typo on reply.test-d.ts

72d35d0

Merge branch 'main' into main

f02fe20

Merge branch 'main' into main

43f4588

voxpelli reviewed Jun 26, 2023

View reviewed changes

types/reply.d.ts Outdated Show resolved Hide resolved

types/utils.d.ts Outdated Show resolved Hide resolved

types/reply.d.ts Outdated Show resolved Hide resolved

types/reply.d.ts Outdated Show resolved Hide resolved

renaming and refactoring

d5efc72

aadito123 requested a review from voxpelli June 28, 2023 01:39

voxpelli approved these changes Jun 28, 2023

View reviewed changes

aadito123 requested a review from Uzlopak June 28, 2023 07:47

fixed failing test

99ac88f

aadito123 added 2 commits June 29, 2023 07:18

Merge branch 'main' into main

0de08f4

Merge branch 'main' into main

21a4b46

mcollina merged commit 7d02f76 into fastify:main Jun 30, 2023

nflaig mentioned this pull request Jun 30, 2023

deps: update fastify to 4.19.0 ChainSafe/lodestar#5726

Merged

aadito123 mentioned this pull request Jul 2, 2023

fix: Type narrow fix issue from PR #4823 #4869

Merged

4 tasks

kibertoad pushed a commit that referenced this pull request Jul 2, 2023

fix: Type narrow fix issue from PR #4823 (#4869)

4c532d7

voxpelli mentioned this pull request Jul 3, 2023

Type narrowing on FastifyReply #4755

Closed

2 tasks

mcollina mentioned this pull request Jul 3, 2023

Type definitions do not compile anymore when using TypeScript v4.6 #4873

Closed

2 tasks

mcollina added a commit that referenced this pull request Jul 3, 2023

Revert "fix: Type narrow fix issue from PR #4823 (#4869)"

7a0d0a0

This reverts commit 4c532d7.

mcollina added a commit that referenced this pull request Jul 3, 2023

Revert "feat: Type narrow on status().send() chains with Reply generi…

6d65ab4

…cs (#4823)" This reverts commit 7d02f76.

aadito123 mentioned this pull request Jul 3, 2023

fix: Type inference breaking when Reply generic is not passed #4875

Merged

4 tasks

voxpelli mentioned this pull request Jul 7, 2023

Numeric Range Types (Feature Update) microsoft/TypeScript#54925

Open

renovate bot mentioned this pull request Sep 5, 2023

chore(integration-tests): update dependency nock to v13.4.0 gatsby-uc/plugins#441

Merged

1 task

hpx7 mentioned this pull request Mar 30, 2024

Type narrow on status().send() chains using Type-Providers schema #5378

Closed

2 tasks

aadito123 mentioned this pull request Jun 3, 2024

Reply type check improvement fastify/fastify-type-provider-typebox#117

Closed

2 tasks

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

Uh oh!

Conversation

aadito123 commented Jun 14, 2023

Checklist

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

metcoder95 left a comment

Choose a reason for hiding this comment

Uh oh!

aadito123 commented Jun 19, 2023

Uh oh!

metcoder95 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uzlopak commented Jun 25, 2023

Uh oh!

voxpelli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aadito123 commented Jun 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

voxpelli commented Jun 27, 2023

Uh oh!

aadito123 commented Jun 27, 2023

Uh oh!

aadito123 commented Jun 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aadito123 commented Jun 28, 2023

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

github-actions bot commented Jul 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

aadito123 commented Jun 27, 2023 •

edited

Loading

aadito123 commented Jun 28, 2023 •

edited

Loading

kibertoad commented Jul 1, 2023 •

edited

Loading

aadito123 commented Jul 2, 2023 •

edited

Loading