-
Notifications
You must be signed in to change notification settings - Fork 71
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
Error handling in the Bridge #319
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Just couple of nits;
ResolveSerialized::Once(_) => { | ||
// The resolve has been used, turn it into a Never | ||
if let ResolveSerialized::Once(f) = | ||
std::mem::replace(self, ResolveSerialized::Never) | ||
{ | ||
f(bytes); | ||
f(bytes)? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be a return
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose it works because it's a result of unit type, just confusing because there's no semicolon, I guess
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah you're right, that just happens to be the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rewrote with let else
and unreachable
@@ -21,15 +21,24 @@ lazy_static! { | |||
|
|||
#[wasm_bindgen] | |||
pub fn process_event(data: &[u8]) -> Vec<u8> { | |||
CORE.process_event(data) | |||
match CORE.process_event(data) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't these effectively just unwrap()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the difference is just the message you get - the errors format pretty nicely.
This is a breaking change
It allows serde and resolve errors in the bridge to be caught and inspected, so instead of crashing the core, they can be passed to telemetry, etc.
to do