-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
connect could be used with a custom store.subscribe method provided as option #269
Comments
Let's also consider redux-batched-subscribe of @tappleby It exposes 2 methods:
If you use this store enhancer in your project, then all your connect HOC's become batched by default. This proposal would permit the user to connect components in both batched and immediate mode. I don't really have a real usecase for this but maybe it could permit to give some control over the priority the components should render themselves. |
If |
That's right but I guess it's a very specific corner case as most of us would probably never want to update the subscription without unmounting/remounting. There's still a solution to this problem:
If we use a selector to pick the data we need for the subscription, it gives the opportunity to However the API becomes harder to use... |
There are some other extension points people desire:
I wonder if there is a way to address these together without complicating the API. This might mean extracting the “core” of |
Basically, what if we separated the component in |
@gaearon a usecase for #208 would be to build a React.Perf middleware so that we can take measures for every dispatched action. If subscription returned a promise resolved on setState callback, then the redux Middleware could be able to know when rendering has ended and measure wasted time that is relative to a given action. There's already a not-really advanced middleware that does not take account that renderings can happen in an async way: https://github.com/AvraamMavridis/redux-perf-middleware Also for my initial custom connect proposal I think another usecase is when we want to bind dom inputs to redux state. It might work well with events on a small app but I guess once the app becomes big, on a mobile device typing in an input triggering hundreds of HOC may be a problem? @gaearon yes that would be great to provide full flexibility so that we can compose code in the way we want to use it without rewritting/forking the lib. Also it can be nice to have an experimental API so that you can eventually change your mind later. I don't have an API in mind I'll have to digg deeper in the implementation details of connect :) |
@gaearon also it seems there is a community need to run multiple redux stores (reduxjs/redux#1385) It could be nice if we could also pass to the provider a map of stores and if the connect method was flexible enough to permit to select the appropriate store from context |
Was playing around with one attempt at splitting the core connect features into two different HOC: connector and cacher. The basic idea is that cacher should not know at all about the store or where state comes from. It receives all of the old connect() arguments, but doesn't actually hook up to a store. Instead, it requires the following props:
Instead of a handleChange method, it will now attempt to calculate mapStateFromProps only when storeState changes or if mapStateFromProps depends on props and ownProps changes. Ditto for mapDispatchToProps. The connector accepts a wide variety of options:
These are designed to give consumers ability to customize the behavior, while falling back to sane default behavior, e.g. You could call Regarding updating the subscription on I based this on suggestions here. The major thing I'm not sure on is how to avoid What I'm not sure on is the best API for each. Right now they are separate functions, but still very coupled in that Connector passes down storeState, ownProps, dispatch, and Cacher consumes those. One possibly way to make them more re-usable is to allow, e.g. Cacher options argument to let the consumer specify something like Also, we now have two wrapper components instead of one to provide the same basic functionality as before. (This could be mitigated by keeping connect as is, but export the two more modular HOC as well, although that leaves future development a pain). Let me know if this seems like a fruitful avenue. |
At first glance I like the extensibility and decomposition of the problem but I'm not sure it would be nice for performances to have 2 wrappers instead of one, when we already focus on optimizing the single one we currently have. I don't know what @gaearon exactly had in mind when saying
But I guess the idea is that the caching logic would be an utility function instead of a new wrapper, to avoid degrading performances See also work being done here: #368 |
Yes, after having quickly made these changes I'm realizing that without any coupling, you just subscribe without any filter, you already make way too many Perhaps I'll try to organize something like this:
But we still have the major problem, I think, that the So while I could easily see just shuffling a few things around and using a custom method instead of In any event, it does seem prudent to wait until #368 is wrapped up, because that will change the nature of the work that needs to be done here, |
This is now solved via #416 and what's currently published on npm as |
Context
Today there was some discussions about the new redux tree view example and this SO question:
http://stackoverflow.com/questions/34981924/performance-issues-with-a-tree-structure-and-shouldcomponentupdate-in-react-re
As I pointed out the solution of Redux examples is fine enough for most usecases but does not scale it the number of nodes is very big. We agree that it's not a good idea in the first place to render a huge list or a huge tree, but it seems the author of the question is still interested in a solution. I don't know if the usecase is valid or an antipattern but I guess it's worth giving the opportunity to render a huge number of items in an efficient way
Solution
As described in my answer if the number of connected items grows too big, then every state change triggers all connected HOC's subscriptions so it does not scale well.
If we want to make it scale better, the HOC subscriptions should only be called when necessary.
If the state slice of node with id=1 is updated, it produces overhead to actually trigger the subscription of the HOC that connects data to node with id=2, because obviously the connected component is not interested at all in this change.
Redux store has a
store.subscribe(listener)
method, and it is called insideconnect
One can easily create a store enhancer that exposes a method like
store.subscribeNode(nodeId,listener)
, which is only triggered when that node, with the provided, id is called. There are multiple ways of triggering this listener efficiently.The problem is that once we have built this custom optimized subscription system, there's no way to reuse
connect
to listen to it because the call tostore.subscribe
is hardcoded here:A flexible solution would be to let the user control hw to subscribe to the store himself, by using connect options for example:
The default
doSubscribe
implementation would be:doSubscribe: (store,props) => store.subscribe()
which keeps the current behavior unchanged.There's one corner case I don't know yet how to solve is when the nodeId props changes over time. It would mean the HOC should then unsubscribe and then resubscribe for the new nodeId. In practice this seems farfetched to do so and I'm not sure it's really worth trying to solve this but if anyone has an idea...
I can make a PR for that, just tell me if it would be accepted or if it seems to much indirection and is not an usecase you want to support in redux.
The text was updated successfully, but these errors were encountered: