-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add an action listener callback middleware #237
Comments
This is how I described this approach to my coworkers: There are essentially two schools of thought for working with side effects with redux. The first approach is to put the side effects in the actions -- e.g. instead of dispatching a simple value, dispatch a promise, or a function that calls side effects. Middleware like redux-promise and redux-thunk intercept these non-value actions and run them. The second approach is to put the side effects directly into the middleware -- the actions are always simple values, but the middleware listens to these actions, and can run side effects in response to them. This is the approach used by redux-saga and redux-loop; it's also how logging and metrics-reporting middleware work. I prefer the second approach because it encourages you to keep the "how" with the state, and the "what" with the UI. However, the tools associated with this approach, particularly redux-saga, tend to have a pretty steep learning curve. The approach I'm using here is less expressive, but much closer to the complexity of redux-thunk. |
Yep, that's the idea I'm going for here. One possible improvement I'd like to consider is adding and removing subscriptions at runtime. The likely approach would be |
What's the status now? It seems to me that the |
Keep in mind, these are still reducers and should not have any side-effects. But yes, API-wise I guess it will go in a similar direction as extraReducers, but we have no definite design yet. Suggestions welcome. |
Yes, what I mean is I can respond to dispatched actions to change state in different slices, which is sort of listening behavior. I have seen the listener pattern in By the way, I do think it's a very important feature to be included in redux-toolkit. |
Yes, this is a feature to make this reducer to react to actions from a different slice/other action. I hope the current documentation reflects that. this part of the StyleGuide should also reflect that we encourage that pattern. What this is not is a feature to trigger asynchronous actions (thunk-like) from anything. I just assumed you were talking about that, because this whole issue is about a middleware for asynchronous actions. |
@phryneas I wasn't aware that this issue is about asynchronous actions. I think a listener pattern is all about do something when some action is dispatched, no matter it is a synchronous or asynchonous action. So if we have the listener pattern implemented, the extraReducers syntax is no longer needed. |
No, because an asynchronous listener cannot change state. Only reducers do. These are completely different responsibilities. An asynchronous listener could only dispatch actions that finally might lead to state change. The distinction is:
|
Yeah, per my original comment, the one missing use case with RTK atm is that there's no built-in way to kick off additional logic in response to dispatched actions. Sagas and observables let you do that, but they're both very heavyweight in terms of bundle size, complexity, and mental overhead. Given that, I'd like to provide a much lighter way to let people run simple callbacks, probably with access to |
When(roughly) would we expect to see this feature? |
No concrete plans or timing atm. Like, it's a thing I want to do, but I'm focused on docs work right now. (Or, more accurately, I'm on business travel for the next few weeks, and don't have much time to do either docs or coding). If someone would like to pitch in and help work on this, I can help steer the work in the direction I'd like. |
You know the drill, give me a direction and I'll try some things out ;) The question is: do you want to do this
|
Yeah, this does go off into some larger API design questions than I think I'd originally realized. Thinking through possible requirements a bit:
But, from there, I'm not sure where and how all the listeners should be defined and pulled together. The Glitch middleware allows you to pass in multiple handler maps at middleware creation time. That's not a bad initial approach, especially if there's the ability to dynamically add and remove listeners at runtime. I don't think I want to define a middleware per slice as you have in the initial PR. If we assume we add some kind of a Also, how should the various configureStore({
reducer,
middleware: [...getDefaultMiddleware({
listeners: [sliceA.listeners, sliceB.listeners]
})]
}) by hand, or should there by some other approach? I'd still prefer to not have any kind of a special "combine slices" function or anything like that. This also goes off into the "larger Redux abstraction" land occupied by Rematch, Easy-Peasy, and Kea. It would be worth reviewing their APIs to see what the capabilities are, what the API looks like, and how it's implemented:
Overall, I think there's value in this idea, but I also want us to come up with something that's reasonably scoped and doesn't turn into a monster. |
Hehe, while you've been writing here, I've been writing some of the same questions in the Draft PR :D While I see the negatives, I also see one big positive of adding it to the slice: the logic stays much more together. If we could solve those problems, I think that could be a big plus, keeping it more "ducksy". As for the dynamic action subscription via action you are suggesting, I see problems with that. So if there were to be a dynamic subscription mechanism, I'd put it as a method on the enhanced store. Either as a |
It'd be easy to have the middleware generate a unique ID when you ask to subscribe and return that from But yeah, I can also see potential pain points from managing that as well. |
Yeah, returning an unsubscribe function would also be no problem there. But I've been bitten by shoddily written middlewares throwing away return values from other middlewares in the past, so I've come to the point where I never trust the return value of a dispatch - especially not for something so important. |
The other question that comes up here: should there be a way to define thunks as part of a slice? If so, how? |
Phew. I'd say no, because it does not provide any extra value. Where the slice reducers/extraReducers build you a reducer and listeners would build you a middleware (and they all benefit from some kind of map object or builder notation), thunks would still be thunks. I'd encourage people to put thunks into the same file as the slice, though (as long as size is manageable). |
Any update on this? I think this is a very important feature as there is currently no good way to do middlewares in |
configureStore has a |
Yes, but this does not play nicely with export const syncToDb = api => next => action => {
switch (action.type) {
case actionTypes.RANDOM_ACTION: {
....code here
}
}
} I see 2 problems here:
|
Maybe using a type guard instead of a discriminating union will help you there? If you are using the discriminating union pattern, you are essentially lying to your compiler, as there is actually no guarantee that only actions you know of will be passed as an argument to your reducer/middelware. |
This looks good. Does it work with |
Works with |
So you can do something like this? mySlice.actions.myAction.match(incomingAction) {
// incomingAction.payload <-- is typed here
} |
Yes. |
I'm pretty late to this discussion, but I've read over the comments here and the #272 PR and I've got some thoughts.
My ideal API would look something like: import { createAction, createSlice, createListener, configureStore } from "@reduxjs/toolkit";
const triggerAction = createAction("triggerAction");
const slice = createSlice({
name: "example",
initialState: {},
reducers: {
something(state, action) {},
somethingElse(state, action) {}
}
});
const listener = createListener({
[triggerAction](action, { dispatch, getState }) {},
[slice.actions.something.type](action, { dispatch, getState }) {}
});
const store = configureStore({
reducer: {
slice: slice.reducer
},
listener
});
listener.add({
[slice.actions.somethingElse]: (action, { dispatch, getState }) {}
});
listener.remove(triggerAction.type, slice.actions.something); Note: in this API I haven't put too much thought into edge cases or usage beyond trivial examples, and I'm not familiar at all with the RTK implementation (although I'm very familiar with the internals of Redux itself), so it might be all kinds of broken. I'm happy to take a run at a PR if others like this API. |
Oh duh. Why was I getting confused on that part? I sorta see what you're saying about "listening to actions from React components", but I'm not overly concerned about that. If there's consensus that it's a bad idea, we can put a note in the docs that says "hey, even if this is possible, we recommend against it because..." and give some specific reasons. I don't think I like that |
Listening to actions in react components can be a really nice pattern for separating your code fwiw - much easier to code split some complex logic and embed it into view chunks if you're able to do that. Side-effects based on changes to state are also super useful, shame that it sounds like the api direction won't support that. 😕 |
@thisRaptori : the "predicate API" I proposed would likely allow that: type ActionListenerPredicate = (state: State, action: Action) => boolean |
Oh huh missed that part somehow! 👀 If we're ensuring these actions to add/remove listeners don't reach the store, what's the advantage of having them be actions at all? Building an enhancer instead of a middleware (which would then add an |
👋I thought I'd mention that (ages ago) I built something similar for myself which addresses some of the points mentioned in this issue and could be of inspiration (though you're welcome to ignore it too!). |
@thisRaptori : think you're misunderstanding things a bit. Of course the actions reach the store, otherwise they'd be completely useless. The point is they don't reach the reducers. I just wrote an extensive Reddit comment talking about how that's an intended use case for middleware. The problem with making it an enhancer that adds an extra method to the store instance is that now you need actual access to that store instance in order to call it. That means either directly importing the store (a no-no) or calling the On the other hand, everything has access to @jameslnewell : yep, I have that one listed in my Redux addons catalog already :) along with a bunch of other similar libs: |
@markerikson yeah I understood, I guess I just fall into the camp of feeling pretty uneasy about opaque middleware which intercepts actions like that. Know it's something which has been (more or less) embraced, but I'm still not quite convinced it's a net positive. Yeah I was assuming it'd be exposed via a custom |
Right, this is another reason why I would want a |
ok, just to confirm @markerikson, you're version looks something like: import { createAction, createSlice, createListener, getDefaultMiddleware, configureStore } from "@reduxjs/toolkit";
import { addListener, removeListener } from 'redux-action-listener' // package name to be determined
const triggerAction1 = createAction("triggerAction1");
const triggerAction2 = createAction("triggerAction2");
const slice = createSlice({
name: "example",
initialState: {},
reducers: {
something(state, action) {},
somethingElse(state, action) {}
}
});
// can construct with action creator or action type
const listener1 = createListener(triggerAction1, (action, { dispatch, getState }) => {})
const listener2 = createListener(slice.actions.something.type, (action, { dispatch, getState }) => {})
const store = configureStore({
reducer: {
slice: slice.reducer
},
middleware: getDefaultMiddleware({
listeners: [listener1, listener2]
// other options take objects, but not sure it makes sense here...
// perhaps `listener: { listeners: [...] }`?
// I've got an idea that they could accept an `extraArgument` similarly to `redux-thunk`
// so having the ability to extend the config might be useful
})
});
// can add by action creator or action type
store.dispatch(addListener(createListener(triggerAction2.type, (action, { dispatch, getState }) => {}))
store.dispatch(addListener(createListener(slice.actions.somethingElse, (action, { dispatch, getState }) => {}))
// can remove by action creator or action type
store.dispatch(removeListener(triggerAction1.type))
store.dispatch(removeListener(slice.actions.something)) My only concern with this approach is that it's the only thing in the standard setup that forces you to use The dispatch to add/remove is something we used at my workplace for another custom middleware (adding analytics handlers) and it worked really well for us. The only difference was that we cleansed the action of the attached function (so it was now serialisable) and passed it on. This way, other things could react to to the action type (although nothing ever actually did) and it showed up in the dev tools, which was useful when debugging why we were reporting out analytics events multiple times (the "same" handler was being attached multiple times through multiple dispatches). All the reducers treated it as a noop so in the end there was very little downsides to passing a cleansed version of the action on. I'm not saying we should definitely do that this time, but it might be worth considering. |
@mpeyper : roughly along those lines, yes. Agreed that having to call |
This API would be pretty helpful for use cases where you have to create one-off subscribers for workflows. Here's an example: a button triggers an upload flow through a library that takes callbacks, which result in a "success" or "fail" action being dispatched. Then the thing that owns the button has to do something with the result (not handled by the reducer). You can detect the result in React.useEffect style by monitoring state changes and expressing conditions for the code to run in terms of state, but it seems a lot simpler to be able to subscribe to the event stream. In some ways it seems like the base store object should just emit the // store.js
const store = createStore(/* */)
// react-app.js
// show upload ui based on store state
// maybe-not-react.js
onUploadButtonClick = () => {
store.dispatch({ type: 'requestUpload' })
let observer = {};
observer.unsubscribe = store.subscribe((change) => {
if (change.action.type === 'uploadSuccess') {
this.setUrl(action.payload.url) // read from payload?
this.setUrl(selectUploadUrl(change.state)) // read from change object?
this.setUrl(selectUploadUrl(store.getState())) // read from store state?
observer.unsubscribe() // done listening
}
})
} |
Lenz just put up a new PR at #547 that tries to implement this. After looking it through, I think it's pretty close what I was envisioning. Can we get some other eyes on that PR and some feedback? |
Our app currently uses We've started looking into alternatives. Thunks get us close, but it's missing the event listener support we rely on in our sagas. This solution seems like a perfect blend of the two, and I was disappointed to see that any progress on this feature was dropped in May. Is there anything I can do to help push this forward? |
It feels like the big question atm is around what the desired semantics should be. That's where #547 bogged down. For example, see #547 (comment) and #547 (comment) . There needs to be an agreement on what exact semantics this listener needs to implement, then figuring out how to implement those semantics and what the API should look like. |
But seeing that the current state of #547 is quite usable, I'll probably just go and release that over into https://github.com/rtk-incubator sometime soon. |
Agreed! That is the boat that we are in. Our main reasons for wanting to phase out Saga:
In general, having some sort of takeLatest/takeLeading esque option on a per function basis would be a game changer. If that were the case there would be practically no reason to use Saga for almost any use case (except maybe a complex auth flow, for example). This type of functionality is probably way out of scope for the initial implementation of this feature. Do you think this is something that could easily become part of this middleware at some point? |
Can you give some examples of what this might even mean or how this might behave for an "action listener" middleware? I can see how "take latest" would work with sagas, because the saga middleware can cancel a saga that might be in-progress when a newer action is dispatched: I don't see how that could work conceptually with the way this middleware is designed - there's just a callback that runs to completion, so there's nothing to cancel. I could sorta maybe see a "take leading" implementation doing something by unsubscribing the listener when it starts running, and re-subscribing after it's done, although again I'm not sure that's entirely applicable to callbacks instead of sagas. On the flip side, perhaps that sort of scenario would be a use case for "XState-as-middleware"? |
I forgot to mention it earlier, but I did publish a standalone version of this middleware as https://www.npmjs.com/package/@rtk-incubator/action-listener-middleware a few days ago. We'd appreciate feedback on whether this API is working out! |
I finally finished such a middleware as I see it. Here's the repo, but I still need to finish the docs. And also it has another approach to bind a handler to action, but, in general, it may show some use-cases I see important.
I implemented two important ideas here, but I'm not sure whether they're generally good:
I have several articles with evolution and reasoning for this library, but it's written in Russian, so I'm not sure how I could deliver them here |
I just collected a list of relevant existing discussions and comments over in the corresponding Discussions thread: I'm going to go through this again tomorrow and extract a list of bullet points for things we ought to nail down specifically. |
And following up from that: I've gone through the issue/PR threads for our WIP "action listener middleware", collected a list of outstanding API design questions that need to be solved, and summarized them along with my own suggestions: Please give us feedback on these questions! |
I just published v0.2.0 of Please see the discussion in #1648 for a change log of the temporary package and provide us feedback! |
This issue has been resolved with the release of |
Thunks are easy to use and a good default, but the biggest weakness is that they don't let you respond to dispatched actions. Sagas and observables are very powerful (too powerful for most apps), but they do let you kick off additional logic in response to actions.
I've been considering adding some kind of middleware that would be in between - something that lets you run callback functions in response to specific actions, but without the complex overhead of sagas and observables.
I have links to a bunch of existing middleware under the Middleware and Middleware - Async sections of my addons list. One of those might do what we need, or we can take inspiration.
Glitch appears to have one at
handler-middleware.js
, which maybe @modernserf could say something about.The text was updated successfully, but these errors were encountered: