[yew-devel] [RFC PATCH] store: use try_borrow and expect_throw for better error messages

[Shannon Sterz]

On Tue May 6, 2025 at 10:49 AM CEST, Dominik Csapak wrote:
> high level question/comment:
>
> AFAIU you want to add proper error messages to the borrows here,
> so wouldn't it be enough to use 'try_borrow().expect("some error")' ?
>
> or does the UnrwapThrowExt trait something additional that we want/need which
> I'm overlooking?

The `UnrwapThrowExt` is generally preferable for WASM usage according to
the documentation:

> These methods should have a smaller code size footprint than the
> normal `Option::unwrap` and `Option::expect` methods, but they are
> specific to working with Wasm and JS.

Hence, why I use them here instead of the ordinary `unwrap` and `expect`
functions.

>
> E.g. in PDM we have a global panic hook that displays the panic (and i think stack trace)
>
> and the default panic hook logs it into the console
> (which also happens for uncaught exceptions)

Yes and that is good practice, but adding these here instead of just
panicking without any information is probably still nicer in other usage
scenarios. So I don't really see why we shouldn't do this also.

IMO, ideally we could give components a way to try acquiring the locks
and let them handle the failure in some way. Panicking in WASM leads to
the entire GUI freezing until it is realoaded, there may be scenarios
where components can recover from not being able to write or read a
store without doing so.

One scenario I can imagine is a component that uses the information in
the Store for front-end validation. If it can't acquire a read lock on
the store, it could fail silently and rely on the backend taking care of
the validation. While this is somewhat inconvenient for users, it's
probably preferable over having to reload everything (and potentially
losing multiple inputs).

If we don't want to break the public API here, I'd suggest adding
something like `try_read()` and `try_write()`.

> On 5/5/25 15:28, Shannon Sterz wrote:
>> this makes use of the `UnwrapThrowExt` trait to improve error messages
>> when acquring store locks. taking such locks in async functions can
>> sometimes lead to bugs that can be unreliably reproduced, this should
>> provide more information about the source of the error compared to
>> just panicking.
>>
>> Signed-off-by: Shannon Sterz <s.sterz at proxmox.com>
>> ---
>>
>> alternatively we could return the error here directly (or wrap it) and
>> let the call site handle such errors. however, that would require
>> breaking this public api, so i decided against this for now.
>>
>>   src/state/store.rs | 13 +++++++++++--
>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/state/store.rs b/src/state/store.rs
>> index bde7d9da..b071182c 100644
>> --- a/src/state/store.rs
>> +++ b/src/state/store.rs
>> @@ -10,6 +10,8 @@ use yew::html::IntoEventCallback;
>>   use yew::prelude::*;
>>   use yew::virtual_dom::Key;
>>
>> +use wasm_bindgen::UnwrapThrowExt;
>> +
>>   use crate::props::{
>>       ExtractKeyFn, ExtractPrimaryKey, FilterFn, IntoFilterFn, IntoSorterFn, SorterFn,
>>   };
>> @@ -130,7 +132,10 @@ impl<T: 'static> Store<T> {
>>       /// Panics if the value is currently mutably locked.
>>       pub fn read(&self) -> StoreReadGuard<T> {
>>           StoreReadGuard {
>> -            state: self.inner.borrow(),
>> +            state: self
>> +                .inner
>> +                .try_borrow()
>> +                .expect_throw("Could not acquire read lock on store!"),
>>           }
>>       }
>>
>> @@ -142,7 +147,11 @@ impl<T: 'static> Store<T> {
>>       /// When the returned [StoreWriteGuard] is dropped, the store listeners
>>       /// are notified. To prevent that use [StoreWriteGuard::skip_update]
>>       pub fn write(&self) -> StoreWriteGuard<T> {
>> -        let state = self.inner.borrow_mut();
>> +        let state = self
>> +            .inner
>> +            .try_borrow_mut()
>> +            .expect_throw("Could not acquire write lock on store!");
>> +
>>           StoreWriteGuard {
>>               state,
>>               update: true,
>> --
>> 2.39.5
>>
>>
>>
>> _______________________________________________
>> yew-devel mailing list
>> yew-devel at lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/yew-devel
>>
>>
>
>
>
> _______________________________________________
> yew-devel mailing list
> yew-devel at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/yew-devel