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

feat: type inference support for data argument #130

Merged
merged 8 commits into from
Mar 18, 2024

Conversation

iann838
Copy link
Contributor

@iann838 iann838 commented Mar 4, 2024

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

  • All significant implementations are type-based / compiler-based; as such, there won't be breaking changes at runtime or JS projects. All typing changes are made optional; therefore, no breaking changes to existing TS project compilers.
  • Added type DataOf<S> infers the resulting type of the data argument of the given typeof Route.schema.
  • Added type alias inferData<S> = DataOf<S> for styling matching with z.infer (although it uses z.TypeOf internally).
  • Modified type RouteParameter<Z> now receives an optional generic zod type. This change is non-breaking and is required for later inference of the type.
  • Added 8 function overloads to Query, Path, and Header to implant the parameter type into RouteParameter<Z>.
  • Forced type overwrites on 12 of the 15 legacy parameter specifications, this is done to support as many legacy types as possible. Arguably, this implementation goes against the purpose of typescript, however, the existing specification of the constructor returning a zod type instead of the class instance is no better.

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:

  • Number | (use Num, Int or z.number() instead)
  • String | (use Str or z.string() instead)
  • Boolean | (use Bool or z.boolean() instead)
  • Num -> ZodNumber
  • Int -> ZodNumber
  • Str -> ZodString
  • DateTime -> ZodString
  • Regex -> ZodString
  • Email -> ZodString
  • Uuid -> ZodString
  • Hostname -> ZodString
  • Ipv4 -> ZodString
  • Ipv6 -> ZodString
  • DateOnly -> ZodString
  • Bool -> ZodBoolean
  • [<Z>] -> ZodArray<Z>
  • Arr | (use [<Z>] or z.array() instead)
  • Obj | (use z.object() instead)
  • Enumeration | (use z.enum() instead)

Detailed explanation:

  • Body schemas must be declared using zod types for type inference to work if it is more than 1 level deep as the current constraint does now allow inferring legacy types recursively.
  • Does not support inferring native types Number, String, and Boolean, there is no good way to support this as the definition is native and typeof Class returns function.
  • Does not support inferring Arr, Obj, and Enumeration in the current runtime specification as typescript does not support type inference out of typing context, const Arr: TypedArr<Z> = class Arr { /*... */ } does not work. Implementing support for these legacy types requires converting them to functions, however, this will break the new syntax.
  • Unsupported legacy types will all infer to any although the key is in the specification for code autocompletion.

Other changes

  • Bumped prettier version to ^3.1 to support prettifying infer ... extends ... ? ... : never syntax.

Copy link

github-actions bot commented Mar 4, 2024

🧪 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
Copy link
Member

G4brym commented Mar 4, 2024

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
Copy link
Contributor Author

iann838 commented Mar 4, 2024

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
Copy link
Member

G4brym commented Mar 4, 2024

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
Copy link
Contributor Author

iann838 commented Mar 4, 2024

@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
Copy link
Contributor Author

iann838 commented Mar 5, 2024

@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
Copy link
Member

G4brym commented Mar 5, 2024

@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
Copy link
Member

G4brym commented Mar 5, 2024

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
Copy link
Contributor Author

iann838 commented Mar 5, 2024

@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
Copy link
Contributor Author

iann838 commented Mar 5, 2024

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

@G4brym
Copy link
Member

G4brym commented Mar 11, 2024

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
Copy link
Contributor Author

iann838 commented Mar 11, 2024

@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
Copy link
Contributor Author

iann838 commented Mar 13, 2024

@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
Copy link
Member

G4brym commented Mar 15, 2024

This is really cool @iann838
I will be merging and publishing this as version 1.1.0 this Monday
Thanks 😃

@iann838
Copy link
Contributor Author

iann838 commented Mar 16, 2024

@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 G4brym merged commit c6a7e88 into cloudflare:main Mar 18, 2024
4 checks passed
@G4brym
Copy link
Member

G4brym commented Mar 18, 2024

@iann838 open an issue and we continue the discussion there

@G4brym
Copy link
Member

G4brym commented Mar 18, 2024

I will add some docs on this tomorrow before opening a new release 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants