cloudflare / chanfana

OpenAPI 3 and 3.1 schema generator and validator for Hono, itty-router and more!
https://chanfana.pages.dev
MIT License
263 stars 35 forks source link

feat: type inference support for data argument #130

Closed iann838 closed 3 months ago

iann838 commented 4 months ago

This PR aims to add type inference support for the data argument in routes. Implementing this feature aims to be non-breaking yet support as many legacy types as possible in that constraint.

Proposed Syntax

export class ToDoCreateTyped extends OpenAPIRoute {
  static schema = {
    tags: ['ToDo'],
    summary: 'List all ToDos',
    parameters: {
      p_num: Query(Num),
      p_num2: Path(new Num()),
      p_arrstr: Query([Str]),
      p_datetime: Query(DateTime),
      p_dateonly: Path(DateOnly),
      p_hostname: Header(Hostname),
    },
    requestBody: z.object({
      title: z.string(),
      description: z.string().optional(),
      type: z.enum(['nextWeek', 'nextMoth']),
    }),
    responses: {
      '200': {
        description: 'example',
        schema: {
          params: {},
          results: ['lorem'],
        },
      },
    },
  }

  async handle(
    request: Request,
    env: any,
    context: any,
    data: DataOf<typeof ToDoCreateTyped.schema>
  ) {
    data.query.p_num
    data.query.p_arrstr
    data.query.p_datetime
    data.params.p_num2
    data.params.p_dateonly
    data.headers.p_hostname
    data.body.title
    data.body.type
    data.body.description
    return {
      params: data,
      results: ['lorem', 'ipsum'],
    }
  }
}

Implementation details

Legacy support and restrictions

The reasoning of this PR for legacy support depends on the value each legacy type provides for parameter coercion and its technical feasibility to manipulate its type with no breaking changes. All body schemas are assumed and should opt to use zod typings instead as coercion is not common and specification can easily get complex.

Supported legacy types:

Detailed explanation:

Other changes

github-actions[bot] commented 4 months ago

๐Ÿงช A prerelease is available for testing ๐Ÿงช

You can install this latest build in your project with:

npm install --save https://prerelease-registry.devprod.cloudflare.dev/itty-router-openapi/runs/8294915631/npm-package-itty-router-openapi-130

Or you can immediately run this with npx:

npx https://prerelease-registry.devprod.cloudflare.dev/itty-router-openapi/runs/8294915631/npm-package-itty-router-openapi-130
G4brym commented 4 months ago

Woow @iann838 this is really cool ๐Ÿ˜ƒ This will take me a few days to review, but i will get back here as soon as possible

iann838 commented 4 months ago

I do want to comment that supporting legacy typings is probably not good, especially after seeing the spaghetti on it, however, it does seem that coercion is closely bound to that part of the code. Using only Zod types looks much cleaner, any thoughts on completely removing the legacy types? I could draw up a big version and hopefully solve many of the open issues together with it.

G4brym commented 4 months ago

Do you think it would be possible to have autocomplete in the data argument of the handle function without having to define the types outside of the endpoint class?

We have always tried to have minimal or none breaking changes, but it might be time to it. I think legacy types are a good way of quickly getting an endpoint started, and it also encapsulates some duplicated logic that is required when using zod, for example when defining a number as a query parameter developers always have to use z.coerce, and when they forget, it just doesn't work.

But this should not be a limiting factor, I think you can go ahead with a bigger refactor, and ignore both legacy types and JavaScript types, and after that I can think in ways to make the migration easier for existing applications. Zod should be the main thing driving itty-router-openapi and users need to use it to get the most out of this package, legacy types is just an after though that can be skipped right now.

iann838 commented 4 months ago

@G4brym Yes, there is a way, it's just not possible in how routes are being defined as of currently. schema being a static variable makes it impossible to infer the class that holds it because a typeof Class is function and can't hold a generic type, typescript has a way to reverse it using InstanceType<T> but that only works for instances.

I could come up with a different syntax that is similar, but that means a complete rework on it to the point that we can build a new router.

iann838 commented 4 months ago

@G4brym After trying a bit with the typings, I have concluded that class-based definitions cannot automatically infer the types of the arguments because the methods are being assigned to the class, typescript will check whether it is assignable but not infer the type before it's assigned. Therefore, function-based definitions seem to be the only way to do that, here is a syntax (it was prototyped with types only) that is working with working types:

router.post(
  "/accounts/:accountId/todos/",
  {
    accountId: Query(z.coerce.bigint()),
    trackId: Query(z.string()),
    accept: Header(z.string()),
    todoItem: Body(z.object({
      id: z.string(),
      title: z.string(),
      description: z.string(),
    })),
  },
  ({ req, env, ctx }, { accountId, trackId, accept, todoItem }) => {
    return new Response(accountId)
  }
)

It has the benefit of unpacking only what developers need and not having unused arguments. Are we interested in creating a smaller router that supports wider audiences with this instead? or providing an alternative to the current itty router?

G4brym commented 4 months ago

@iann838 i think function-based definitions are already done in a very good way by Hono Zod OpenAPI

here's an example definition for zod:

import { z } from '@hono/zod-openapi'
import { createRoute, OpenAPIHono } from '@hono/zod-openapi';

const app = new OpenAPIHono();

app.openapi(createRoute({
  method: 'get',
  path: '/users/{id}',
  request: {
    params: z.object({
      id: z
        .string()
        .min(3)
        .openapi({
          param: {
            name: 'id',
            in: 'path'
          },
          example: '1212121'
        })
    })
  },
  responses: {
    200: {
      content: {
        'application/json': {
          schema: z
            .object({
              id: z.string().openapi({
                example: '123'
              }),
              name: z.string().openapi({
                example: 'John Doe'
              }),
              age: z.number().openapi({
                example: 42
              })
            })
            .openapi('User')
        }
      },
      description: 'Retrieve the user'
    }
  }
}), (c) => {
  const { id } = c.req.valid('param');
  return c.json({
    id,
    age: 20,
    name: 'Ultra-man'
  });
});
G4brym commented 4 months ago

I think itty-router-openapi should continue with the class based approach, as it is what differentiates from Hono, and seams to be a feature most users appreciate Maybe defining the endpoint types outside the class like what you are proposing here, is a good middle ground between what we have right now (no type checks), and the goal to have type checks in the data parameter Then after this pr is merged and released, we can continue thinking is ways to improve developer experience with this package what do you think @iann838

iann838 commented 4 months ago

@G4brym I am also a Hono user, but I keep finding myself having to write wrappers on top of it because as you see, the code is too long for specifying only id and a response which is almost indifferent to explicitly defining the openapi spec.

iann838 commented 4 months ago

@G4brym I agree, let's go back to whether supporting legacy types or just doing a major version jump then.

G4brym commented 4 months ago

After testing your example, i'm seeing the type hints in the root of data parameter instead of the nested .query, .params like it should: image

Here's the documentation of parameters for example, where you can see that url params are located in data.params.todoId https://cloudflare.github.io/itty-router-openapi/user-guide/path-parameters/

iann838 commented 4 months ago

@G4brym I have added commit, and now it correctly places all queries in .query, all path params in .params, and all headers in .headers. image

iann838 commented 3 months ago

@G4brym After "sleeping on it", the latest commit will allow parameters and request body to be defined within the schema, this completely overwrites the original PR proposal, I have updated the PR description. It now simply uses DataOf<typeof Route.schema> and does not require foreign definitions of parameters and request bodies.

G4brym commented 3 months ago

This is really cool @iann838 I will be merging and publishing this as version 1.1.0 this Monday Thanks ๐Ÿ˜ƒ

iann838 commented 3 months ago

@G4brym I will begin doing a bigger refactoring, I don't see a discussion section for this repo, where shall I move the communications to?

G4brym commented 3 months ago

@iann838 open an issue and we continue the discussion there

G4brym commented 3 months ago

I will add some docs on this tomorrow before opening a new release ๐Ÿ‘