Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Release 3.1.0 broke type inference on fastify route handler request parameter #73

Closed
2 tasks done
g0di opened this issue Apr 21, 2023 · 18 comments
Closed
2 tasks done

Comments

@g0di
Copy link

g0di commented Apr 21, 2023

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the bug has not already been reported

Fastify version

4.15.0

Plugin version

3.1.0

Node.js version

16.19.1

Operating system

macOS

Operating system version (i.e. 20.04, 11.3, 10)

Monterey 12.6.5

Description

Hi,

Just updated plugin to version 3.1.0 recently and it looks like I lost types inference on the fastify routes handler request object. I rollbacked to 3.0.0 which is working fine. The following screenshot is showing that I no longer get types inference on the params object of the fastify request despite schema definition being provided and Typebox type provider being used.

image

Thank you for your help ;)

Steps to Reproduce

import { TypeBoxTypeProvider } from '@fastify/type-provider-typebox';
import { Type } from '@sinclair/typebox';
import fastify from 'fastify';

fastify()
  .withTypeProvider<TypeBoxTypeProvider>()
  .get('/', {
    schema: {
      params: Type.Object({
        foo: Type.String()
      })
    },
    handler(req, rep) {
      console.log(req.params.foo);
      rep.send();
    }
  });

Expected Behavior

  • In the previous example, req.params.foo should be typed as a string.
  • Using strict flag with typescript, I should not get the following error:
'req.params' is of type 'unknown'.ts(18046)
@SeanReece
Copy link

I think this could be a Typebox version compatibility issue. I ran into something similar, type-provider-typebox now exports @sinclair/typebox directly so you should use that export. The docs have been updated to reflect the new usage.

import { TypeBoxTypeProvider, Type } from '@fastify/type-provider-typebox';
// Do not import @sinclair/typebox directly
import fastify from 'fastify';

fastify()
  .withTypeProvider<TypeBoxTypeProvider>()
  .get('/', {
    schema: {
      params: Type.Object({
        foo: Type.String()
      })
    },
    handler(req, rep) {
      console.log(req.params.foo);
      rep.send();
    }
  });

@digitalmio
Copy link

I'm experiencing the same problem - changed import to type-provider-typebox but it didn't help.

@semoal
Copy link
Contributor

semoal commented Apr 25, 2023

@sinclairzx81 any guidance here? Experiencing this upgrading from *27.8 to 28.5

@sinclairzx81
Copy link
Contributor

sinclairzx81 commented Apr 25, 2023

@semoal Hi!

In 3.0, TypeBox was taken as a direct dependency on the provider (where as previously it was a peerDependency). If manually upgrading to TypeBox 0.28.x (or versions higher than the dependency used by the provider) this may be the cause type inference problems as TypeScript may be observing both installed versions of TypeBox (leading to possible type inference ambiguities).

There's a couple of options I can think of to resolve this.

A) Upgrade the provider to use TypeBox 0.28.x
B) Retain re-export of Type.* but revert to peerDependency,

I think option B might be the most flexible option for sourcing the most recent TB revisions.

@mcollina @RafaelGSS Any thoughts on this?

@seia-soto
Copy link

seia-soto commented Apr 26, 2023

Hopefully, seems like the patches in progress.

Just sharing my use: I always write a dedicated import to TypeBox from the actual TypeBox package (@sinclair/typebox) instead of Type exported from @fastify/type-provider-typebox. It's because I want to match the import with other script files that uses TypeSystem.Format. For later releases, I also suggest that exporting the TypeSystem and other ecosystem in TypeBox is preferred.

Also, the library should be peer dependency in my case.

nadhifikbarw added a commit to nadhifikbarw/fastify-type-provider-typebox that referenced this issue Apr 27, 2023
@nadhifikbarw
Copy link
Contributor

Since I personally prefer reverting back to peerDependency approach i've opened PR #78 if we do agree to pick this alternative. If we pick other alternatives, PR can be closed.

For others looking for temporary solution, in the meantime, you can keep using 3.1.0 by version pinning your @sinclair/typebox dependency to 0.27.8 (but I also recommend also performing exact version pinning to 3.1.0 in case fix is being rolled out later for your project stability).

@RafaelGSS
Copy link
Member

