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

Proposal: Switch http-types::error::Error.error from anyhow to eyre #214

Open
jbr opened this issue Aug 5, 2020 · 9 comments · May be fixed by #215
Open

Proposal: Switch http-types::error::Error.error from anyhow to eyre #214

jbr opened this issue Aug 5, 2020 · 9 comments · May be fixed by #215
Labels
semver-major This change requires a semver major change

Comments

@jbr
Copy link
Member

jbr commented Aug 5, 2020

https://lib.rs/crates/eyre is a fork of anyhow that uses the https://lib.rs/crates/backtrace instead of std::backtrace::Backtrace. the backtrace crate currently provides a much better interface than std::backtrace::Backtrace for programmatic interaction (iterating over frames, for example, allowing for backtrace filtering)

This is important for a web framework because the framework can provide a highly usable in-browser interface to understand errors. Here's an example from the phoenix framework:
image

In development mode, they render a relevant snippet of code and a backtrace annotated with filesystem locations. IIRC, clicking any of those stack frames jumps to that line, syntax highlighted, in browser. This is obviously way beyond what http-types, rust, and tide can currently offer, but switching to eyre and the backtrace crate would allow us to take further steps in that direction.

@jbr
Copy link
Member Author

jbr commented Aug 5, 2020

cc @yoshuawuyts @Fishrock123 because there's no way to request review on an issue

@yoshuawuyts
Copy link
Member

Not opposed to this in principle; I'm curious how a PR would turn out. The Elixir page you shared looks fantastic and enabling that would be amazing.

One gotcha tho is that (I believe) we recently introduced an explicit "from anyhow" conversion which we cannot remove without a major version bump. So at least for a while we would need to keep both dependencies in tree.

@jbr
Copy link
Member Author

jbr commented Aug 5, 2020

Yeah, I do think this would have to be a major release change. Unless eyre provides a Fromanyhow::Error for eyre::Error, this would necessarily be a breaking change. Without specialization I don't think we can support both anyhow and eyre simultaneously (not sure about that, but would be pleasantly surprised if it was possible)

@jbr
Copy link
Member Author

jbr commented Aug 5, 2020

also see #213 for a screenshot from a tide app that's a step in the direction of the phoenix error page

@jbr
Copy link
Member Author

jbr commented Aug 5, 2020

On further investigation, eyre might not be the solution either due to eyre-rs/eyre#32. We might just want to capture our own backtrace::Backtrace. I'll take a look at that, shouldn't be too bad

@jbr jbr closed this as completed Aug 5, 2020
@jbr jbr reopened this Aug 5, 2020
@Fishrock123
Copy link
Member

From<anyhow::Error> for eyre::Error

That seems reasonable to request/PR to eyre?

@jbr
Copy link
Member Author

jbr commented Aug 5, 2020

@Fishrock123 there's a closed issue about that eyre-rs/eyre#31 and we can't add our own From<anyhow::Error> for http_types::Error because of our generic From<E: Into<eyre::Error>> for http_types::Error (I believe we'd get one of those compiler errors about how anyhow could could implement Into<eyre::Error> for anyhow::Error or something like that)

@jbr
Copy link
Member Author

jbr commented Aug 5, 2020

We weren't anyhow compatible prior to 2.3.0, shouldn't be a big deal to remove that in 3.0.0 in exchange for better backtraces

@jbr jbr linked a pull request Aug 6, 2020 that will close this issue
@Fishrock123 Fishrock123 added the semver-major This change requires a semver major change label Jan 25, 2021
@Fishrock123
Copy link
Member

It'd be neat to have this at least as a optional feature

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major This change requires a semver major change
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants