State should be passed with the action
kloy opened this issue · 19 comments
Hey glad to see someone finally open sourced this approach. Something I have found to typically be true when using this approach internally is that you need the state with the action. This is useful as you will want to select a piece of state to conditionally decide if enough effect should sink. For example, you may want to check if you already have state for a given api before requesting it from the api again.
A downside to keeping these separate is you either do not have the latest state or action as soon as one updates, so you end up needing to implementing logic to take next. I tried this initially as well and it just ended up introducing unnecessary complexity.
Do you mean passing 'store' so that you can getState() when needed? (a-la redux-observable)
Sometimes you only need one or the other, though, and I think having both separate kind of makes sense. Both thunks and sagas are explicit when it comes to accessing the state.
The readme already shows how to combine the two: https://github.com/cyclejs-community/redux-cycles#drivers
It might look a bit too implicit. Maybe it would be a good idea to provide a utility function, like actionsWithState(sources), that would do the combining for you. Maybe not. @nickbalestra @lmatteis wdyt?
@nickbalestra I would say against passing store in and doing getState inside. It is imperative and it might be possible for the getState() call to occur several actions later after the one in question, so the state obtained this way would be too new for the action.
I think passing getState() could work. I personally am passing { action, state } in each value of my REDUX source. I avoided passing getState() in my implementation under the premise that grabbing state in an imperative manner was not the best "cycle" way to do things (this is just my opinion). By passing getState you can guarantee you have the absolute latest state, which does seem advantageous.
The key difference between why redux-saga separates this out and how cycle works is that a generator is sequentially executing. With a source you have to wait for the next value instead of pulling it on the next line of code.
@goshakkk totally agree on staying away from having an imperative getState(), was asking to get a better understanding of the context.
Personally I'm totally fine with how it's at the moment, as you said you don't need both all the times, and when you need combining them is pretty trivial, but I can see when this can get a bit into the way. Not sure I'll like to provide specific actionsWithState either I'll go with one API (a combined one) or a separate one, but not both, my 5c.
@kloy what sort of API are you thinking about? Sources.REDUX would emit an array such as: [ action, state ] or an object? Also, what if you want to react only to state changes? The state driver is still needed.
EDIT: also we should think about our cycle functions more abstractly - not specifically tied to redux. Concepts of actions and state are pretty abstract. However, combing them together would imply a system (like redux) where each action triggers a state change - which might not necessarily always be the case in other state management systems. Also how would you call such stream? ACTIONSTATE? REDUX?
After thinking about it I guess it does make sense. But I'm still in doubt about the name. Also the API would make it more clunky (.filter(({ action }) => rather than .filter(action =>. Having the state is indeed convenient and, after using redux-observable, it's something we do quite often.
sources.ACTION.filter(({ state }) => state.counter % 2 == 0) seems nice though. Even though semantically speaking you're listening for ACTION changes, not state changes. I'm not sure.
I named the driver REDUX in my current implementation and pass the store to it versus running the cycle app from redux. This allows for anything matching the store contract to be used for state management.
The REDUX source passes an object with action and state props. New values are emitted for every action regardless of state change. This is to work around the actions being leveraged as an event bus at times in our app. Listening for state changes is done using distinct since the references are unique.
How do you create the middleware though?
const reduxCycles = reduxCycles();
const middleware = reduxCycles.createCycleMiddleware();
const store = createStore(
rootReducer,
applyMiddleware(middleware)
);
Cycle.run(main, { REDUX: reduxCycles.makeDriver(store) });So .createDriver sets a local listener which is accessed by the middleware defined earlier?
Apart from merging the ACTION and STATE drivers, I like the idea of keeping the run in userland rather than inside the middleware. This would also help when we switch to Unified and allow users to use different stream libs.
I don't use middleware. If I understand your middleware correctly you are just using it to build the driver for cycle, and subsequently also calling cycle run. I instead break the driver and the wrapping of redux's state in an observable into separate approaches. This is what it looks like...
// cycle driver for redux
export function makeReduxDriver(store$, dispatch) {
return function reduxDriver(input$) {
input$.subscribe(action => {
dispatch(action);
});
return store$;
};
}// redux store enhancer
import { Subject } from 'disco/rx';
function magic(
createStore,
reducer,
initialState
) {
const store = createStore(reducer, initialState);
const store$ = new Subject();
function dispatch(action) {
const dispatched = store.dispatch(action);
const state = store.getState();
store$.next({action, state});
return dispatched;
}
return {
...store,
dispatch,
store$: store$.share()
};
}
const enhancer = createStore => (reducer, initialState) => (magic(
createStore,
reducer,
initialState
));
export default enhancer;@kloy that's cool implementing this as a store enhancer, although you'd probably achieve the same using a more API-safe solution in the form of a middleware.
In terms of usage it doesn't change much from the solution I proposed above - still puts Cycle.run in user-land.
Thoughts @goshakkk @nickbalestra? We can:
- Create a single REDUX driver as specified above and pass
{action, state}along the stream. So it's objects in and actions out. Not sure of any shortcoming of this apprach - Otherwise we keep ACTION and STATE but we don't run Cycle.run from within the middleware, but in user-land (as my example above).
- Or we just don't do anything, keep it as is, and perhaps pass
runas a third argument when Unified is shipped.
Cast your preference!
- 👍 on the single
REDUXdriver (not sure about naming though) - 👍 on passing run as a third argument
- 👎 on having run in userland (current API are cleaner imho)
If run is not in userland how would you support users writing their own cycle adapters (I do this now for optimizing bundle size).
Also, I like the names store or redux for the driver name personally.
👎 on a single driver
👍 on either run as an argument or exposing ACTION and STATE drivers more explicitly
You can see referenced 2 PRs:
#26 is about using a single STORE driver - frankly I'm not keen about this solution.
#27 I think is the best solution. I exported run in user-land and am exporting drivers explicitly. Also upgraded to Cycle Unified so now we can use RxJs and added a test for it as well.
@goshakkk @nickbalestra @kloy
EDIT: the only thing I'm worried about with #27 is that the drivers are no longer using the store passed by the middleware, but the one from user-land (from createStore). I doubt there are differences though, right?