Update CombinedError API to expose the combinedError record as the core module type t. #109
Conversation
…re module type t. Co-authored-by: Avery Morin <[email protected]>
docs/api.md
Outdated
|
|
||
| Due to the verbosity of the above, sometimes it isn't useful to handle every potential case around errors in your application. When you just want a simple error message of what went wrong to display to the user, just access the `message` field on the `CombinedError.t`: | ||
|
|
||
| ```reason |
There was a problem hiding this comment.
For me this ends up being what I use 90% of the time, any thoughts on putting it before the more complex solution?
There was a problem hiding this comment.
It's also how I mainly handle errors in the front-end, I would also prefer if it was above the more complex way to handle errors just for the sake of it and to not confuse beginners
There was a problem hiding this comment.
Sounds good, thanks for the feedback!
|
The version doesn't make much difference to me personally, I can't imagine not upgrading either way :P Thanks for adding the docs, it's great to have something for that. Thanks for checking out the coda code :) though ironically writing code like that is replaced easily with e.message (and I actually removed that code from our codebase haha)! I wonder if it's worth writing something that would return some different kind of react component depending on the kind of error, or in one case perform some side effect like recording a log message... I guess it doesn't matter much either way. |
|
@Schmavery that's an interesting idea. It would be cool to handle all of the more complex parsing of |
|
Sorry, I didn't mean that the library should do that sort of thing, I meant that the example could do something other than stringifying the error, since e.message is already that. |
|
I think that'd be a worthwhile example to show. I'm considering starting an |
Fix #106
This PR updates our
CombinedErrorAPI to pass thecombinedErrorrecord as the module typetthat users will interact with. This feels more ergonomic to me and I believe @Schmavery felt the same. I also added the beginnings of docs on how to useCombinedError. I decided it wasn't useful to add a.reifile for this module b/c it is full of a bunch of BuckleScript accessors generated by all the[@bs.deriving abstract], and we basically want the entirety of the module public anyway.In terms of how we want to release this, I'm a little torn. It is a small breaking change that will (I suspect) break folks' builds, but it also feels fairly minimal. I'm inclined to release it as a minor (
v1.1.0) but could also consider a patch (v1.0.2) if others feel that works better. cc/ @Schmavery @gugahoa let me know your thoughts here. @Schmavery I co-authored you on the commit by the way, since I slightly modified the error modifier you have on Coda for the example here (it's really nice btw, love it).