cc@slorber,@ellbee,@tgriesser who might be interested in this

This one looks like a clear win to me, but I wonder if it would be worth working on a benchmark suite again like the one started in#104? I've used benchmark.js before, but not for testing UI code. Is it even really feasible?

For now, I’m testing againsthttps://github.com/mweststrate/redux-todomvc manually.
My work in progress on optimizing app itself to Redux patterns is inmweststrate/redux-todomvc#1.

Yeah, I have just been reading through that issue. It is what made me start thinking about it. I might have a play around with it.

👍

Oh wait, I’m supposed to emoji your post inline.

@gaearon this is great news that connect not using component's props get faster :)

However I'm not sure the current implementation is the best we could do:

• The factory method works but is kind of unintuitive API at first
• If a connected component do care about ownProps, and these props can change (ie can't use the factory), then the component will setState even if the props don't change often

Instead of

functionmakeMapStateToProps(initialState,initialOwnProps){varid=initialOwnProps.idreturnfunctionmapStateToProps(state){return{item:state.items[id]}}}connect(makeMapStateToProps)(Component)

I would rather have something like:

connect({mapperPropsSelector:ownProps=>{id:ownProps.id},mapper:(state,{id})=>state.items[id]})(Component)

(this is probably not the best API but you get the idea)

What it permits here is to make it clear on what the mapping function has to rely on to do its job, so you can more easily detect when props needed for the mapping have changed and run the mapper only when needed

And I think, by default, not providing any ownProps to the mapping function (ie no selector provided) would actually encourage people to write more optimized code by default (unlike factories which would be opt-in optimization), but it would be a breaking change

(This is related to my proposal for more flexible subscriptions here:#269 (comment))

I’m confused: how does accepting a selector for props solve the problem ofsetState()? As soon as something depends onownProps, due to#99, we are forced to delay its calls untilrender() at which point we have already paid the price ofsetState().

Sorry if I'm going on a tangent with this comment but I'll share some performance optimizations I implemented earlier this month when I rewrotereact-redux-provide. These optimizations might not perfectly translate toreact-redux but perhaps similar algorithms could be implemented.

I added awatch method to the store which accepts areducerKey and acallback function to be called whenever the reducer returns a different state (via a reducer enhancer). The declarative nature ofreact-redux-provide makes it possible to automatically efficiently watch for changes to the relevant reducers, and in which case, adoUpdate flag is raised so that we can simply callforceUpdate after the action has completed, since we already know some relevant state has changed. The wrapped component's props are cached and updated accordingly per reducer. This means no need for shallow comparisons or loops, andshouldComponentUpdate can always return false (unless the component receives new props of its own, of course). And so updates should be near instantaneous as they, in theory, only require a few clock cycles per relevant component - i.e., new state -> relevant components -> update.

Condensed version:

for(letreducerKeyofreducerKeys){this.unsubscribe.push(store.watch(// when some reducer returns a new statereducerKey,nextState=>{// update wrapped component's propsthis.componentProps[reducerKey]=nextState;this.doUpdate=true;// and we know for sure we should update}));}this.unsubscribe.push(store.subscribe(()=>{// after action has completedif(this.doUpdate){// simply check for doUpdate flagthis.forceUpdate();}}));

See the full version where the same method is used for props derived from some combination of state and own props:
https://github.com/loggur/react-redux-provide/blob/master/src/provide.js#L93-L212

To elaborate a little bit more, deriving props from a combination of state and own props looks like this:

// here we'll provide some item from some listconstmerge={item:{// this is run only for components with an `item` propTypekeys:['list'],// the provided `item` depends on the state of the `list`get(state,props,context){// this function is executed only when `list` changes// and components will be updated only when the `item` has changedconst{ list}=state;const{ itemIndex}=props;returnlist[itemIndex]||null;}}};

There are alreadytests to ensure renders occur only as absolutely necessary, but more in-depth performance tests are coincidentally next on my todo list. I was planning on seeing how things perform when incorporated intodbmon, but I think I'll also run some benchmarks onreact-redux-provide/examples/todomvc.

I’m confused: how does accepting a selector for props solve the problem of setState()? As soon as something depends on ownProps, due to#99, we are forced to delay its calls until render() at which point we have already paid the price of setState().

@gaearon sorry it's hard to think well on such code late in the night :) My proposal may not prevent thesetState cost finally.

The problem I see with current code is that for example if this component changes from

<Todoid={123}className="someName"/>

to

<Todoid={123}className="anotherClassName"/>

then yes we necessarily have to do asetState because ownProps have changed and we must render the child. The problem is that here ourmapStateToProps is relying on ownProps but actually does only care about theid, and notclassName.

This line:

shouldUpdateStateProps=hasStoreStateChanged||(haveOwnPropsChanged&&this.doStatePropsDependOnOwnProps)

it will makemapStateToProps be run in such a case, while it could have been avoided if we could have known thatmapStateToProps is not interested to receive theclassName property but only the Todo id.

Also, not a big deal, but not surethis.doStatePropsDependOnOwnProps = this.finalMapStateToProps.length !== 1 is relyable if someone writes mapStateToProps witharguments[0] (yes it is unlikely... :p). It would become more relyable if that wish to depend on ownProps was more explicit

This is only what I can say for now, I have to study the code a bit more to say anything else :) (ie#99)

@gaearon Here ismy use case, in which early bailout yields significant performance boost.

With regards to@slorber's concern that propscan change, I encountered a similar issue, and I did this hack to get around it.

Let's say we have:

//factory for map state to props, as per Dan's suggestion aboveconstmakeMapStateToProps=(initialState,initialProps)=>{const{ userID}=initialProps;//props that's not expected to change (often)constmapStateToProps=(state)=>{constuserNickname=state.users[userID].nickname;//slice of state dependent on propsconstuserAvatar=state.users[userID].avatar;//slice of state dependent on propsreturn{      userNickname,      userAvatar};};returnmapStateToProps;}constUserComponent=React.createClass({// component implementation});exportdefaultconnect(makeMapStateToProps)(UserComponent);

Of course, userID isn't expected to change, but there may be some cases where it does. For example, the above component may be a react-router route component, which gets its userID from the pathname:http://example.com/userpage/cat. When the path changes tohttp://example.com/userpage/dog the component holds on to stale userID (cat) and this causes problems. (because react router doesn't re-mount for the same route)

Icould revert back tomapStateToProps (state, ownProps), but I would loose the performance boost gained from the early bailout. So instead, I do this:

//instead of this,exportdefaultconnect(makeMapStateToProps)(UserComponent);//do this:constConnectedComponent=connect(makeMapStateToProps)(UserComponent);exportdefault(props)=>(<ConnectedComponentkey={props.userID}{...props}/>);

Because of key, when userID is changed, the old component is completely unmounted, and new one is mounted instead, with the updated props. It's a bit hacky, but I think this is an acceptable trade-off. I guess for multiple props that aren't expected to change often I can stringify as a key:

exportdefault(props)=>(<ConnectedComponentkey={props.userID+'_'+props.anotherSparselyChangedProp}/>);

but that .. is beyond ugly. It does bother me there are so many different steps within different levels involved in simply invalidating and updating a component, but was the only way I could make this work.

nice trick@joonhyublee :D

Maybe this usecase will be easier to handle once the code of connect becomes much simpler, and it might with#368