salsita / prism

React / Redux action composition made simple http://salsita.github.io/prism/
496 stars 24 forks source link

[Performance] Pure view (shallow render) #11

Closed minedeljkovic closed 8 years ago

minedeljkovic commented 8 years ago

In gif-viewers-pair example, it should be possible to make random-gif-viewer component pure, so it should rerender only when its model change. For example, when 'Request more' is clicked on 'funny cats' topic and new gif is received, component for 'funny dogs' topic should not be rerendered.

This should be easily achievable with something like recompose pure helper.

But, the problem is that forwardTo calls propagate new instances of dispatch down the component hierarchy.

I made proof of concept implementation of gif-viewers-pair example that tries to solve this problem. Here is higher order component views should use: higher order component that enables shallow render Here is example of usage.

Do you think solution to this problem (similar to the one I gave in this quick implementation, or something different) should be also part of redux-elm architecture?

namjul commented 8 years ago

you could also use https://github.com/acdlite/recompose/blob/master/docs/API.md#onlyupdateforkeys and just not include dispatcher.

minedeljkovic commented 8 years ago

Thanks, @namjul ! I didn't check their API in enough detail. Would onlyUpdateForKeys() also ensure that call to dispatch from wrapped component will be properly delegated to latest instance, despite wrapped component not being rendered with it? Also general behavior for redux-elm views would be to update for all keys except dispatch but that can probably be achieved by some function from recompose api.

But, final question still remains. Since redux-elm is opinionated with usage of dispatch (it comes with forwardTo) and immutability of model, shouldn't this be also included out of box?

tomkis commented 8 years ago

Thanks @minedeljkovic and @namjul

I'd agree that this should definitely be part of the framework. IMO tagged dispatch should be memoized, so I'd just change implementation of forwardTo.

minedeljkovic commented 8 years ago

Great! Looking forward to see that implementation. I must confess I tried that approach, but all I came up with was pretty cumbersome to use in components like gif-viewers-pair and gif-viewers-dynamic-list. Hope it will not be that way. :)

namjul commented 8 years ago

@minedeljkovic i think it will just use the instance of dispatch it got initially.

minedeljkovic commented 8 years ago

Thanks, @namjul . That would not be good for this case.

namjul commented 8 years ago

so in your case your forwardTo function isn't static and it can change based on some logic.

minedeljkovic commented 8 years ago

Yes, I think that is generally the case in redux-elm. Take for example gif-viewers-dynamic-list. Components in dynamic list could be reordered so their dispatch path would change.

namjul commented 8 years ago

oh right .. missed that one :)

namjul commented 8 years ago

@minedeljkovic FYI actually "onlyUpdateForKeys" should also return the latest dispatch function. So whenever shouldUpdate returns true, the current dispatch function is used. https://github.com/acdlite/recompose/blob/master/src/packages/recompose/onlyUpdateForKeys.js

tomkis commented 8 years ago

Solved in #14