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

Reflux.Store aggregation fails using listenTo(otherStore) #480

Closed semikolon closed 7 years ago

semikolon commented 8 years ago

The ES6 Reflux.Store is apparently missing a listen() method, which is used by listenTo()

I tried: this.listenTo(IdeaStore, this.ideasUpdated)

and got:

main.js:61874 Uncaught Error: function IdeaStore() {
    _classCallCheck(this, IdeaStore);

    var _this = _possibleConstructorReturn(this, Object.getPrototypeOf(IdeaStore).call(this));

(...)

  } is missing a listen method
BryanGrezeszak commented 8 years ago

@semikolon - I'll look at it later today. Reflux.Store just loops through and copies all methods/properties from the store object it wraps, so it should have it. But we'll see what turns up as I check it out.

BryanGrezeszak commented 8 years ago

@semikolon - Actually, I think I might know the issue. I'm guessing IdeaStore is the class name, not actually an instance of the class. Is that right?

You assign the class itself to this.store because it then automatically handles instantiating it and making a proper singleton (if one isn't already made), registering it with the global state (if it has an id), etc. But that doesn't mean the store class itself is what you work with, an instance still needs to exist. That instance is what has all the methods/properties just like an instance create via Reflux.createStore would.

If you've already got this store attached and mounted to a component then there will be a IdeaStore.singleton static property on the class you can access that will be the singleton instance of the class.

An API for utilizing ES6 stores without them being attached to a component (including them working with the global state) is actually already done and waiting to push. But I believe @devinivy wants to make a stable release of the API we have already first, so I was waiting to push it. @devinivy - what're your thoughts on that?

devinivy commented 8 years ago

Yeah– unless you think a breaking change will be necessary, my vote is to make a stable release with the current API.

BryanGrezeszak commented 8 years ago

@devinivy - I'd say an update to the docs might be warranted to make more clear that singleton instances are the actual stores themselves, not the class. But other than that I agree. I'll push that doc update shortly.

We should probably also wait to hear back from semikolon to be sure it really is a class vs instance issue and not a true bug though. I can't actually be 100% sure without his code or word from him about it.

@semikolon - let us know. Also, just wanted to say thanks for taking the time to make these issues. It's a huge help having people give their real-world experience with it and contribute with issues they find.

semikolon commented 8 years ago

Thanks! Just great that you guys are taking care of this library, so we can all keep using it without friction. I have to admit I did figure out that it was because of the class/instance reference, I tried referring to IdeaStore.store, but that didn't do the trick. I should probably have mentioned this in my bug report. I'll try .singleton when I'm back at my computer in an hour On Wed, 10 Aug 2016 at 17:06, Bryan Grezeszak notifications@github.com wrote:

I'd say an update to the docs might be warranted to make more clear that singleton instances are the actual stores themselves, not the class. But other than that I agree. I'll push that doc update shortly.

We should probably also wait to hear back from semikolon to be sure it really is a class vs instance issue and not a true bug though. I can't actually be 100% sure without his code or word from him about it.

@semikolon https://github.com/semikolon - let us know. Also, just wanted to say thanks for taking the time to make these issues. It's a huge help having people give their real-world experience with it and contribute with issues they find.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/reflux/refluxjs/issues/480#issuecomment-238896523, or mute the thread https://github.com/notifications/unsubscribe-auth/AAA2ME25MOQ_QBoaYUW4K-QKyxtpq72nks5qeejlgaJpZM4Jg7ao .

BryanGrezeszak commented 8 years ago

@semikolon - The .singleton will be available any point after the component's .componentWillMount point in its lifecycle, since that's when Reflux does it's magic mounting the store to the component.

However, if you need it to be available before then you can always do things manually. If you don't need this store to tie into the global state then it's really easy. Instead of assigning the class itself to this.store you can make the singleton yourself and assign it:

IdeaStore.singleton = new IdeaStore();
this.store = IdeaStore.singleton;

If you want it to tie into the global state (and it has an id and such) then it's a bit more complex to do manually:

IdeaStore.singleton = new IdeaStore();
IdeaStore.singleton.id = IdeaStore.id;
if (Reflux.GlobalState[IdeaStore.id])
    IdeaStore.singleton.state = Reflux.GlobalState[IdeaStore.id];
else
    Reflux.GlobalState[IdeaStore.id] = IdeaStore.singleton.state;
this.store = IdeaStore.singleton;

The latter, of course, is kinda unacceptable to ask people to do just to make their store. So a simple function for initializing a globally tracked store manually is part of the API I'll add later should devinivy accept it. But I know you're using this now, so there's some solutions should you need them.

BryanGrezeszak commented 8 years ago

@devinivy - so I guess that means we're good on this bug report, and documentation clarifying this has been added to the pull request, so I think that pull request is good to go.

semikolon commented 8 years ago

Thanks for spelling this all out for me, it's appreciated. And sorry I'm so slow at replying sometimes.

The reason I wasn't super-motivated/stress about getting the .listen() to work is I had to find another solution and what I did was just that I listened to the actions instead, and used the this.stores feature to aggregate data to the component state.

I have two React components subscribed to multiple stores: IdeaList -> [IdeaStore, UserStore] and PraiseList -> [PraiseStore, UserStore]. There's also a UserList -> UserStore.

When the components mount, they trigger (well actually an enclosing HOC triggers) a getAll action for the primary stores [IdeaStore, PraiseStore], and the response data also includes all relevant users for the [idea|praise] objects fetched, so I merge that into the UserStore, and it shows up in the components.

IdeaActions.getAll.completed.listen(data => UserActions.setPartial(data))
PraiseActions.getAll.completed.listen(data => UserActions.setPartial(data))

export class UserStore extends Reflux.Store {
  static id = 'users'

  constructor() {
    super()
    this.state = {
      loading: true,
      users: []
    }
    this.listenToMany(UserActions)
  }

  // SET PARTIAL
  setPartial(data) {
    const newUsers = data.users
    if (!newUsers || newUsers.length === 0) { return false }
    const updatedUsers = this.mergeListsById(this.state.users, newUsers)
    this.setState({ users: updatedUsers })
  }

  mergeListsById(oldArr, newArr) {
    var hash = new Map()
    oldArr.concat(newArr).forEach(function(obj) {
      hash.set(obj.id, Object.assign(hash.get(obj.id) || {}, obj))
    })
    return Array.from(hash.values())
  }

...
semikolon commented 8 years ago

But it may be a bit cleaner to just do this.listen(), and if that works, it'd listen to all changes, not just the getAll.completed stuff. Like, if an idea could get likes/upvotes from other users, you could keep track of the count automatically, or something

Though I was thinking of possibly later using GlobalState for caching all of the store data between browser sessions, and the manual maintenance you mention if I want both listen() and GlobalState to work would be a downer, so a simple manual "on-switch" for a store, like you said, would be sweet...

BryanGrezeszak commented 8 years ago

Yeah, that's pretty much where this.stores = [StoreOne, StoreTwo] works out great. It basically aggregates stores on a component-by-component basis as needed.

The aggregation happens on the component level each component can choose their own set of stores to aggregate without it affecting any other component that may be using those stores. That allows the most efficient usage of stores possible, with the minimum re-rendering of components possible, while also being as modular as possible since the store level of the architecture need not even be aware of what the component level is doing with its aggregation.

So that's the intended pattern to use for aggregation, with you as the developer deciding how to break up stores so that you can assign/aggregate them within components without unnecessary re-renders.


However, it's still nice to have options outside the intended pattern. So I'll still later be adding that ability to easily create the store singletons outside of component so they can be manipulated however you need.

And note: that snippet of code I give for making the singleton store manually outside of a component that works with global state is a 1 time thing. You can literally just put it right at the end of your class, right after it. From there on the IdeaStore.singleton is properly created and you can attach it (the instance itself or the class itself) to any component and it'll use the proper singleton. The only point of that code would be to make .singleton available sooner instead of after the first mounting of a component that uses it. Other than that nothing else changes.

The reason I find that snippet unacceptable is not that it would require any sort of maintenance or repeated use, but simply because I would never require someone to learn such a verbose and possibly confusing method to initialize something when a simple function that is part of the API should be all they need to learn to do it.

BryanGrezeszak commented 7 years ago

Solved in newest release via addition of/documentation for Reflux.initializeGlobalStore and better docs about the singletons of a Reflux.Store

songguangyu commented 6 years ago

I hava same bug, in reflux 6.4.1 reflux-core 1.0.0。 can provide a demo?