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

connect could be used with a custom store.subscribe method provided as option #269

Closed
slorber opened this issue Jan 25, 2016 · 13 comments
Closed

Comments

@slorber
Copy link
Contributor

slorber commented Jan 25, 2016

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 inside connect

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 to store.subscribe is hardcoded here:

    trySubscribe() {
        if (shouldSubscribe && !this.unsubscribe) {
          this.unsubscribe = this.store.subscribe(::this.handleChange)
          this.handleChange()
        }
      }

A flexible solution would be to let the user control hw to subscribe to the store himself, by using connect options for example:

const mapStateToProps = (state,props) => {node: selectNodeById(state,props.nodeId)}

const connectOptions = {
   doSubscribe: (store,props) => store.subscribeNode(props.nodeId)
}

connect(mapStateToProps, undefined,connectOptions)(ComponentToConnect)

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.

@slorber
Copy link
Contributor Author

slorber commented Jan 25, 2016

Let's also consider redux-batched-subscribe of @tappleby

It exposes 2 methods:

  • store.subscribe is batched (because it's wrapped)
  • store.subscribeImmediate is not batched

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.

@gaearon
Copy link
Contributor

gaearon commented Jan 26, 2016

If props was an argument we'd have to resubscribe on every componentWillReceiveProps for consistency which would give bad perf.

@slorber
Copy link
Contributor Author

slorber commented Jan 27, 2016

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:

const mapStateToProps = (state,props) => {node: selectNodeById(state,props.nodeId)}

const connectOptions = {
   doSubscribePropsSelector: props => props.nodeId,
   doSubscribe: (store,nodeId) => store.subscribeNode(nodeId)
}

connect(mapStateToProps, undefined,connectOptions)(ComponentToConnect)

If we use a selector to pick the data we need for the subscription, it gives the opportunity to componentWillReceiveProps to unsubscribe/resubscribe with the new nodeId only when it changes.

However the API becomes harder to use...

@gaearon
Copy link
Contributor

gaearon commented Jan 28, 2016

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 connect into an independent low-level function but I'm not sure about the API.

@gaearon
Copy link
Contributor

gaearon commented Jan 28, 2016

Basically, what if we separated the component in connect from the caching and invalidation logic.

@slorber
Copy link
Contributor Author

slorber commented Jan 29, 2016

@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.
See also this issue that can make it more complicated to build: facebook/react#3611

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 :)

@mhodgson
Copy link

@gaearon @slorber I have a PR that attempts to address this here: #285. Would be interested in your thoughts. Not ready to merge, but want to know if it is directionally interesting.

@slorber
Copy link
Contributor Author

slorber commented Feb 13, 2016

@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

@jtadmor
Copy link

jtadmor commented May 2, 2016

@gaearon @slorber @mhodgson

Was playing around with one attempt at splitting the core connect features into two different HOC: connector and cacher.

https://github.com/jtadmor/react-redux/commit/b829ece0688f71b0b5acb2ed73233f075487d8d6

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:

Cacher.propTypes = {
      ownProps: PropTypes.object.isRequired,
      storeState: PropTypes.object.isRequired,
      dispatch: PropTypes.func.isRequired,
    }

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:

const {
    store,
    subscribe = true,
    subscribeFromStore,
    subscribeFromStoreAndProps,
    resubscribeIf,
    dispatchFromStore,
    dispatchFromStoreAndProps,
    recalcDispatchIf,
    handleChange,
    pure = true
  } = options

These are designed to give consumers ability to customize the behavior, while falling back to sane default behavior, e.g. store falls back to context.store, dispatchFromStore falls back to (store) => store.dispatch and so forth.

You could call connector()(Component) and it would work just fine.

Regarding updating the subscription on componentWillReceiveProps, I've tried to solve that by allowing the options to specify either subscribeFromStore or subscribeFromStoreAndProps, and optionally allow a resubscribeIf that would look like (props) => props.node_id. This forces consumers to be explicit about whether resubscriptions should ever happen while the component is mounted but also provide fine-grained control.

I based this on suggestions here.

The major thing I'm not sure on is how to avoid setState on Connector when Cacher is not going to end up re-rendering. Right now the handleChange default behavior is to do a setState whenever the store broadcasts. Although Connector does allow for other optimizations, I'd need to figure out how to keep that one.

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 getOwnPropsFromProps = (props) => {} and getDispatchFromProps, getStoreStateFromProps likewise.

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.

@slorber
Copy link
Contributor Author

slorber commented May 2, 2016

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

Basically, what if we separated the component in connect from the caching and invalidation logic.

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

@jtadmor
Copy link

jtadmor commented May 2, 2016

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 setState calls before you even get to the Cacher component. The consumer can perhaps optimize in other ways by provider a better subscribe, (such that the callback should only fire when we are actually interested in updating), but we lose this tool in the default case, which is a no-no.

Perhaps I'll try to organize something like this:

import { cacheMe } from 'react-redux'

// Render function of a HOC
render() {
  cacheMe.apply(this)

  return this.renderedElement
}

But we still have the major problem, I think, that the handleChange method is deeply tied to the implementation details both of the "caching" and the "get new state from store" parts of this component.

So while I could easily see just shuffling a few things around and using a custom method instead of store.connect, something like returning a Promise instead of just calling setState isn't going to be nearly as easy to let the consumer control.

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,

@jimbolla
Copy link
Contributor

If you look at #405, I've re-implemented connect in such a way that splits caching and connection. It's still a single component, but all the cache maintenance is handled by reselect.

@timdorr
Copy link
Member

timdorr commented Aug 14, 2016

This is now solved via #416 and what's currently published on npm as react-redux@next

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

6 participants