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 for sub fiber's error handlers #1560

Merged

Conversation

josebalius
Copy link
Contributor

@josebalius josebalius commented Oct 2, 2021

Please provide enough information so that others can review your pull request:

Closes #1438

It captures the mounted fiber's configured error handlers and of that fiber's subapps (it's recursive by nature). It is then searched by matching request path to prefix to find the best match. I figured there shouldn't be "that many" sub error handlers even in a large app, however, if this is the wrong assumption we will need a better data structure, something close to what the router does, to be honest. I opted to keep it simple and not over engineer it.

This is a submission for #hacktoberfest !

Explain the details for making this change. What existing problem does the pull request solve?

  • Mounted fiber and its sub-apps error handlers are now saved in a new App.errorHandlers map
  • New public App.ErrorHandler method that wraps the logic for which error handler to use on any given context
  • Error handler match logic based on request path <=> prefix accuracy
  • Typo fixes
  • Tests

Commit formatting

Use emojis on commit messages so it provides an easy way of identifying the purpose or intention of a commit. Check out the emoji cheatsheet here: https://gitmoji.carloscuesta.me/

- Mounted fiber and its sub apps error handlers are now saved a new
  errorHandlers map in App
- New public App.ErrorHandler method that wraps the logic for which
  error handler to user on any given context
- Error handler match logic based on request path <=> prefix accuracy
- Typo fixes
- Tests
@welcome
Copy link

welcome bot commented Oct 2, 2021

Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

@ReneWerner87
Copy link
Member

ReneWerner87 commented Oct 4, 2021

thanks for the code

@josebalius can you add some documentation for the new feature in our documentation repository https://github.com/gofiber/docs

@josebalius
Copy link
Contributor Author

@ReneWerner87 sure thing. Is https://github.com/gofiber/docs/blob/master/api/fiber.md the best place to add these docs? I am thinking of expanding on the ErrorHandler definition to mention that mounted fiber error handlers are retained by the top-level app and applied on prefix associated requests.

@ReneWerner87
Copy link
Member

I am thinking of expanding on the ErrorHandler definition to mention that mounted fiber error handlers are retained by the top-level app and applied on prefix associated requests.

sounds good

@josebalius
Copy link
Contributor Author

Here is docs PR: gofiber/docs#204

@josebalius
Copy link
Contributor Author

@ReneWerner87 is there something else I should do to get these PRs merged?

@ReneWerner87
Copy link
Member

No, its okay for now

@ReneWerner87 ReneWerner87 merged commit 587f3ae into gofiber:master Oct 5, 2021
@welcome
Copy link

welcome bot commented Oct 5, 2021

Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

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.

🐛 Custom error handler not working for a mounted app instance
2 participants