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

🔥 Support returning response from handler #218

Closed
alinnert opened this issue Mar 11, 2020 · 4 comments
Closed

🔥 Support returning response from handler #218

alinnert opened this issue Mar 11, 2020 · 4 comments
Assignees

Comments

@alinnert
Copy link

Is your feature request related to a problem?

It's a very common pattern while building an API to call a function, check the err and send a response to the client. At this point the handler should also end executing. Normally this looks like following:

user, err := getUser()
if err != nil {
  c.JSON(someErrorStruct)
  return
}

For demonstration: in this example this pattern repeats 29 times in ~210 LOC.

The problem is: it's super super easy to forget one of these returns. This leads to hard to detect bugs that can't be caught by the language.

Describe the solution you'd like

One solution would be to make it possible to return results instead of calling a function to send them. Like:

user, err := getUser()
if err != nil {
  return someErrorStruct
}

(plus: this would also reduce the linked example code by 29 LOC)

There are multiple ways to implement this. Some alternatives are:

if err != nil {
  return &fiber.JSONResponse{
    Status: 403,
    Body: someErrorStruct
  }
  // or
  return c.JSONResponse(someErrorStruct, 403)
  // or
  return c.JSONResponse(someErrorStruct).withStatus(403)
}

This feature would be purely additive. It should not be necessary to use this over c.Send() or c.Write(). Therefore it also shouldn't break any existing code.

Describe alternatives you've considered

What I could do is write a wrapper function that calls the real handler, accepts anything the handler returns and takes care of sending the response. But I would need to wrap every single handler (possibly also middleware) with this wrapper. That would look like this:

app.Get('/', wrapper(indexHandler))

I want to avoid that if that's possible.

Additional context

I found a few frameworks that actually support this way of sending a response, for Go and other languages. One example for Go would be Atreugo.

@welcome
Copy link

welcome bot commented Mar 11, 2020

Thanks for opening your first issue here! 🎉 Be sure to follow the issue template!

@Fenny
Copy link
Member

Fenny commented Mar 11, 2020

I do agree that appending a return within if statements can be tedious.
However, Fiber is designed to have no return value for simplicity.

Your example can be written in different ways to avoid using return

app.Get("/", func(c *fiber.Ctx) {
  var err error
  if user, err := getUser(); err == nil {
    c.JSON(user)
  } else {
    c.JSON(errStruct)
  }
})

If we want to implement a return one liner we need to add a return value to the handlers.

app.Get("/", func(c *fiber.Ctx) error {
  user, err := getUser()
  if err != nil {
    return c.JSON(errStruct)
  }
  return c.JSON(user)
})

This also means that we have to document which Ctx methods support error returns.
I think we will over complicate the handler, Ctx methods and documentation for some extra space.

What are your thoughts? @koddr

@alinnert
Copy link
Author

Oh, right. So return also turns into return nil, right? Then this would be a breaking change. This changes quite a bit.

if { ... } else { ... } would be fine for one error check, but it turns into a deeply nested if else tree for more complex handlers - which isn't really uncommon. So, that's not a very usable alternative.

But, the repetition isn't really an issue for me. It's mainly a source of errors I trip over again and again. This is what really bugs me.

@Fenny
Copy link
Member

Fenny commented Mar 14, 2020

@alinnert, the only solution I see is to create an middleware for this because we don't want to break any code. I'm closing this for now, thank you for your input and ideas, might be a thing for v2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants