-
Notifications
You must be signed in to change notification settings - Fork 83
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
Eyre #215
base: main
Are you sure you want to change the base?
Eyre #215
Conversation
ooh, as a library you'll probably want to avoid hard coding a dependency on one hook or another. I think this means I really should add back the object provider API in Eyre so libraries can do hook agnostic context lookup. Let me make some PRs to Eyre, stable-eyre, and color-eyre. then you'll need to use a diff fn to extract the backtrace but it will work for both stable-eyre and color-eyre. |
What if we just use That still allows the benefits and makes more structural sense. It also means that downstream could choose which reporter (such as color-eyre). Additionally, perhaps we could make this a build option (feature)? We could then play around with it - I'm willing to try it out in my employer's application. I'm probably going to make my own branch to check things anyways, so I don't mind picking this up. |
If you don't mind carrying the StatusCode along side the Report instead of inside, which this PR seems to imply already, then that's definitely the best option. I'm still a little sad about it. I wrote two PRs of different designs I was considering for how we could allow composable error reporting, or even extra handlers that you could push into reports from libraries, but neither of them really seemed like the right solution so they've kinda stalled.
My hope is that we'd eventually be able to consistently store the status code inside of the reporter, and retrieve it, and have little risk of users accidentally discarding them. Not there yet :/ |
@yaahc I noted in #263 (comment) (my rebase of this PR) that changing the underlying type and then having a global hook doesn't really solve anything for Tide in particular, which is where this issue is most focused for me. In Tide all runtime errors stay within Tide - any that end up at the response are converted into a |
Probably, but I'll need more details:
Deal with how, just by logging or discarding them?
might also need clarification here, depending on the answer to the previous question. |
Tide turns all returns of A bare tide server does nothing with this information (on purpose, to avoid leaking internal error messages) other than use it's status code. The default log middleware, or a user log middleware, error body transform middleware, or some other mechanism, must grab an optional error off the request to check if there was one, and then can decide what to do with it (use something from it, downcast it, etc). I think the default |
Okay, I think I'm following so far. And what extra features did you need to get from the wrapped |
this is on top of the changes in #212 and not mutually exclusive with that or #213. The eyre-specific changes are in 46d2a1c
Tests are failing because somehow the Once is getting poisoned in testsThe Once is necessary because there's no obvious "app start" hook in http-types. One possibility is to just provide the backtrace method and require tide or some consumer to register stable-eyre. Or, ideally, we could just enable it with a feature flag on eyre, but eyre wants to be configured at runtime.
PR-ing this anyway for review and discussion
and maybe someone can help me figure out why the Once is getting poisoned in test? @yoshuawuyts @yaahcThe upside is that this enables backtraces with files and line numbers, which seems like a big usability advantage over just a module name, and we can do stuff like filtering the backtrace by path in tide
Closes #214