router deep refactoring discussion

Question

router deep refactoring discussion

iann838 opened this issue 9 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:

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)
})

Answer 1 · 2024-03-18T15:03:12.000Z

Hey @iann838

Regarding the Legacy schema parameters:
I think we should keep them, while improving it, we can drop the class behavior and make them just a function like Path and Query is today, doing this would solve the as unknown as ..
Wrapping with z.preprocess(...) when using with header, path or query is also a very good suggestion as most developers spend a lot of time debugging this issue today when using zod directly

Regarding the Unused handle parameters:
I think we can use the same approach as hono uses today, that is passing the validated data inside the request parameter
this would solve the unused parameters warning, and the data would be accessible as request.data

Regarding the middleware
I like the Depends approach, and this would solve some currently open issues
i'm just unsure where developers using the class based endpoint would define it, would a new property inside the schema called dependencies be a good option?

Regarding the Repeated explicit typings:
I think we can have a function based definition, as for some use cases, defining a complete class and then having a handle function with 2 or 3 lines is a bit overkill

Currently, we tried to keep the schema property as close as possible to the real openapi definition with just some shiny quality of life improvements, this meant developers how knew openapi already to not need to learn a complete framework from start, and could get up to speed almost without reading the documentation
I'm just unsure if we should go the Fastapi parameters schema approach with almost everything on the root object like you are suggesting for example statusCode and responseClass

Improved class based endpoints
Another braking change i thought we could do is having the class based endpoint behave like Django class based views
where instead of having the main code inside the handle function you could change the function name to get or post, patch, etc, and then you didn't need to verify the request.method inside your code, as we could do that before calling the function.
This could easily be done by having a default handle function the OpenAPIRoute class, that would do this routing, the means already existing applications didn't need to do anything, as the request always hits the handle function first, and then new endpoints or new application could start using this new function naming.
The main feature for this is that users could inside a single class have a get method to return maybe html with a form, and then the post method to validate the submitted form, and share some code between these.

My question is how would the route registration on the router look like for this, maybe we can just tell developers to register the this kind of endpoints with .all and then in the default handle function that does the. routing when a given method is request but the function we expect is not defined, the handle function would just not return anything, resulting on the current router to continue calling matching routes like what middlewares do today

@meddulla can you share your thoughts here

Answer 2 · 2024-03-18T16:20:11.000Z

@G4brym

Responding to Django class-based views

Route registration will likely look like views.py, a given route points to a given view regardless of method or other information.
How do we know if a method is defined so we can send it to openapi.json? There are only unconventional ways to know if a method was redefined on a subclass, and not defining it on a base class will allow type-unsafe method definitions in a subclass.
How do we define schemas for multiple methods? GET and POST will likely share different paths (/items, /items/:id), and GET PUT DELETE PATCH will likely not use the same schemas.
Still cannot infer parameter types without explicit redefinition.
Django class-based views route definition uses .as_view(), which returns a callable roughly like the function-based views.

Not to say that class-based views are bad, I use them in production to this date, it's just that it is hard to make it brilliant in JS as the language is not as scriptable as Python (of course, Python is way slower for a reason).

As a long-time Django user (in truth, it was the first web framework I used in production), I have come to dislike it over time. Here are my thoughts after using them for years:

For it to support schema definition and validation (Django is a traditional HTML first framework), it requires Django-REST-Framework (DRF), its schema and view definitions are considered overly complex and nested when it could very well be simplified a lot.
Views logic definition is rather scattered, a bit of the same logic is being dispersed across multiple places making it hard to debug (to give some perspective, kind of how Vue 2 looked like a few years back).

Answer 3 · 2024-04-06T20:54:10.000Z

Hi @G4brym, I have some news here:
Due to the complexity and effort of refactoring an existing package while being limited, I have decided to write a brand-new package from the ground up instead.

As an open-source enthusiast, of course, it will be open-source and here I have an alpha version of what I wrote: Workery. Feel free to revise and try it out.

I do not know where the new package will lead, whether it will be widely promoted and used among developers or quietly used by a limited population. Continuous support of the package will also depend on this factor ...

Answer 4 · 2024-04-11T14:31:43.000Z

Hey @iann838 yeah, i think that's probably the best way to implement these improvements, itty-router-openapi cannot do much of the changes you are proposing due to backwards compatibility
I really like way you made defining endpoints, looks like fastapi for js
Share your work with the community on discord they will like it, make sure to advertise it something like fastapi for workers for people to click and go see it 😄
https://discord.com/channels/595317990191398933/996501505936994374

Answer 5 · 2024-04-12T03:43:58.000Z

Alright, that will require me finishing the docs first, which is probably tomorrow