nuclear-js-react-addons icon indicating copy to clipboard operation
nuclear-js-react-addons copied to clipboard

[WIP] Call setState on React components once per dispatch

Open iansinnott opened this issue 9 years ago • 3 comments

This PR is incomplete and open for discussion. I updated connect to only call setState on the wrapped component once per dispatch. I haven't yet touched nuclearMixin as I wanted to get feedback first and see if this is a change you would be interested in, but I'd be happy to update that as well.

This relates directly to https://github.com/optimizely/nuclear-js/pull/195 and #13. Open to any feedback or suggestions.

iansinnott avatar Aug 05 '16 00:08 iansinnott

Also relates to https://github.com/optimizely/nuclear-js/issues/193

iansinnott avatar Aug 05 '16 00:08 iansinnott

Hmm, your approach seems reasonable, however we are trading evaluating all getters bound to a component vs only triggering getters that have changed and potentially calling re-render more than once.

So the question comes down to whether its more expensive to evaluate extraneous getters or call render multiple times, and I'm not quite convinced one is always better than the other.

It'd be nice to allow this to be configured somehow when using the decorator or have multiple decorators.

Thoughts?

jordangarcia avatar Aug 10 '16 18:08 jordangarcia

Very good point. My initial thought is that there is probably a better means of doing aggregation than creating a new getter that will be recomputed when any of the bound getters change.

I'm not yet sure what that would look like. For now i'm happy to simply use my own fork. In my codebase having aggregated state updates proved very helpful, especially with components that implement componentDidUpdate since intuitively it should only be called once with all changes applied.

I agree that the perf tradeoff is likely a deal breaker for some.

iansinnott avatar Aug 15 '16 23:08 iansinnott