Skip to content
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

Have a way to inject listeners into derived Store implementation #51

Closed
wainwrightmark opened this issue Mar 9, 2023 · 5 comments · Fixed by #52
Closed

Have a way to inject listeners into derived Store implementation #51

wainwrightmark opened this issue Mar 9, 2023 · 5 comments · Fixed by #52

Comments

@wainwrightmark
Copy link
Contributor

Hi. I love this crate - I use it in all my yew projects.

At the moment it's a bit of a pain to use listeners because I am almost always using the Derive attribute to implement Store and persist to local storage. If I want to add a listener to a Store with zero listeners I need to expand the macro and add the init_listener statements. This is a bit ugly and potentially error prone. It would be nice to have an attribute to initialize extra listeners in the generated code.

For example you could write something like:

use yewdux::{store::Store, prelude::Listener};
pub struct ExampleListener;

impl Listener for ExampleListener{
    type Store = ExampleStore;

    fn on_change(&mut self, state: std::rc::Rc<Self::Store>) {
        log::info!("Changed")
    }
}

#[derive(Default, PartialEq, Store)]
#[store(storage = "local", storage_tab_sync )]
#[store_listener(ExampleListener)]
pub struct ExampleStore{
    prop: bool
}

Instead of:

use yewdux::{store::Store, prelude::Listener};
use yewdux::prelude::init_listener;
pub struct ExampleListener;

impl Listener for ExampleListener{
    type Store = ExampleStore;

    fn on_change(&mut self, state: std::rc::Rc<Self::Store>) {
        log::info!("Changed")
    }
}

#[derive(Default, PartialEq)]
pub struct ExampleStore{
    prop: bool
}

impl Store for ExampleStore {
    #[cfg(not(target_arch = "wasm32"))]
    fn new() -> Self {
        init_listener(ExampleListener);
        Default::default()
    }

    #[cfg(target_arch = "wasm32")]

    fn new() -> Self {
        init_listener(StorageListener);
        init_listener(ExampleListener);

        storage::load(storage::Area::Local)
            .ok()
            .flatten()
            .unwrap_or_default()
    }

    fn should_notify(&self, other: &Self) -> bool {
        self != other
    }
}

struct StorageListener;
impl Listener for StorageListener {
    type Store = ExampleState;

    fn on_change(&mut self, state: Rc<Self::Store>) {
        #[cfg(target_arch = "wasm32")]
        save(state.as_ref(), Area::Local).expect("unable to save state");
    }
}

Happy to have a go and do a PR if you think this is a good idea.

@intendednull
Copy link
Owner

Great idea! Feel free to open a PR

@wainwrightmark
Copy link
Contributor Author

Am working on implementing this. However I've run into a bit of a problem - the code I posted above doesn't work - it seems like you can only have one listener for each Store - in the book it says

NOTE: Successive calls to init_listener on the same type will replace the existing listener with the new one.

but I took that to mean that you can't register the same listener twice - not that you can only have one type of listener for each Store. Is that intended? It seems like it would be nice to have more than one (that's the whole point of this issue :) )

If it's not intended or you want it changed I can have a go but that's a slightly bigger job and might be a breaking change...

@intendednull
Copy link
Owner

Are you getting an error? You should definitely be able to have multiple types of listeners for one store

@wainwrightmark
Copy link
Contributor Author

No error. It's just the first listener is dropped when the second listener is registered.
Try out this example: https://github.com/wainwrightmark/yewdux/tree/multiple-listeners/examples/multiple_listeners

I believe it should be relatively easy to fix - just make ListenerStore generic over the type of listener rather than the type of store.

@intendednull
Copy link
Owner

That's definitely a bug! Yes you're correct, that would fix it. Feel free to include it in your PR, and maybe a unit test too?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants