added functionaly of an store subscriber

[ejkkan]

I have an example of feature i would like to add.
A cross store subscriber functionality would be really handy when one wants to execute certain logic depending on state changes.

For example, in my app i want an AsyncStorage module to store certain values on update, a hook like this certainly makes the it user friendly.

Please come with feedback and improvement suggestions!

[jhonnymichel]

hey @ejkkan thank you so much for you contribution, I'm looking forward to merge it, but I need you to teste your feature. Let's try and keep coverage at 100%!

[jhonnymichel]

We need tests here ;)

[ejkkan]

Sorry, PRs and testing are new to me.
Of course we should keep the test suit at 100%!

Would you deem the feature fine with this implementation or do you have more feedback on it?

[jhonnymichel]

@ejkkan It's starting to look good with tests now! Thank you for your contribution, I really appreciate it.

I liked that you are only listening to specific actions, good idea, but I have one extra feedback that I would like you to review:

the way your code is working makes it possible for only one subscription at a time.

My suggestion would be to have only one new variable (let's say, 'subscriptions') of type object.

Each key of the object would be the name of an action, and the value would be an array off callbacks.

Also, the subscribe function could return an unsubscribe callback to be used later by the subscriber, once it does not need to listen to changes anymore.

const subscriptions ={};

function subscribe(action, callback) {
  if (!subscriptions[action]) {
    subscriptions[action] = [];
  }
  subscriptions[action].push(callback);
  return () => {
     subscriptions[action] = subscriptions[action].filter(c => c !== callback);
  }
}

What do you think?

[ejkkan]

Awesome feedback!

After some tinkering I found that the when adding some more functionality, the implementation was not as userfriendly as i had hoped.

I remade it so that the api now resembles the "createStore". Not only does it follow the structure already used by this lib, by passing an identifier to the subscriber it will also give some flexibility as for how to unsubscribe to it. What do you think?

[jhonnymichel]

That ID part does makes sense because you wanted it to look more like the createStore API, which is nice, but I believe the way to go here would be to subscribe be a part of the StoreInterface API, so no need for an ID IMO.

This will be my last change request, I promise 🙈

All the rest is looking very good! Looking forward to use store subscribers when using the library!

[ejkkan]

No worries, just want to make this right.

I'm not sure as to what you mean by making it a part of the StoreInterface.
Do you mean that each created store should carry its own subscribe function?
And that you would have to call the subscribe function from the created store?

My case for using the subscribe would be to have one centralized point where i could subscribe to all the created stores. So instead of having to make each stores subscribe function call a specific module, i could just subscribe to all of them at one spot. Or is this the wrong way to go?

Thanks for being patient with me!

[jhonnymichel]

Got it. I actually have to think about it. You're making sense, I'm just not sure what would fit this library pattern best. I'm leaning towards StoreInterface holding this responsibility. Because every store is a self-enclosed entity. So far there is no interaction between them (or global ones). I think using subscribe inside stores would fit best within the current design of the library. Later we could rethink this but it would require more changes. How are you using the library? This is important to know, because how users make use of it is what really matters, so I can for sure accept this new design dogma (interacting with all stores at once) if users want it and if they can provide good use cases for it

…

________________________________ De: Erik Magnusson <notifications@github.com> Enviado: domingo, 17 de março de 2019 08:57 Para: jhonnymichel/react-hookstore Cc: Jhonny Michel; Comment Assunto: Re: [jhonnymichel/react-hookstore] added functionaly of an store subscriber (#15) No worries, just want to make this right. I'm not sure as to what you mean by making it a part of the StoreInterface. Do you mean that each created store should carry its own subscribe function? And that you would have to call the subscribe function from the created store? My case for using the subscribe would be to have one centralized point where i could subscribe to all the created stores. So instead of having to make each stores subscribe function call a specific module, i could just subscribe to all of them at one spot. Or is this the wrong way to go? Thanks for being patient with me! — You are receiving this because you commented. Reply to this email directly, view it on GitHub<#15 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AGrosxnGY7Zs4Qjrs4TgiwVounbcydgpks5vXi26gaJpZM4b3Kqr>.

[ejkkan]

Take the time you need! I totally understand that your thoughts about keeping to the current design flow.

My inspiration for the subscribe actually comes to redux's subscribe function. This subcriber listens for the dispatch function to be called for the whole redux store, containing multiple reducers. However, this listens to all changes and one would have to handle each action-type from there on. This could also be pattern if you think it would suit this lib.

If we were to enforce the current design, the usage of the this library would look something like this:

const store1 = createStore(...);
const store2 = createStore(...);
store1.subscribe(['isLoggedIn'], (action, state) => centralizedActionHandler(action, state));
store2.subscribe(['hasUnlockedAchievement'], (action, state) => centralizedActionHandler(action, state));

const centralizedActionHandler = (action, state) => {
    ... example: code to store locally
}

But, how I had implemented this with redux and what was my thought about this implementation from the start, it would look something like this.

const store1 = createStore(...);
const store2 = createStore(...);
subscribe(['isLoggedIn', 'hasUnlockedAchievement'], (action, state) => centralizedActionHandler(action, state));

const centralizedActionHandler = (action, state) => {
    ... example: code to store locally
}

Hope these examples helps you to understand what I was going for. But either implementation would be fine!
Looking forward to your decision!

[jhonnymichel]

@ejkkan How would the state argument that your centralizedActionHandler receives look like? You are subscribing to receive state changes from different stores at once... but the code that actually handles the subscription callbacks can only know about its own state (its inside the store.setState method), and currently there is no way to do that globally.

So, I think to make your idea work it would require a quite significant refactor in the library code, what you want works great for redux because it doesnt' matter how much you partition your state by using combineReducers., in the end, it is really only a single big store object. It is not our case, so it wouldnt work the way you plan out of the box.

Because of that, and because of what I expressed previously, I want to commit to the idea of each store having its own subscribe method. If the user ends up wanting something closer to the redux implementation, I would recommend they to use only one store for now.

I hope it makes sense to you as well.

Once the code is ready, let me know so I can merge it.

This is the biggest contribution from the community so far and I am very pleased by that, once again, thank you so much, I'll make sure you're listed inside a future contributors section on the repository.

It was very nice discussing this implementation with you!

[ejkkan]

Totally understand your decision. It does makes sense to me!

Had a lot of things to do this week so haven't had time to look in to this until now!

Please let me know if the current implementation is ok!

[jhonnymichel]

Looks good, thanks @ejkkan I'll merge it now