[redux-subspace] Thunk actions not namespaced
mradionov opened this issue · 11 comments
Is it a bug, feature request or question?
No sure if it's a bug, but maybe an incomplete feature
Which package(s) does this involve?
namespacedAction from redux-subspace used with redux-thunk middleware
Input Code
https://codesandbox.io/s/m33poxnnky
Expected Behavior
Basically I have some modular component Counter which is connected to a store (it has it's own reducer, actions and is communicating with the store using connect). I include this component somewhere in my app and want to be able to call it's actions from outside by a parent component Page. The problem is that a regular action creator which returns an object { type: String } is namespaced, but a "thunked" action creator is not.
This is namespaced (page/counter/INCREMENT):
// Counter
export const INCREMENT = "INCREMENT";
export const increment = () => ({
type: INCREMENT
});
// Page
dispatch(namespacedAction("counter")(counterActions.increment()));This won't be namespaced (INCREMENT):
// Counter
export const incrementAsync = () => dispatch => {
setTimeout(() => {
dispatch(increment());
}, 1000);
};
// Page
dispatch(namespacedAction("counter")(counterActions.incrementAsync()));I would expect my thunked actions be namespaced as well.
Current Behavior
I can see from source of namespacedAction it simply namespaces "pure" action creators like { type: String }. When I have a "thunked" action creator, it returns a function and is simply by-passed by namespacedAction and I end up with none of my "pure" action creators namespaced inside of "thunked" action.
Possible Solution
I was able to workaround it by creating a subspace for a child component from a parent component, but it looks like an overkill if I will have to do it in for every call. Also it seems that I must have to know of the entire store structure from the root.
https://codesandbox.io/s/5kq4zq3qxp
// Store setup
let store;
const getStore = () => store;
store = createStore(
rootReducer,
applyMiddleware(thunk.withExtraArgument({ getStore }))
);
// Page action
export const incrementCounterAsync = () => (
dispatch,
getState,
{ getStore }
) => {
const substore = subspace(state => state.page.counter, "page/counter")(
getStore()
);
substore.dispatch(counterActions.incrementAsync());
};Maybe there you could advice some easier way of getting it done without knowing the app structure.
Context
I wanted to control child components from parent components by calling actions of child components, because some things are pretty difficult to do only using props and I would like to have some imperative form of manipulating the child components.
Your Setup
Everything is the latest version except react-redux because it looks like the latest version is not supported yet.
"react": "^16.6.0",
"react-dom": "^16.6.0",
"react-redux": "5",
"react-redux-subspace": "3.0.1",
"redux": "^4.0.1",
"redux-subspace": "^3.0.1",
"redux-thunk": "2.3.0"
The issue here is that, like redux itself, thunks are not supported by default. They are just one of many middlewares that enable side-effects/async dispatching.
The easiest solution is to just namespace the actions dispatched internally to the thunk, e.g.
// Page
export const incrementAsync = () => dispatch => {
setTimeout(() => {
dispatch(namespacedAction("counter")(increment()));
}, 1000);
};but this can result in more duplication and in a more complicated thunk you might prefer to only define it once. The next level is to allow the thunk to take the namespace as an optional argument, e.g.
// Counter
export const incrementAsync = (namespace) => dispatch => {
setTimeout(() => {
dispatch(namespacedAction(namespace)(increment()));
}, 1000);
};
// Page
dispatch(counterActions.incrementAsync("counter"));If the namespace is falsey, namespacedAction will just return the original action, so it can be used safely internally to Counter as well.
This now makes the assumption you can change the internals to Counter, which isn't always the case when using libraries or if your don't "own" that part of the code (I've seen projects before where sepearate teams are responsible for diffent sections of the code). The minimal wrapper for supporting this use case (i.e. namespacing dispatches) would be something like:
// NOTE: untested
const namespacedThunk = (namespace) => (thunk) => (dispatch, ...args) => {
const namespacedDispatch = (action) => {
const namespacer = typeof action === 'function' ? namespacedThunk : namespacedAction
return namespacer(namespace)(action)
}
return thunk(namespacedDispatch, ...args)
}
// Counter
export const incrementAsync = () => dispatch => {
setTimeout(() => {
dispatch(increment());
}, 1000);
};
// Page
dispatch(namespacedThunk("counter")(counterActions.incrementAsync()));This does nothing to try to scope the getState arg of the thunk, so if the thunks are getting complicated enough to need that (or you want a more complete solution), you would need to do something more like what you have suggested.
Personally, I would write my own thunk implementation (it's not complicated) as you get access to the store from the middleware API, rather than needing to provide it as a extra argument, but there are some issues that would need to be worked through to get that working nicely. There is potentially a redux-subspace-thunk package that we could provide with a replacement thunk middleware and namespacedThunk wrapper, but there hasn't been a lot of people crying out for this to date. Let me know if you want to have the more complete solution and I'll put some more time into thinking through the issues.
The first and the second approaches that you've suggested are not ideal for me, because the counter kinda has to know that it is namespaced and has to handle namespacing inside. I would prefer if it happens under the hood or outside of the component which is being included in parent. Middleware sounds like a great option, thanks for this idea. I'll try to play around with it following you example and see if it solves my problems. Indeed, I sometimes have a logic inside thunked actions which involves calling getState(), so I'll need to patch it too.
If it won't take much time for you it would be great to have some guidance about the possible issues, because I believe you are more experienced with Redux internals and especially how to integrate it with redux-subspace. Maybe if I come up with something in the end and it will cover mine and most general use cases and possible issues overall, it could be organized as an extra package as you said.
Thanks for your detailed input.
Try this out for size: https://codesandbox.io/s/13603rjjo3
Check out the middleware/redux-subspace-thunk.js file for the midldleware and the wrapper.
This is basically what we described above. The main thing that needed consideration was how to enable getState, which boiled down to implementing the full subspace(mapState, namespace) api that is used all over this library. consequently I renamed the wrapper subspacedThunk instead of namespacedThunk to denote the additional funcitonality.
I also had to "wrap" the thunk in a new thunk to add the additional details so that the same thunk could be used with different namespaces, which is mostly unnecessary when using action creators, but because they are not compulsary, I put the safety in there, just in case.
Let me know how you go with this.
I just made a change to that sandbox to handle nested thunk dispatching (both multiple levels of subspaces and within the same level).
Thank you very much for it. I tried it out for a few components on the project and it is looking good so far. The only thing I've changed is that I've renamed subspacedThunk to namespacedAction and used it for both simple "object" actions as well as thunk actions. I believe that a calling component should not be aware if an action is a thunk and use the same helper function for all cases. It also allows to change internals of a called component whenever I want i.e. making simple action a thunk action and vice versa. The change was basically the following:
import {
namespacedAction as namespacedSimpleAction,
} from 'redux-subspace';
export const namespacedAction = (mapState, namespace) => (action) => {
if (typeof action === 'function') {
return {
[NAMESPACED_THUNK]: true,
mapState,
namespace,
thunk: action,
};
}
return namespacedSimpleAction(mapState, namespace)(action);
};The rest is unchanged. I am mostly using just thunk middleware on my projects, so I am not sure how it integrates with any other middleware.
I am going to use it from now on and see how it goes, I'll let you know if anything else comes up. I guess we can close this issue, if you don't have any more plans for this case. I am ok with custom implementation of thunk in my project, but I guess it would be nicer to have it is a package. I will be happy to help.
@mpeyper I'm trying to get redux-thunk set up too. Maybe you can shed some light on something that is currently leaving me very confused -- why doesn't it just work?
The docs say this:
Basically, when the dispatch occurs, it uses the dispatch function of the root Redux store, not the dispatch function of the subspace, so the namespacing is not applied properly.
But why is the dispatch function that of the root redux store? Subspace provider is putting into context an enhanced store object. Why would that store not simply have a subspace dispatch method which would make the appropriate processAction call? Redux thunk would pick it right up off the store.
Hi @conartist6,
The issue is the way middleware wires into redux. The thunk middleware is essentially:
return store => next => action => {
if (typeof action === 'function') {
return action(store.dispatch, store.getState);
}
return next(action);
};
}When you applyMiddleware(thunk) (the default redux one), the dispatch that gets passed in to the middleware is the root store's. The subspaced store's dispatch the that thunk get's dispatched into can be boiled down to:
function dispatch(action) {
if (namespace && isPlainObject(action) && action.type) {
return parentStore.dispatch({ ...action, type: `${namespace}/${action.type}` })
}
return parentStore.dispatch(action)
}The reason we only handle plain actions is that there are too many types of middleware for us to handle with custom implmentations within the dispatch function, so we defer to the middleware pipeline by allowing the parent store (which is eventually the root store) to handle it, but this comes at the cost any deferred dispatches using the root store's middleware pipeline.
This is why we provide our own applyMiddleware function (which is where that quoted section of the docs is from), which wraps the subspace with it's own middleware pipeline so that it does pick up the namepaced dispatch for cases like the thunk middleware.
This issue came about because it all works fine, so long as you dispatch your thunk from within the subspace you want to handle it in. If you want to dispatch a namespaced thunk from the subspace's parent (or higher in the ancestor chain) and have the child handle it, it falls down because the dispatch that is given to the thunk is the parent's, so the namespaces are calculated incorrectly.
That is a lot of words but I hope it helps your understanding. The long story short is use our applyMiddleware and thunks will work for most cases. The above solutions is for some usecases that won't work out of the box.
I think I do understand. I'm at least getting quite close to understanding all of it. Now I just need to do a few tests to see how applySubspace middleware really gets called. Is it used each time redux-subspace creates a new subspace store? It implies that it is, but I don't yet see how.
EDIT: Still really don't see how this happens. The arguments are even different. redux createStore() gives its enhancers (reducer, initialState) while subspace() gives them (store). It doesn't seem possible that applySubspaceMiddleware would be en enhancer to redux createStore, and yet that is how examples show it used.
It seems to me that what I think I want to do is possible too though. I'm now imagining writing this as a wrapper around SubspaceProvider:
const { mapState, namespace, middlewares, children } = props;
const subspaceDecorator = store => {
return subspace(
mapState,
namespace
)({...store, subspaceOptions: { enhancer: applySubspaceMiddleware(middlewares) }
}
<SubspaceProvider subspaceDecorator={subspaceDecorator})}>
{...children}
</SubspaceProvider>Now the subspaced component can easily have its own copy of thunk middleware which will execute prior to the subspace store's dispatch, capturing a copy of the subspace store's dispatch method. That seems to best match my mental model for what should go on: each subspace may need different middleware, and the configuration is done at the level that the required options could actually differ.
Also I see the problem with dispatching a thunk from the parent clearly now.
Closing due to inactivity. Happy to repoen if there is more discussion to be had.
@mpeyper @mradionov Thank you very much for your work on that matter. Its been a while since we wanted thunks to be compatible with namespacedAction.