gofiber / fiber

⚡️ Express inspired web framework written in Go
https://gofiber.io
MIT License
33.88k stars 1.66k forks source link

🚀 [Feature]: v3 Router interface change #2160

Closed pjebs closed 2 years ago

pjebs commented 2 years ago

Feature Description

v2 definition:

type Router interface {
    Use(args ...interface{}) Router

    Get(path string, handlers ...Handler) Router
    Head(path string, handlers ...Handler) Router
    Post(path string, handlers ...Handler) Router
    Put(path string, handlers ...Handler) Router
    Delete(path string, handlers ...Handler) Router
    Connect(path string, handlers ...Handler) Router
    Options(path string, handlers ...Handler) Router
    Trace(path string, handlers ...Handler) Router
    Patch(path string, handlers ...Handler) Router

    Add(method, path string, handlers ...Handler) Router
    Static(prefix, root string, config ...Static) Router
    All(path string, handlers ...Handler) Router

    Group(prefix string, handlers ...Handler) Router

    Route(prefix string, fn func(router Router), name ...string) Router

    Mount(prefix string, fiber *App) Router

    Name(name string) Router
}

v3 definition:

It would be good if the signature could be changed for these:

    Get(path string, handler Handler, middleware ...Handler) Router
    Head(path string, handler Handler, middleware ...Handler) Router
    Post(path string, handler Handler, middleware ...Handler) Router
    Put(path string, handler Handler, middleware ...Handler) Router
    Delete(path string, handler Handler, middleware ...Handler) Router
    Connect(path string, handler Handler, middleware ...Handler) Router
    Options(path string, handler Handler, middleware ...Handler) Router
    Trace(path string, handler Handler, middleware ...Handler) Router
    Patch(path string, handler Handler, middleware ...Handler) Router

    Add(method, path string, handler Handler, middleware ...Handler) Router
    All(path string, handler Handler, middleware ...Handler) Router

Secondly,

Add(methods []string, path string, handler Handler, middleware ...Handler) Router

OR alternatively,

method string interprets methods with pipes: eg. "GET|POST"

Additional Context (optional)

It will be operationally equivalent to (but cleaner):

func WithMW(handler fiber.Handler, middleware ...fiber.Handler) []fiber.Handler {
    stack := make([]fiber.Handler, 0, len(middleware)+1)
    stack = append(stack, middleware...)
    return append(stack, handler)
}

app.Get("/hello", WithMW(func(c *fiber.Ctx) error {
    return c.SendString("Hello, World 👋!")
})...)

Code Snippet (optional)

No response

Checklist:

efectn commented 2 years ago

I think current interface is simple and cleaner. We also aim to be compatible with Express.js in most cases. What do you think @ReneWerner87

pjebs commented 2 years ago

I think the proposed interface is clearer in terms of what the developer's trying to achieve. @Fenny What do you think?

li-jin-gou commented 2 years ago

I think the proposed interface is clearer in terms of what the developer's trying to achieve. @Fenny What do you think?

I have observed that other frameworks such as Gin and Hertz have similar routing interface with Fiber. What are the key reasons to think the proposed interface is clearer for developer?

pjebs commented 2 years ago

Let's say you have a handler. A handler's job is to produce a response. The middleware's job is to modify the request/response in someway and/or create some kind of side-effect.

The current interface is a bit confusing in terms of what is happening behind the scenes.

  1. We know that a Get must have a handler.

Therefore, app.Get("/", handler).

  1. If we want to add middleware, we need to then add the middleware first in the stack and then the handler:

i.e.

app.Get("/", mw1, mw2, handler) to get the intuitive outcome.

  1. If we change the interface so that 1 argument is explicitly the handler, everything is crystal clear. If the developer wants to add middleware, they can add it as optional arguments.

app.Get("/", handler) ==> if we now chose to add mw ==> app.Get("/", handler, mw1)

This is how laravel (possibly the most popular web framework) does it:

https://laravel.com/docs/9.x/middleware#assigning-middleware-to-routes


Route::get('/profile', function () {
    // handler
})->middleware('auth'); // adding optional middleware

Route::get('/', function () {
    // handler
})->middleware(['first', 'second']);

use App\Http\Middleware\EnsureTokenIsValid;

Route::get('/profile', function () {
    // handler
})->middleware(EnsureTokenIsValid::class);

Look at how cleaner that looks compared to below:

Laravel also allows the less preferred way (they don't even document it anymore):


Route::get('/',['middlware' => 'auth', function () {
     // handler Code goes here
}]);

Route::get('/', ['middleware' => ['first', 'second'], function () {
    // handler Code goes here
}]);
ReneWerner87 commented 2 years ago

i like some parts of the idea

changing the parameter structure, so that for the methods where a handler is really always needed, there is also a fixed handler in the method declaration I find very good

also the idea with the add with multiple methods maybe we can implement this with generics so that we allow single and lists

otherwise i miss some methods in the router interface for the new version, like the USE and GROUP method

mirusky commented 2 years ago

Personal thoughts:

With the proposed change it will be strange to figure out how to migrate to fiber. Imagine comming from gin, echo or another one (express maybe) with Use and Group and suddenly it don't have these methods. As a developer I probably will give up.


Real Discussion:

The current interface is a bit confusing in terms of what is happening behind the scenes.

I think currently interface is good, simple and easy to understand. The current interface has same methods as others frameworks and also it's pretty simillar to express (as the philosophy of this project)

If we want to add middleware, we need to then add the middleware first in the stack and then the handler

Yes, many frameworks works in this way in many languages like C#, Java, Javascript... The only exception is php.

So the big question is, "what is a middleware?"

Middleware is software that's assembled into an app pipeline to handle requests and responses. Each component:

And we have terminal middleware who is responsable to process the request itself (illustrated as mw3):

image

And It could be easly translated to code as following:

app.Get("/", mw1, mw2, mw3)

The main problem of new developers is think that a handler and middleware are different, but they are pretty much the same.

pjebs commented 2 years ago

@mirusky Use and Group will still exist and operate as per v2. The handler and middleware will operate exactly like v2 and this proposal is mostly semantics.

Use and Group can be used to apply middleware more generally (and to multiple routes). The proposal is for the Method routes. Those routes (in practice) must have a handler. The new proposal reinforces that. If you want to then add ad-hoc middleware to that route specifically, the new signature also gives that freedom - but emphasises that they are optional.

This signature encourages better practice. i.e. Use Use and/or Group to add middleware generally.

efectn commented 2 years ago

Secondly,

Add(methods []string, path string, handler Handler, middleware ...Handler) Router

OR alternatively,

method string interprets methods with pipes: eg. "GET|POST"

I think it's better idea to do it slice. It may increase ns/op to split string like GET|HEAD

efectn commented 2 years ago

Hi. You can create a PR on v3-beta for these updates. If someone wants to apply these changes, i can assign them to this issue.

pjebs commented 2 years ago

Assign to me

efectn commented 2 years ago

Assign to me

Done

mirusky commented 2 years ago

@mirusky Use and Group will still exist and operate as per v2. The handler and middleware will operate exactly like v2 and this proposal is mostly semantics.

Use and Group can be used to apply middleware more generally (and to multiple routes). The proposal is for the Method routes. Those routes (in practice) must have a handler. The new proposal reinforces that. If you want to then add ad-hoc middleware to that route specifically, the new signature also gives that freedom - but emphasises that they are optional.

This signature encourages better practice. i.e. Use Use and/or Group to add middleware generally.

Idk, MIDDLEware is something in the middle, how that semantics are better than the actual one?

Just because the php framework does, doesn't means thats better. As I said before:

Is cleaner to read and figure out the flow path using app.Get("/", mw1, mw2, handler) than app.Get("/", handler, mw1, mw2)

Imagine a novice reading and thinking that handler is executed before the mw1 and mw2. If we gonna follow any framework semantics why not use the most common pattern in the principal languages ?

pjebs commented 2 years ago

Just some points:

Is cleaner to read and figure out the flow path using app.Get("/", mw1, mw2, handler) than app.Get("/", handler, mw1, mw2)

  1. No-one needs the "text" to be ordered to be graphically continually reminded how the flow path works.

  2. Unless you are using a text-based editor like Vim, all modern IDE's will inform the developer of the function's signature.

Get(path string, handler Handler, middleware ...Handler) Router

When the coder sees the parameter name: handler, they know where it belongs in the flow-path. When they see the (optional) parameter name: middleware, they know where it belongs in the flow: in the middle.

  1. handler argument can be nil and the entire function can contain just middleware (including the 'handler' as 'middleware') to recreate what @mirusky wants to do.
mirusky commented 2 years ago

Maybe I'm little late, but I opened a survey in Reddit.

Asked opinion about it and they like the current style with Method(route, handlers...).

The time I'm writing was 217 votes. 116 v2 style and 101 v3 style.

In the entire survey v2 style was ahead. People also suggested using some kind of "decorator" like:

Get(route, handler).With(middlewares...)

The survey could be found here

pjebs commented 2 years ago

Awesome survey:

This comment was one of the reasons for my proposal: https://www.reddit.com/r/golang/comments/yj9ah8/comment/iuopr8w/?utm_source=reddit&utm_medium=web2x&context=3

mirusky commented 2 years ago

Awesome survey:

This comment was one of the reasons for my proposal: https://www.reddit.com/r/golang/comments/yj9ah8/comment/iuopr8w/?utm_source=reddit&utm_medium=web2x&context=3

Yeah, there's misunderstanding about it. Of course it's not clear for someone who doesn't use the fiber.

But as someone said, our codebase may have a different experience and understanding about it.

efectn commented 2 years ago

We created voting in Fiber's discord server https://discord.com/channels/704680098577514527/848707314180423750/1037295391336583220

pjebs commented 2 years ago

It would have been good if my reasoning for the change was provided before people voted. Perhaps a link to this page.