chanfana icon indicating copy to clipboard operation
chanfana copied to clipboard

router deep refactoring discussion

Open iann838 opened this issue 11 months ago • 5 comments

This issue is for discussing the refactoring of this project to resolve issues and improve the developer experience. Here will be laid out topics and their potential improvements if any.

Legacy schema parameters

A major part of the developer experience involves type safety and IDE code auto-completion. Legacy parameters were not implemented in a way to support these improvements. In #130, an attempt to type legacy parameters ended up having to force type overwrites using as unknown as ...

  • Removing legacy parameters altogether and using zod types instead.

However, coercion in zod is rather tricky, z.coerce.number() means Number(...), an empty parameter will coerce to 0 instead of throwing a bad request with "parameter required", things are even worse with z.coerce.boolean(). My suggestion for fixing this is to preprocess the input with JSON parse only for z.number() and z.boolean() as these explicitly require coercion. Two ways to approach this:

  • Wrap z.preprocess(...) when the input type is z.number() and z.boolean() used with Query, Path, and Header.
  • Extend zod with a constant similar to z.coerce that wraps JSON parse preprocessing. This approach however requires developers to define them explicitly.

Unused handle parameters

Most of the time due to the existence of the data parameter in handle, the previous 3 parameters are very likely to be left unused: image

  • Instead of sequential arguments, they can be bundled into an object and later unpacking only what is needed handle({ req }, data) ....

Middleware

Currently, middleware functionality is limited, for example, the example provided in: https://cloudflare.github.io/itty-router-openapi/user-guide/middleware/. How many instances would developers like to have a User object with the authenticated user info accessible within handle?

A popular Python web framework FastAPI provides a solution by forcing middleware to be just "middleware" (like CORS, other general checks, etc.) and approaches authentication and other similar logic using Dependencies.

Repeated explicit typings

Due to how JS is implemented and how the TS compiler works, implicitly (or infer) typing a class method before it is defined is not possible, thus, developers have to explicitly type each of the parameters of the handle method after it is supposed to be typed already in schema, this also applies for the env parameter of handle. While writing extra typing may not be a problem, it could become a source of errors because it is explicit and not type-safe.

To improve this experience, it is required to make route definitions to be function-based. While I agree that class-based route definition is unique for this project, my argument is that it can still be unique with function-based routes. JS classes are just syntax sugar after all. I have a rather different syntax that I want to propose heavily inspired by FastAPI, although the background logic will mostly remain the same, the surface identify would shift dramatically:

router.post("/accounts/:accountId/todos/", {
    tags: ["Todos"],
    summary: "diruhgoerhgoie",
    description: "sroigheriog",
    deprecated: false,
    statusCode: 200,
    includeInSchema: true,
    responseClass: JSONResponse,
    responses: [
        Responds(200, "application/json", z.object({...})),
        Responds(400, "application/json", z.object({...})),
        Responds(401, "application/json", z.object({...})),
    ],
    parameters: {
        accountId: Path(z.coerce.bigint()),
        trackId: Query(z.string()),
        accept: Header(z.string()),
        todoItem: Body(z.object({
            id: z.string(),
            title: z.string(),
            description: z.string(),
        })),
    },
    dependencies: {
        session: Depends(authSession),
    }
},
({ req, env, ctx }, { accountId, trackId, accept, todoItem }) => {
    return new Response(accountId)
})

iann838 avatar Mar 18 '24 14:03 iann838