reflux / refluxjs

A simple library for uni-directional dataflow application architecture with React extensions inspired by Flux
BSD 3-Clause "New" or "Revised" License
5.36k stars 330 forks source link

ADD: Ability to use ES6 listenables with an Array of Actions #487

Closed eek closed 7 years ago

eek commented 7 years ago

I added the ability to use this.listenables with an array of Actions, just like this.stores with has with an Array of stores.

What do you guys think?

FlorianWendelborn commented 7 years ago

Might be good to add an optional prefix, so actions could both be used, even if both actions contain save or something else that's generic.

eek commented 7 years ago

And both actions to trigger the same function from the store? That sounds like a good idea 😀

FlorianWendelborn commented 7 years ago

@eek no. Exactly the opposite. So you can prevent it with a prefix and onASave gets mapped to A's save, while onBSave gets mapped to B's save.

eek commented 7 years ago

I don't think that will be added value, in my case specifically, I decided I needed to include some GlobalActions to the UserActions, both with their stores, so that I can trigger a logout from a GlobalAction. I think it will be simpler to keep the same structure for triggering actions (onAction or directly action) in the store, in the case that 2 actions conflict by name, they could easily be renamed, we can show a console warn in case this happens instead of renaming everything with onASave or onBSave, then the question would be, if they don't conflict should we rename? I think the answer should be no, but in either cases, it will not be consistent with the rest of the stores that listen just to a list of actions. I think conflicting actions in the same store will happen rarely, and it's easier to change the action than change the functions.

What do you think? Also, any other suggestions? @spoike @BryanGrezeszak ?

FlorianWendelborn commented 7 years ago

I am talking about an optional prefix. A setting which allows users to choose a prefix.

eek commented 7 years ago

I'd like keep things simple, and similar to how it was in the previous versions:

var Store = Reflux.createStore({
    listenables: [require('./darkspells'),require('./lightspells'),{healthChange:require('./healthstore')}],
    // rest redacted
});

And to be honest, I never had the problem of having the same actionName twice in the previous versions.

If you check this answer by @spoike - https://github.com/reflux/refluxjs/issues/271#issuecomment-76517210

The only time naming will matter (to my knowledge) is if you put two actions objects that have same name on actions into the listenables. It won't throw an error, instead the last listenable's action of the same name will be used to bind with the appropriate store method IIRC.

So, in my pull request it's exactly the same functionality, but for the ES6 Stores.

We can discuss to extend it, but I can't imagine a real-world scenario where the prefix would be helpful :(

FlorianWendelborn commented 7 years ago

@eek Isn't that exactly what I said? Doesn't that just work like an optional prefix? It surely looks exactly like that (I don't know if it is, since that part seems to be poorly documented).

In addition to that - how can you not imagine that 2 sets of actions may contain one with the same name? How is someDataNamespace.save() or someDataNamespace.getInitialStateFromServer() not something that's done a lot?

eek commented 7 years ago

I think there's a miscommunication here, currently, the implementation I made has exactly the same functionality Reflux had with mixins until now. Allowing your store to listen to an array of actions.

Now, let me understand. You're saying that you have:

var someDataNamespace = Reflux.createActions([
        "save",    
        "getInitialStateFromServer",
        "showPotatoToUser"
    ]);

and

var anotherDataNamespace = Reflux.createActions([
        "save",     
        "getInitialStateFromServer",
        "eatTomato"
    ]);

and you want to listen to someDataNamespace and anotherDataNamespace:

class MyStore extends Reflux.Store
{
    constructor() {
        this.listenables = [anotherDataNamespace, anotherDataNamespace];
    }

   onSave = () => {
      // save something
      // this onSave will work only triggered by anotherDataNamespace.save()
      // but there's no problem in triggering someDataNamespace.showPotatoToUser()
      // and anotherDataNamespace.eatTomato()  - both would work.
   }
}

and have 2 different onSave functions in that store that both work with a prefix? like onASave and onBSave ?

Until now, when you added [someDataNamespace, anotherDataNamespace] to listenables in the Reflux Store that used mixins, only the second save could have been used. anotherDataNamespace.save() would work but someDataNamespace.save() would not work according to the previous comment (by spoike) that I posted.

No one had a problem with this (one in which to open a issues), they can easily change the save actions to savePotato and saveTomato, instead of adding a prefix in for the store functions to understand both.

FlorianWendelborn commented 7 years ago

@eek, yes that's exactly what I mean. I know it's easy to workaround this, however - this would basically force actions to be a lot more verbose than they have to be (and lead to something similar to Hungarian notation).

In your example above {healthChange:require('./healthstore')} wouldn't healthChange act as a prefix for the actions? I thought that was your point. If that acts as a prefix, then I'm fine with having an option like that. 🙂

BryanGrezeszak commented 7 years ago

I haven't really thought too deeply about it, but at first glance I like it. Seems like it could be useful and doesn't add any complexity to the API; extremely intuitive and clean to use and extremely readable exactly what's going on with minimal library knowledge (which is how it should be!).

As for the prefixing for conflicts idea, I won't weigh in much on that because personally I simply like to solve such issues via architecture rather than additional features. You guys can fight that out :P I just suggest that whatever additions you make to this concept: don't make the more simple/normal usage any harder in the process.

Glad to see more people getting involved here too. This is almost starting to look like a project again!

eek commented 7 years ago

I think this PR is complete for the moment, it adds the same functionality that Reflux had before, I wouldn't go into too much details and make it harder to use for now. If someone needs something specifically, we can discuss via a new Issue.

I'd love to see this project going on again, and I'll do my best to stay involved and help as much as possible 😀