-
Notifications
You must be signed in to change notification settings - Fork 7
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
[PM-13660] implement catchable non generic error types #31
[PM-13660] implement catchable non generic error types #31
Conversation
this won't fully work because the TS types will be appended into crates that won't necessarily have WASM enabled
[bitwarden_error(flat)] on struct was doing basically the same thing as #[bitwarden_error(flat)] so I'm removing it
No New Or Fixed Issues Found |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #31 +/- ##
==========================================
+ Coverage 62.87% 63.39% +0.51%
==========================================
Files 179 184 +5
Lines 12699 12935 +236
==========================================
+ Hits 7985 8200 +215
- Misses 4714 4735 +21 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
this is mostly because of `bitwarden_core::Error` which exports as `Error` and overrides the JS `Error` type.
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, looks pretty good to me, some initial comments on the macro implementation.
let variant_ident = &variant.ident; | ||
let variant_name = format!("{}", variant_ident); |
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.
There's a bit of repetition with these lines for all the branches, can they be moved outside the match?
Also small subjective choice, but variant_ident.to_string()
is the same as format!("{}", variant_ident)
, but I think it looks nicer
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.
Good catch, I'll fix both!
#wasm | ||
|
||
#[automatically_derived] | ||
impl FlatError for #type_identifier { |
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.
When referring to types in macro generated code, we want to always use the fully qualified names, otherwise we're requiring users to import the correct type. If you're importingprelude::*
that's not an issue, but nothing keeps the user from directly importing just the macro.
impl FlatError for #type_identifier { | |
impl ::bitwarden_error::prelude::FlatError for #type_identifier { |
Note that we also use ::
in front of the crate name, this is to avoid collisions with user defined modules using the same name.
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 did this for types that we depend on (e.g. wasm-bindgen
) but never considered our own types, good catch!
🎟️ Tracking
📔 Objective
Every error outputs a function
is<type_name>()
that can be used from TS to check and cast errors.Example:
⏰ Reminders before review
team
🦮 Reviewer guidelines
:+1:
) or similar for great changes:memo:
) or ℹ️ (:information_source:
) for notes or general info:question:
) for questions:thinking:
) or 💭 (:thought_balloon:
) for more open inquiry that's not quite a confirmedissue and could potentially benefit from discussion
:art:
) for suggestions / improvements:x:
) or:warning:
) for more significant problems or concerns needing attention:seedling:
) or ♻️ (:recycle:
) for future improvements or indications of technical debt:pick:
) for minor or nitpick changes