-
Notifications
You must be signed in to change notification settings - Fork 106
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Request: Allow funcs as dependencies that take store as a parameter #26
Comments
@michael-wolfenden thanks for the kind feedback and for submitting this issue. I believe I understand what you need here. I want to be careful to allow people to use deps to pass in any sort of object, function, or primitive, so we'd have to keep that in mind with any additions. To go down that path, it seems like we'd need a special way to signify that this is a fn that receives state if redux-logic is supposed to automatically provide it and thus the API becomes a little more complex. So I'm assuming that you would have something like this currently. const api = {
foo(accessToken, id) // returns a promise
}
const logicMiddleware = createLogicMiddleware(logic, { api });
const fooLogic = createLogic({
type: FOO,
process({ getState, action, api }, dispatch, done) {
const accessToken = getState().accessToken;
const id = action.payload;
api.foo(accessToken, id)
.then(result => dispatch({ type: FOO_SUCCESS, payload: result }))
.then(() => done());
}
}); Personally this is how I like to craft my logic so that all of the redux related stuff is encapsulated in the logic keeping my API clean and separate from anything redux, so it can be used in other ways even without redux. So for me all the mapping to and from actions, state, selectors, dispatching, all of that occurs inside the logic so that everything else can stay pure. It is also a good practice to use selectors to find things in the state so you can easily restructure the state as necessary, so in the above example const accessToken = accessTokenSelector(getState()); // in fooLogic
// and the selector would have been defined in the same file with the reducer something like
export const accessTokenSelector = state => state.accessToken; So for me this is ideal having the api.foo be passed in accessToken and id, the two things it needs to do its work. It is easy to test in isolation, I don't have to set up a full state when I only need the accessToken. However in thinking through your desire to not need to always pass this accessToken through for every call. I'm guessing that your desired code would look like this. const fooLogic = createLogic({
type: FOO,
process({ getState, action, api }, dispatch, done) {
const id = action.payload;
api.foo(id) // only need to pass in id, since foo already has getState
.then(result => dispatch({ type: FOO_SUCCESS, payload: result }))
.then(() => done());
}
}); Basically eliminating the need to retrieve and pass the accessToken inside the logic. So while I would still prefer the previous form, I can see how some might prefer less repetition (though we have basically shifted the selector call from the logic to inside the api call). I had been already pondering the ability to add dependencies at runtime but I wanted to be careful on how this is done. So I am wondering if that mechanism could be used to help you with your goal as well. Here's how your code might look if we added the function createAPI(getState) {
// we basically capture getState in our closure so it doesn't have
// to be passed in later
return { // api
foo(id) {
const accessToken = accessTokenSelector(getState());
// return promise
}
}
}
const logicMiddleware = createLogicMiddleware(logic);
const store = createStore(reducer, applyMiddleware(logicMiddleware));
logicMiddleware.addDeps({
api: createAPI(store.getState)
});
const fooLogic = createLogic({
type: FOO,
process({ getState, action, api }, dispatch, done) {
const id = action.payload;
api.foo(id)
.then(result => dispatch({ type: FOO_SUCCESS, payload: result }))
.then(() => done());
}
}); If we added something like |
Thank @jeffbski for your response Your correct in regards to how we are currently using it. We are also using a selector to access the token. In terms of
So the Having said that either way will solve my issue. |
Yeah, I thought that's what you were getting at. My main reason for suggesting the alternative is that otherwise we'd need a way to indicate that a dependency needed to be called with state first before providing to the hooks. We can't just take the fact that it is a function since we could have lots of other deps that are functions (which are not expecting to be called prior to injection). So we'd likely have to use a special key or wrap in an object deps that need pre-execution with state. It gets even worse if we provide state rather than getState since then we have to execute this for each an every one of these before each and every hook in each and every logic even if they don't use them. Thus we will bypass a bunch of API complexity and extra work if we go with the addDeps approach. What should we call these injected dependencies? Is Since I'm adding a new API method, I guess I better see if this is the best term. We can always change variable names around later, but API method names are more rigid. Any thoughts on the best term for these injected dependencies? |
Hmmm, I see what you mean. How about |
I'll write up an RFC for this addition and see what everyone thinks. |
I have added https://github.com/jeffbski/redux-logic/releases/tag/v0.11.7 I'll go ahead and close this, but if there are any additional questions just let me know. |
We've been using this approach for setting API credentials: When a token is fetched from somewhere, it is stored in state by sending |
@tehnomaag That sounds like a nice approach, thanks for sharing. |
Firstly, just wanted to say that I'm loving the library.
Being able to pass a
dep
intocreateLogicMiddleware
is really useful , especially for testing scenarios, however there are scenarios where the dependency needs access to the store.For instance we have a
api
dependency, however the access token it needs is contained in the store. Right now, within the logic we have to set the access token onapi
by usinggetState
, it would be nice if we could registerapi
indeps
as a function that takesstate
and returns theapi
.The text was updated successfully, but these errors were encountered: