-
Notifications
You must be signed in to change notification settings - Fork 42
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
Conversation
🧪 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 https://prerelease-registry.devprod.cloudflare.dev/itty-router-openapi/runs/8294915631/npm-package-itty-router-openapi-130 |
Woow @iann838 this is really cool 😃 |
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. |
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. 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. |
@G4brym Yes, there is a way, it's just not possible in how routes are being defined as of currently. 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. |
@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? |
@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'
});
}); |
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 |
@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 |
@G4brym I agree, let's go back to whether supporting legacy types or just doing a major version jump then. |
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: Here's the documentation of parameters for example, where you can see that url params are located in |
@G4brym I have added commit, and now it correctly places all queries in |
@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 |
This is really cool @iann838 |
@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? |
@iann838 open an issue and we continue the discussion there |
I will add some docs on this tomorrow before opening a new release 👍 |
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
Implementation details
DataOf<S>
infers the resulting type of thedata
argument of the giventypeof Route.schema
.inferData<S> = DataOf<S>
for styling matching withz.infer
(although it usesz.TypeOf
internally).RouteParameter<Z>
now receives an optional generic zod type. This change is non-breaking and is required for later inference of the type.Query
,Path
, andHeader
to implant the parameter type intoRouteParameter<Z>
.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
| (useNum
,Int
orz.number()
instead)String
| (useStr
orz.string()
instead)Boolean
| (useBool
orz.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>]
orz.array()
instead)Obj
| (usez.object()
instead)Enumeration
| (usez.enum()
instead)Detailed explanation:
Number
,String
, andBoolean
, there is no good way to support this as the definition is native andtypeof Class
returnsfunction
.Arr
,Obj
, andEnumeration
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 thenew
syntax.any
although the key is in the specification for code autocompletion.Other changes
prettier
version to^3.1
to support prettifyinginfer ... extends ... ? ... : never
syntax.