I'd go with option B. However, I'm not sure it will solve all the edge cases.

@erfanium
Copy link

erfanium commented Apr 29, 2023

+1
I suggest move it back to peerDependecies
more detail here

mcollina pushed a commit that referenced this issue May 2, 2023
* build(deps): revert to peerDependency, refer to #73

* docs: mention peer dependency requirement and provide direct import example

* build: widen patch version range and remove fastify from peer dependency
@nadhifikbarw
Copy link
Contributor

@mcollina okay to release 3.3.0 and close this?

@mcollina
Copy link
Member

mcollina commented May 4, 2023

yes

@mcollina
Copy link
Member

mcollina commented May 8, 2023

@nadhifikbarw I'm lost in what should be in v3.3.0, could you clarify?

@mcollina
Copy link
Member

mcollina commented May 8, 2023

I think this issue was solved in v3.2.0.

@mcollina mcollina closed this as completed May 8, 2023
@nadhifikbarw
Copy link
Contributor

nadhifikbarw commented May 8, 2023

@mcollina v3.3.0 updated the ci/cd to test and support wider typebox minor version range to cover scenario like #63 , but yes the issue is solved in v3.2.0

@mcollina
Copy link
Member

mcollina commented May 8, 2023

yeah, there is no need for a release, the CI is already updated! Thx for your help!

@baughmann
Copy link

baughmann commented Feb 24, 2024

I seem to be experiencing this issue again:

import { Type, TypeBoxTypeProvider } from "@fastify/type-provider-typebox";

// ...

fastify .get(
      "/:id",
      {
        schema: {
          params: Type.Object({
            _id: Type.String(),
          }),
          response: {
            200: UserRef,
            404: ErrorResponseRef,
          },
          description: "Get a user by ID",
          operationId: "getUserById",
          tags: ["Users"],
        },
        preHandler: [is_authenticated()],
      },
      async (request) => {
        const user = await users_service.getById(request.params._id); // 'request.params' is of type 'unknown'.ts(18046)
        // ...
      }
    )

The only solution I've found is to redundantly add types twice, which doesn't seem right:

const GetByIdParams = Type.Object({
 _id: Type.String(),
});
type GetByIdParams = Static<typeof GetByIdParams>;
   
// this first line is the workaround
fastify.get<{ Params: GetByIdParams }>(
     "/:id",
     {
       schema: {
         // ...
       },
       preHandler: [is_authenticated()],
     },
     async (request) => {
       // same as before
     }
   );

@nadhifikbarw
Copy link
Contributor

@baughmann, I might be able to assist you further if you can provide more complete repo to get wider context of the general setup, my generic steps from previous experience when trying to figure out why inference doesn't get resolved correctly is to

  1. Ensure I'm calling withTypeProvider() correctly
  2. When I'm writing Fastify plugin (or as general rule if it's not within the same file where I would call withTypeProvider()), which is 90% of the time where I would be defining my route handlers, I would ensure to manually typed my plugin definition with FastifyPluginAsyncTypebox

So my plugin definition will generally always looks like

const plugin: FastifyPluginAsyncTypebox = async (fastify, _opts) => {
    // fastify.get() here will be correctly typed
}
  1. Ensuring I'm currently using "CI-tested" version of TypeBox, you can see the list, I love using TypeBox but in general it's great to be aware that minor version update for this dependency can still break inference sometimes.

Recent example I went through, bumping my dependency from 0.29 -> 0.31 broke RegEx validation due to schema changes being discussed here. If you want less flaky update it might be beneficial to pin which TypeBox version to use.

I hope this somewhat helps, this approach had helped me to avoid overall flaky-ness I had before.

@zacharynoble
Copy link

zacharynoble commented Apr 14, 2024

For people still facing this problem, this is the best solution to this problem that I could find for now:

preHandler: (request, reply, done) => [authenticate(request, reply, done), rateLimiter(request, reply, done)]

A lot less clean than being able to just do:

preHandler: [authenticate, rateLimiter]

I found similar unresolved issues on other type providers like zod, would really like to see a better solution for this issue.

@mcollina
Copy link
Member

Please open new issues for new problems. This one was long solved, and adding comments here for seemingly unrelated things would not attract help. I'm locking this.

@fastify fastify locked and limited conversation to collaborators Apr 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests