Skip to content
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

🚀 [Feature]: v3 Router interface change #2160

Closed
3 tasks done
pjebs opened this issue Oct 18, 2022 · 18 comments · Fixed by #2176
Closed
3 tasks done

🚀 [Feature]: v3 Router interface change #2160

pjebs opened this issue Oct 18, 2022 · 18 comments · Fixed by #2176
Assignees
Milestone

Comments

@pjebs
Copy link
Contributor

pjebs commented Oct 18, 2022

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:

  • I agree to follow Fiber's Code of Conduct.
  • I have checked for existing issues that describe my suggestion prior to opening this one.
  • I understand that improperly formatted feature requests may be closed without explanation.
@efectn
Copy link
Member

efectn commented Oct 18, 2022

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
Copy link
Contributor Author

pjebs commented Oct 18, 2022

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
Copy link
Contributor

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
Copy link
Contributor Author

pjebs commented Oct 18, 2022

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
Copy link
Member

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
Copy link
Contributor

mirusky commented Oct 19, 2022

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:

  • Chooses whether to pass the request to the next component in the pipeline.
  • Can perform work before and after the next component in the pipeline.

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
Copy link
Contributor Author

pjebs commented Oct 19, 2022

@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
Copy link
Member

efectn commented Oct 19, 2022

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 efectn added this to v3 Oct 19, 2022
@efectn efectn added this to the v3 milestone Oct 19, 2022
@efectn efectn moved this to Todo in v3 Oct 19, 2022
@efectn
Copy link
Member

efectn commented Oct 19, 2022

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
Copy link
Contributor Author

pjebs commented Oct 19, 2022

Assign to me

@efectn
Copy link
Member

efectn commented Oct 19, 2022

Assign to me

Done

@mirusky
Copy link
Contributor

mirusky commented Oct 20, 2022

@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
Copy link
Contributor Author

pjebs commented Oct 26, 2022

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.

@efectn efectn moved this from Todo to In Progress in v3 Oct 28, 2022
@efectn efectn linked a pull request Oct 28, 2022 that will close this issue
@efectn efectn closed this as completed Nov 1, 2022
Repository owner moved this from In Progress to Done in v3 Nov 1, 2022
@mirusky
Copy link
Contributor

mirusky commented Nov 1, 2022

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
Copy link
Contributor Author

pjebs commented Nov 1, 2022

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
Copy link
Contributor

mirusky commented Nov 2, 2022

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
Copy link
Member

efectn commented Nov 2, 2022

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

@pjebs
Copy link
Contributor Author

pjebs commented Nov 2, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

5 participants