Document Emitter trait method panics
#12444
Conversation
There was a problem hiding this comment.
Thank you, this LGTM, could you add a change file in .changes directory?
This PR makes all methods on the
Emittertrait default methods and saves boilerplate
This was done intentionally to customize the documentation example based on the used type, but I guess it is fine to remove.
Package Changes Through 2d63772There are 4 changes which include tauri-bundler with patch, tauri-cli with patch, tauri with patch, @tauri-apps/cli with patch Planned Package VersionsThe following package releases are the planned based on the context of changes in this pull request.
Add another change file through the GitHub UI by following this link. Read about change files or the docs at github.com/jbolda/covector |
|
Should we even panic here? It may be good for development for most cases but what if devs use dynamic event names that are derived from some http request for example? idk panicking in a gui library just always sounds awaful to me |
|
yeah maybe returning an Error here is better, the methods already return Result type |
|
I've added some changes that inch closer in that direction, replacing the assert with a checked While |
| /// # Panics | ||
| /// Will panic if `event` contains characters other than alphanumeric, `-`, `/`, `:` and `_` | ||
| fn emit<S: Serialize + Clone>(&self, event: &str, payload: S) -> Result<()> { | ||
| let event = EventName::new(event).unwrap(); |
There was a problem hiding this comment.
instead of panicking here, just propagate the result from EventName::new
| } | ||
|
|
||
| #[derive(Clone, Debug, PartialEq, Eq, Hash)] | ||
| pub struct EventName<S = String>(S); |
There was a problem hiding this comment.
Let's move this type to tauri/src/event/mod.rs
| #[derive(Clone)] | ||
| pub(crate) struct InvalidEventName; |
There was a problem hiding this comment.
This could be just a variant on tauri::Error type
| } | ||
| pub(crate) fn into_inner(self) -> S { | ||
| self.0 | ||
| } | ||
| pub(crate) fn as_str_event(&self) -> EventName<&str> { | ||
| EventName(self.0.as_ref()) | ||
| } |
There was a problem hiding this comment.
| } | |
| pub(crate) fn into_inner(self) -> S { | |
| self.0 | |
| } | |
| pub(crate) fn as_str_event(&self) -> EventName<&str> { | |
| EventName(self.0.as_ref()) | |
| } | |
| } | |
| pub(crate) fn into_inner(self) -> S { | |
| self.0 | |
| } | |
| pub(crate) fn as_str_event(&self) -> EventName<&str> { | |
| EventName(self.0.as_ref()) | |
| } |
| pub fn emit<S: Serialize + Clone>(&self, event: &str, payload: S) -> crate::Result<()> { | ||
| assert_event_name_is_valid(event); | ||
| let event = EventName::new(event).unwrap(); |
There was a problem hiding this comment.
this seems unnecessary since Emitter::emit does the same check
| pub fn emit_filter<S, F>(&self, event: &str, payload: S, filter: F) -> crate::Result<()> | ||
| where | ||
| S: Serialize + Clone, | ||
| F: Fn(&EventTarget) -> bool, | ||
| { | ||
| assert_event_name_is_valid(event); | ||
| let event = EventName::new(event).unwrap(); |
| @@ -680,14 +680,14 @@ fn on_webview_event<R: Runtime>(webview: &Webview<R>, event: &WebviewEvent) -> c | |||
| paths: Some(paths), | |||
| position, | |||
| }; | |||
| webview.emit_to_webview(DRAG_ENTER_EVENT, payload)? | |||
| webview.emit_to_webview(&DRAG_ENTER_EVENT, payload)? | |||
There was a problem hiding this comment.
isn't DRAG_ENTER_EVENT a copy type? can't we remove the &?
| @@ -124,20 +111,20 @@ impl FromStr for EventTarget { | |||
| #[derive(Clone)] | |||
| pub struct EmitArgs { | |||
| /// Raw event name. | |||
| pub event_name: String, | |||
| pub event_name: EventName<String>, | |||
There was a problem hiding this comment.
let's remove this field and use the event field, we don't need both, see #12460 (comment)
Almost all methods of the
ListenerandEmittertraits take an event name and will panic if that name does not satisfy some preconditions. This is not documented and this PR fixes this. This PR also moves the panic locations closer to the user-facing code, making it immediately clear what has gone wrong and what methods were responsible for the panic. Prior to this change, the panic pointed to a location of little use to a user oftauriunless he reverse engineers how that code was called withintauri.As a drive-by improvement, it was a bit unclear that the
Emittertrait is purely defined based on methods on theAppManager. This PR makes all methods on theEmittertrait default methods and saves boilerplate. This change should not have any downsides becauseEmitteris a sealed trait that can only be implemented insidetauri.