ambiorix-web / ambiorix

🖥️ Web framework for R
http://ambiorix.dev
GNU General Public License v3.0
211 stars 9 forks source link

Pass error to handler #57

Closed kennedymwavu closed 9 months ago

kennedymwavu commented 9 months ago

Description

When an error occurs, it would be useful if it is passed to the handler. The handler can then choose what to do with the error (in most cases log it).

Currently, if I want to pass the error to the handler, I have to do a tryCatch() on the route and pass the error via res:

app <- Ambiorix$new()

app$get("/", \(req, res) {
  res$send("Hello, World!")
})

# simulate error (object 'Pikachu' is not defined)
app$get("/whoami", \(req, res) {
  tryCatch(
    expr = {
      res$send(Pikachu)
    },
    error = \(err) {
      # pass error via res:
      res$error <- err
      # force error:
      stop()
    }
  )
})

app$error <- \(req, res) {
  # retrieve error from res:
  err <- res$error
  # print the error:
  err_msg <- conditionMessage(err)
  cli::cli_alert_danger("Error: {err_msg}")

  res$status <- 500L
  res$send("Uhhmmm... Looks like there's an error from our side :(")
}

app$start(port = 8000L, open = FALSE)

Suggestion

To avoid having to do unnecessary tryCatch()s on every route, we could do this instead (similar to express js):

app <- Ambiorix$new()

app$get("/", \(req, res) {
  res$send("Hello, World!")
})

# simulate error (object 'Pikachu' is not defined)
app$get("/whoami", \(req, res) {
  res$send(Pikachu)
})

error_handler <- \(req, res, err = NULL) {
  if (!is.null(err)) {
    # print/handle the error here:
    err_msg <- conditionMessage(err)
    cli::cli_alert_danger("Error: {err_msg}")
  }

  # response to client:
  res$status <- 500L
  res$send("Uhhmmm... Looks like there's an error from our side :(")
}

app$error <- error_handler

app$start(port = 8000L, open = FALSE)

Hints

Still making myself familiar with the code base but I believe what I suggested can be done in these 3 sections:

I can make a pull request if need be.

JohnCoene commented 9 months ago

Ah yes you're correct, it should really have the error passed to it.

kennedymwavu commented 9 months ago

Thank you for the fix 60508af. It is working great!