Skip to content
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

Feature request for a notify middleware: Prevent notifying all listeners in certain conditions #2445

Closed
zeachco opened this issue Jun 9, 2017 · 9 comments

Comments

@zeachco
Copy link

zeachco commented Jun 9, 2017

Hi, this is a feature request

What is the current behavior?
Every dispatch cause all listeners to be called without the possibility to prevent it.

The "hackish" to do it would be with the enhancer function from createStore. I don't find this clean for the following reason:
It require to rewrite createStore completely which make me wonder why do we require Redux in the first place if it's to pass a function that replace it completely.

What is the expected behavior?
I think allowing to pass a middleware would be helpfull for cases such as Redux used with Immutable.js. A middleware could verify that the state has muted and not call uselessly all the listeners if not. Other usecase could also apply such as "locking" dispatch in certain conditions for other performances reasons.

I think the goal of Redux is to notify the state changes, not to notify that a dispatch happen otherwise his role wouldn't be to hold a store a well but more a simple event bus system. Maybe I didn't get correctly the misson of Redux here so let me know if I'm in the wrong here.


The current Redux version is 3.6.0
this applies for all browsers / node environments and all OS


I currently have a working version of this which need polishing so I would provide the PR if this feature is accepted.

Thank you

@zeachco zeachco changed the title Feature request for a notify middleware: Prevent notifying all listeners in certain condition Feature request for a notify middleware: Prevent notifying all listeners in certain conditions Jun 9, 2017
@timdorr
Copy link
Member

timdorr commented Jun 9, 2017

This isn't really the job of redux, as it's supposed to be small, simple, and somewhat dumb in it's implementation. It shouldn't distinguish who to notify because those listeners can use the state in a variety of ways. Basically, it can't and shouldn't know how state is used.

It sounds like what you want is reselect: https://github.com/reactjs/reselect

@timdorr timdorr closed this as completed Jun 9, 2017
@zeachco
Copy link
Author

zeachco commented Jun 9, 2017

@timdorr thanks for the fast answer, I think I didn't correctly described the issue here.

I do not implies that Redux should handle when or when not to notify listeners, but being able to receive a function that does that check for him which knows about the data structure etc.

The point here is that in a complex application with multiple subscribes everywhere we have no ways to prevent a loop of dispatcher to being called and this sometimes kills the application. Additionally, some nice tools such as reduxDevTool can spam a lots of dispatch information which just crash the browser or generate so much noise that it becomes useless.

I will give you an example of a usage and let me know if that makes sense for you

// User can specify when he wants the notify to happens
// In this example (so it make sense) I use Immutable.js
// so I can test if there is a change in any node of the store with one super fast check
const preventUselessNotifies = (oldState, newState) => !(oldState && oldState.equals && oldState.equals(newState));

store = createStore(reducers, initState);
// This is an example of setting the middleware but could be passed directly in the createStore too
// I found it easier to add this in the API since the argument checks in createStore are implicit
store.setNotifyMiddleware(preventUselessNotifies);

I know react-redux's connect does a check and manage his own listeners to avoid calling a thousand components on every dispatch if there is 0 changes. They managed to optimise this on their part but some application use a mix and match of connect, store.suscribe and other utils, at a certain level it doesn't scale well and optimisation could easily take place in the core where the notifies happens in the first place.

Alternatively, if Redux still don't want to go there, It's not impossible to reuse the optimisation from react-redux as a layer replacing store.suscribe everywhere (causing a minimal pain for large project that needs to refactor) and managing listeners on his side, but then again, we use an even dispatcher to dispatch to an other event dispatch which optimise, feels like we would do something wrong here.

@zeachco
Copy link
Author

zeachco commented Jun 9, 2017

I linked an example PR to help illustrate what I meant, this is not polished, just explain better what I meant

@markerikson
Copy link
Contributor

markerikson commented Jun 9, 2017

@zeachco : the "right" way to implement this is as a store enhancer that would override the store's dispatch() API and supply its own "notify subscribers" logic.

For a related discussion, see #1813 .

Are you actually experiencing performance issues of some kind, or is this a more theoretical concern?

@zeachco
Copy link
Author

zeachco commented Jun 9, 2017

There is a measured performance impact, but I understand my case might be very specific as it's a fast paced multiplayer platform and event dispatches happen very often (much more dispatch than a traditional web app).

If I understand correctly, the proposed solution is to transform my latest PR into a middleware/enhancer instead (which replace Redux completely and remove the need to have Redux in the first place since this is what the enhancer does: replacing createStore).

I was hoping to have a different solution as this freeze the snapshot-ed version of createStore in time and just create a cloned version with yet another name and get maintained separately (which is a problem in javascript I wish I wouldn't contribute).

I will read #1813 and probably continue using an in-house solution for that specific project.
I understand the need for rigidity on a project used by so many projects.

@markerikson
Copy link
Contributor

Uh... no, you're misunderstanding what enhancers are.

A store enhancer accepts createStore as a parameter, but then can return a new store object. This allows the enhancer to substitute its own version of functions like dispatch or subscribe instead of the original versions.

As an example, applyMiddleware is a store enhancer. It creates the "base" store, creates the middleware pipeline, and returns a new store object with the middleware pipeline as the new dispatch function.

Another example is the redux-batch enhancer, which replaces the store's default dispatch and subscribe behavior, and allows you to pass in an array of actions to dispatch with only a single notification.

So no, enhancers do not "remove the need for Redux", enhancers build on the Redux core to provide customized behavior.

@zeachco
Copy link
Author

zeachco commented Jun 9, 2017

I didn't get that part indeed 😅 , it looks to me that the confusion comes from https://github.com/reactjs/redux/blob/master/src/applyMiddleware.js#L21
which cause to store to be created with his own scoped variable we can't access but the point is not to use applyMiddleware, am I right? I haven't explored this way so I guess I am wrong here, I will read these last resources and hopefully will come to a nicer solution on my end.

Thanks again

@zeachco
Copy link
Author

zeachco commented Jun 9, 2017

For future references:
https://gist.github.com/zeachco/1195b69744a899a1162c704bfa69fe5e

For my case, it worked with minimal code in the enhancer, I didn't had to rewrite the dispatch functionnality

@markerikson
Copy link
Contributor

Yep, told you something like that would work :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants