jetli / yew-hooks

Hooks for Yew, inspired by streamich/react-use and alibaba/hooks.
https://jetli.github.io/yew-hooks/
Apache License 2.0
169 stars 13 forks source link

UseAsyncOptions::enable_auto() can lead to none of the options set #40

Closed Ekleog closed 8 months ago

Ekleog commented 8 months ago

Hey!

I wanted to try out use_async with UseAsyncOptions::enable_auto. So I wrote code like:

    let db = use_async_with_options(
        [...],
        UseAsyncOptions::enable_auto(),
    );
    if db.loading {
        html! {
            <h1>{ "Loading…" }</h1>
        }
    } else if let Some(err) = &db.error {
        panic!("Unexpected error loading database:\n{err}");
    } else {
        let db: &Rc<basic_api::db::Db> = db.data.as_ref().unwrap();
        [...]
    }

As, intuitively, given the async is triggered ASAP, then at least one of the three options must be true.

However, it looks like there is a one-frame delay, and db.loading is set to false at the first run, along with error and data both being set to None.

I'm pretty new to yew_hooks, but… why is db.loading not set to true from the beginning if running with enable_auto? Is that a missing-feature oversight, a bug, or just something that's out of scope?

jetli commented 8 months ago

The initial state is loading=false, data=None, error=None, this is by design for now.

Ekleog commented 8 months ago

I see, so a PR to change the initial state to be loading = true, data = None, error = None if auto-mount is enabled would be rejected.? (Possibly that'd have needed to go through replacing loading with a function if the false is required for internal purposes)

Well, thank you for yew-hooks anyway! :)

(BTW, I'm also not entirely sure I understand why there's both data and error types, so for the next major version bump I'd also suggest considering just having loading and data fields where if the user wants to return a Result, they'd just have data: Result to match on — or even better, having the returned data be an enum of exactly all the possible states for the future)