Use zod-openapi for API documentation#933
Conversation
| z.string().meta({ example: 'Bad request.' }), | ||
| ).meta({ | ||
| id: 'BadRequestError', | ||
| }) |
There was a problem hiding this comment.
I'm just starting to dig into your code (so take this first comment with a grain of salt): Don't we have the factory methods in factories.ts for creating error schemas? Plus: The implementation here seems to contain more information than the factory version. 🤔
Is there a reason we have both?
There was a problem hiding this comment.
How come we don't use the location schema in the device schema? It seems to me that we are defining what a location is in multiple locations.
| { | ||
| "openapi": "3.1.0", | ||
| "info": { | ||
| "title": "My API", |
There was a problem hiding this comment.
Is My API overwritten somewhere? If not, we should give this a meaningful name 😉
There was a problem hiding this comment.
So far, only the documentation part of the routes has been replaced with the zod schema implementaiton but the logic itself (e.g. in this file line 285 onward) is still implemented by hand and doing the validation and error handling itself. According to my understanding, this should actually be covered with the zod schemas right? 🤔
There was a problem hiding this comment.
I did a bit of research: Maybe we could already benefit from the schemas if we use them for the validation of incoming requests during runtime? If we put that into a middleware, we would not need to adjust all the route implementations.
There was a problem hiding this comment.
Another thought: The ticket is about using another (here: zod-openapi) documentation framework. You could argue that we are thus good with "only" using it for dumentation purposes in this PR. Some kind of Schema validation would still be nice, though
| badRequestResponse, | ||
| internalServerErrorResponse, | ||
| notFoundResponse, | ||
| } from '~/lib/openapi/errors' |
There was a problem hiding this comment.
Two import statements for the same file? Also in the other routes
|
|
||
| import * as z from 'zod/v4' | ||
| import 'zod-openapi' | ||
| import { type ZodOpenApiPathItemObject } from 'zod-openapi' |
| internalServerErrorResponse, | ||
| methodNotAllowedResponse, | ||
| notFoundResponse, | ||
| } from '~/lib/openapi/errors' |
There was a problem hiding this comment.
Is this route only for the deletion of measurements?
| }, | ||
| } | ||
|
|
||
| export async function action({ request, params }: Route.ActionArgs) { |
There was a problem hiding this comment.
Method seems to duplicate logic and error handling that is already in the schemas?
| import { | ||
| badRequestResponse, | ||
| internalServerErrorResponse, | ||
| } from '~/lib/openapi/errors' |
3 participants
Type of Change
Implementation
As a POC I have replaced the documentation for the sign-in route with one generated via zod-openapi and corresponding zod schemas (that could potentially be shared between routes).
The docs page also got a little bit of an overhaul to make it easier to differentiate between regular API docs and integrations docs.
The zod schema makes the code of the actual api handler a lot more simple (there is almost no logic anymore). All of the "complexity" is within the schema definition which does generate its entire documentation as a side product. This nicely keeps the documentation aligned with the actual implementation and not only with whats supposedly implemented (a problem we have with the old docs and one that I came across while implementing this too!).
The build step is now also gone. What looks like a runtime generation is a little smarter though: Vites
import.meta.globis actually derived via statical analysis and thus should be very fast.Totally open for opinions. Should I continue this?
Sidenote: Since there is nothing generated about the integrations spec, we might want to place it somewhere else or even convert it to an actual .yml file once and just put it under public assets?
There must be a better way than this right now... 🤔
Checklist
devbranchAdditional Information