Stateless dispatcher

[acdlite]

This PR mainly does two things:

1. Makes the dispatcher stateless. Side-effect-y, so not quite "pure", but with no local state. Instead, it is passed setState() by the dispatcher.
2. createRedux() accepts prepareState() as the third parameter*, which is used to prepare the atom before being sent to subscribers. Akin to a global selectState(), but with a different name to avoid confusion (e.g. prepareState() does not have to return a plain object).

*I don't like this as a final API, but we should discuss this more before introducing a breaking change to createRedux().

These two changes allow custom dispatchers to store "meta" state on the state atom without exposing it to consumers. #113

It also changes the signature of the dispatcher (by which I mean the higher-order function that creates dispatchFn() — we really need to work on / document some of this terminology) from dispatcher(initialState, setState) to dispatcher({ getState, setState }). It does not include dispatch as suggested here, since that can be implemented inside the dispatcher instead.

Speaking of which, the createDispatcher() API has been changed slightly to I'll submit a separate PR for this, since it's not really related.

createDispatcher({
  stores,
  middleware: ({ setState, getState, dispatch }) => middlewares
});

Thoughts? I will update with better createRedux() API once we come to a consensus.

[johanneslumpe]

Just curious: any reason to prefer destructuring of a single argument vs 2 separate arguments? It does introduce the overhead of creating a new object each time. Yes, adding new arguments is less likely to break things, but is that the only motivation here?

[acdlite]

Yep, it's entirely to avoid an API change. And it only gets called once, not on each dispatch. (But even if it did, the performance impact would be infinitesimal compared to everything else that goes on during a dispatch cycle.)

[johanneslumpe]

@acdlite yeah I'm sure you're right about that. Just trying to play the devil's advocate a bit. I think the more we question general, the better or more "prove" the final solution will be :)

[acdlite]

@johanneslumpe Of course :)

[johanneslumpe]

We do have to call prepareState + getState on each dispatcher call though now. Still two function calls are probably not a huge problem - only if your prepareState function would be doing something quite involved (but that should never be the case?)

[acdlite]

By default it's just the identity function. I can't think of any use case where it would be very computationally heavy.

The more I think about it, though, I don't like the current implementation in this PR. selectState() feels like it will always be very coupled to the dispatcher, but right now they are two separate parameters. I do like how the dispatcher is a pure function, though. Perhaps the interface for dev tools should wrap around the Redux instance rather than the dispatcher, since the dispatcher is no longer stateful. E.g. something like createDebuggerRedux() instead of createRedux(debugDispatcher()).

[acdlite]

The most important part of this PR for me is figuring out where the state lives. Based on the discussion in #113, I think it makes most sense for the state to always live in a single place, the Redux instance. The dispatcher should have no local state, and use setState() to update Redux.

If we can agree on that, everything else is just a matter of finding the correct API.

[johanneslumpe]

I most certainly agree with that sir! After all, we are striving to only have a single source of truth. The less scattered the state is - even though it might just be references to the same object - the better.

Regarding the debugger api: the example in #113 does have the need to wrap the dispatcher directly though. And in order to log actions properly there does not seem to be a different way than hooking into it directly?

[acdlite]

Yes, it should hook into it directly, but my concern is what the top-level API looks like for users. Right now it's createRedux(dispatcher, initialState, prepareState). So if you're using custom dispatcher that is storing meta state, you have to also remember to pass a corresponding prepareState that knows how to filter out any meta state. Ideally you'd only need to configure one thing, not two.

[acdlite]

Anyway, gotta go to bed, but I'll come back to this tomorrow.

[emmenko]

we really need to work on / document some of this terminology

Agree

it makes most sense for the state to always live in a single place, the Redux instance

It does make sense yes. The Redux instance is also not directly exposed and if you want to access / change something you have to use the API. So passing around those API methods (e.g.: setState) would be correct I guess.

[gaearon]

Regarding the debugger api: the example in #113 does have the need to wrap the dispatcher directly though. And in order to log actions properly there does not seem to be a different way than hooking into it directly?

In fact in that example I'm using the default dispatcher. I'm not wrapping it.

Instead, I'm wrapping the store. And my wrapper puts all actions (and intermediate states) into the main state into a “hidden” field.

So there's no need for a custom dispatcher (or wrapping the dispatcher) to implement time travel I think. (I might be wrong though :-). It's enough to store the history inside the main state, and to subscribe to that history from a React component.

[gaearon]

I'm going to take a look at this in a few days. Thank you! Very busy atm :-/

[acdlite]

As mentioned in here, this PR also fixes the nested dispatch bug from #110.

[gaearon]

Please correct me if I'm wrong:

My only concern with this is the addition of prepareState. At this point, to get #113 working, I need both a custom dispatcher and a custom prepareState function. Whereas I want #113 to be pluggable just by wrapping the user's composed Store, just like it works now. I don't think there's any sensible prepareState other than x => x.atom (or whatever we decide to call it).

[acdlite]

@gaearon I think we'd need to API to look something like this:

// Normal Redux
createRedux(stores);

// Recorder Redux
createRecorderRedux(stores);

With an API like createRedux(recorder(store)) there's no way to keep the state in one place, which leads to bugs like the one in #110.

[gaearon]

Hmm, I'm a bit confused why. Can you please elaborate? How is recorder thing fundamentally different from the normal store composition?

[acdlite]

I guess I phrased that poorly.

If you're storing meta information on the state atom, and the state lives entirely on the Redux instance, then prepareState() must also be accessible by the Redux instance so it can strip the meta information before sending to the consumer. Does that make more sense?

[acdlite]

In other words, a single function cannot express both prepareState() and dispatch(), so with an API like createRedux(recorder(store)) we'd need to change the contract of the dispatcher.

[gaearon]

Is it a bad idea to have a convention that the application's state lives inside a field called atom inside the state object? (It probably is.)

[gaearon]

Here's something else. I want to be able to subscribe to the hidden state updates from the monitoring components:

@connect(state => ({
  entries: state[StateKeys.LOG]
}))
export default class ReduxMonitor {

Am I asking for too much at this point?

[acdlite]

@gaearon Haha, no that's a very reasonable thing to want to do. Maybe...

1. Application state goes in a special sub-key like atom or something, like you suggested above
2. Redux has both getState() and getRawState(), where getState() only returns the applications state, and getRawState() returns the entire state object, including meta state.
3. It's up to devtools to implement their own Connect-like subscribers that call getRawState() instead of getState(). Or we could add a prop to Connect called raw (not sure about that name, but whatever) that toggles whether to use getState() or getRawState().

[acdlite]

I like parts 2) and 3) but I'm still in favor of using a prepareState() rather than enforcing a specific sub key, since it's more flexible, and the default can just be the identity function.

[acdlite]

@gaearon Here's my proposal:

// Create redux only accepts a hash of stores, not a dispatcher
createRedux(stores)

// For advanced usage, like dispatchers and middleware, expose raw Redux class
// I know, "ew, gross, classes"
// IMO it's preferable to overloading `createRedux()`
class Redux({ dispatcher, initialState, prepareState })

// Extensions, like dev tools, should provide factories a la `createRedux()`
// that wrap around the Redux class. E.g.
createRecorderRedux(stores)

[taylorhakes]

@acdlite I see where you are going, but I think the way @gaearon implemented the recorder (wrapping the store) is much simpler and intuitive. The only issue is exposing meta state in getState, which doesn't seem much of a problem to me. It makes it very accessible to the default connector and I don't see it causing much confusion.

Adding more API weight with prepareState, Redux class, getRawState, createRecorderRedux, etc. just to create a hidden recorder, seems like a bad trade off. Maybe you have other uses for it? I haven't found any need to manipulate/hide the main state like prepareState allows.

[acdlite]

@taylorhakes The main point of this PR is to make the dispatcher stateless so we can prevent bugs like this one: #110. I'm totally fine dropping the prepareState() and getRawState() stuff, but moving the state into a single location is important.

[taylorhakes]

@acdlite Definitely. Didn't mean to imply anything about the stateless dispatcher part. I think that is a great idea.

[acdlite]

@taylorhakes Cool, glad we're on the same page :)

[gaearon]

Separate instances are starting to appeal to me. (Although I'm not sure about how exactly we want to arrange their interaction.)

This also plays well with time travel monitor component wanting to subscribe to its own "state". If we have separate instances, it will just work within its own Provider. There will also be no need for something like prepareState.

I'll think about it some more and probably post another API proposal..

[acdlite]

Can we at least move forward with the making the dispatcher stateless? I don't really care about the prepareState part at all.

On Sun, Jun 21, 2015 at 5:34 PM, Dan Abramov [email protected]
wrote:

Separate instances are starting to appeal to me. (Although I'm not sure about how exactly we want to arrange their interaction.)
This also plays well with time travel monitor component wanting to subscribe to its own "state". If we have separate instances, it will just work within its own Provider. There will also be no need for something like prepareState.

I'll think about it some more and probably post another API proposal..

Reply to this email directly or view it on GitHub:
#119 (comment)

[gaearon]

Yeah I understand, I just don't want to change internal API twice in a few days :-). I think I'm ready to draft the new internal API based on #113 proof of concept I have locally (will demo at the conf). Give me a little more time!

[acdlite]

Closing in favor of #166

[gaearon]

Superseded by #195.