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: add initial support for hooks #1777

Merged
merged 30 commits into from Mar 10, 2022

Conversation

efectn
Copy link
Member

@efectn efectn commented Feb 14, 2022

Simple Usage:

func main() {
	app := fiber.New()

	app.OnResponse(func(c *fiber.Ctx, m fiber.Map) error {
		fmt.Print(c.Route())

		return nil
	})

	app.OnRoute(func(c *fiber.Ctx, m fiber.Map) error {
		fmt.Print(m)

		return nil
	})

	app.OnName(func(c *fiber.Ctx, m fiber.Map) error {
		fmt.Print(m)

		return nil
	})

	app.Get("/", func(c *fiber.Ctx) error {
		return c.SendString("-")
	}).Name("index")

	app.Listen(":5000")
}

Closes: #1758

To-Do List:

  • Add detailed tests.
  • Comment lines.
  • Create documentations.

@efectn efectn marked this pull request as draft Feb 14, 2022
@efectn efectn marked this pull request as ready for review Feb 18, 2022
app.go Outdated Show resolved Hide resolved
@efectn efectn requested review from ReneWerner87 and hi019 Feb 19, 2022
@ReneWerner87
Copy link
Member

ReneWerner87 commented Feb 21, 2022

gave him some tasks to move all the code into a separate struct called hooks.go(+tests) and thus reduce the size of the file for the app

@hi019 what do you think about the different hooks

@hi019
Copy link
Member

hi019 commented Feb 23, 2022

Will look tomorrow

@hi019
Copy link
Member

hi019 commented Feb 24, 2022

Looks good, I'll test it out tomorrow. But why do we need OnResponse/OnRequest when the same thing can be done with a middleware? Also what about changing the hook signature to not return anything, so that the hook handles any errors itself?

@efectn
Copy link
Member Author

efectn commented Feb 25, 2022

Looks good, I'll test it out tomorrow. But why do we need OnResponse/OnRequest when the same thing can be done with a middleware? Also what about changing the hook signature to not return anything, so that the hook handles any errors itself?

OnX methods don't return anything already.
Right about onResponse/onRequest, i can remove them.

hooks.go Outdated Show resolved Hide resolved
@hi019
Copy link
Member

hi019 commented Feb 26, 2022

I mean HookHandler:

 type HookHandler = func(*Ctx, Map) error

@efectn
Copy link
Member Author

efectn commented Feb 26, 2022

I mean HookHandler:

 type HookHandler = func(*Ctx, Map) error

i thought handling OnRoute and OnListen errors would be useful but if you're not OK, i can remove them.

@hi019
Copy link
Member

hi019 commented Mar 1, 2022

Actually, I think the current implementation is fine. I'll finish the review tomorrow

@hi019
Copy link
Member

hi019 commented Mar 2, 2022

What about using a struct instead of a map for the hook function params, to provide better type safety? It would be nice if each hook had its own struct. Also this print statement seems to be executed twice:

package main

import (
	"fmt"
	"log"

	"github.com/gofiber/fiber/v2"
)

func main() {
	app := fiber.New(fiber.Config{DisableStartupMessage: true})

	app.Hooks().OnRoute(func(c *fiber.Ctx, m fiber.Map) error {
		fmt.Println(m["route"].(fiber.Route).Path)

		return nil
	})

	app.Get("/hello", h)
	log.Fatal(app.Listen(":3001"))
}

func h(c *fiber.Ctx) error {
	return c.Status(fiber.StatusOK).SendString("Hello")
}

It's because app.register is being called twice somehow. Still looking into that

app.go Outdated Show resolved Hide resolved
@efectn
Copy link
Member Author

efectn commented Mar 2, 2022

What about using a struct instead of a map for the hook function params, to provide better type safety? It would be nice if each hook had its own struct. Also this print statement seems to be executed twice:

package main

import (
	"fmt"
	"log"

	"github.com/gofiber/fiber/v2"
)

func main() {
	app := fiber.New(fiber.Config{DisableStartupMessage: true})

	app.Hooks().OnRoute(func(c *fiber.Ctx, m fiber.Map) error {
		fmt.Println(m["route"].(fiber.Route).Path)

		return nil
	})

	app.Get("/hello", h)
	log.Fatal(app.Listen(":3001"))
}

func h(c *fiber.Ctx) error {
	return c.Status(fiber.StatusOK).SendString("Hello")
}

It's because app.register is being called twice somehow. Still looking into that

I think it's not bug because of Get creates GET and HEAD methods.

@efectn
Copy link
Member Author

efectn commented Mar 2, 2022

What about using a struct instead of a map for the hook function params, to provide better type safety? It would be nice if each hook had its own struct.

done

@hi019
Copy link
Member

hi019 commented Mar 2, 2022

I meant changing the hook handlers to be something like this:

// Old
type HookHandler = func(*Ctx, Map) error

// New
// naming could be improved
type OnRouteHandler = func(*Ctx OnRouteParams)

This will prevent typecasting:

app.Hooks().OnRoute(func(c *fiber.Ctx, m fiber.OnRouteParams) error {
	// Old
	fmt.Println(m["route"].(fiber.Route).Path)

	// New
	fmt.Println(m.Route.Path)

	return nil
})

@efectn
Copy link
Member Author

efectn commented Mar 2, 2022

I meant changing the hook handlers to be something like this:

// Old
type HookHandler = func(*Ctx, Map) error

// New
// naming could be improved
type OnRouteHandler = func(*Ctx OnRouteParams)

This will prevent typecasting:

app.Hooks().OnRoute(func(c *fiber.Ctx, m fiber.OnRouteParams) error {
	// Old
	fmt.Println(m["route"].(fiber.Route).Path)

	// New
	fmt.Println(m.Route.Path)

	return nil
})

Good idea. I'll change it. Also i plan to add OnGroup. Thanks for suggestions.

@efectn efectn requested a review from hi019 Mar 2, 2022
hi019
hi019 approved these changes Mar 3, 2022
@hi019 hi019 requested a review from ReneWerner87 Mar 3, 2022
@hi019
Copy link
Member

hi019 commented Mar 3, 2022

Also we'll need to update your docs pr with the struct changes

@efectn
Copy link
Member Author

efectn commented Mar 3, 2022

Also we'll need to update your docs pr with the struct changes

I'll update docs tomorrow.

@efectn
Copy link
Member Author

efectn commented Mar 5, 2022

Also we'll need to update your docs pr with the struct changes

I'll update docs tomorrow.

done

app.go Outdated Show resolved Hide resolved
hooks.go Outdated Show resolved Hide resolved
@ReneWerner87 ReneWerner87 merged commit bd20e90 into gofiber:master Mar 10, 2022
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants