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

[Performance] Pure view (shallow render) #11

Closed
minedeljkovic opened this issue May 4, 2016 · 11 comments
Closed

[Performance] Pure view (shallow render) #11

minedeljkovic opened this issue May 4, 2016 · 11 comments
Milestone

Comments

@minedeljkovic
Copy link

minedeljkovic commented May 4, 2016

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
Copy link

namjul commented May 4, 2016

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

@minedeljkovic
Copy link
Author

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
Copy link
Collaborator

tomkis commented May 4, 2016

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
Copy link
Author

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
Copy link

namjul commented May 4, 2016

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

@minedeljkovic
Copy link
Author

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

@namjul
Copy link

namjul commented May 4, 2016

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

@minedeljkovic
Copy link
Author

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
Copy link

namjul commented May 4, 2016

oh right .. missed that one :)

@tomkis tomkis added this to the 1.4.0 milestone May 5, 2016
@namjul
Copy link

namjul commented May 9, 2016

@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
Copy link
Collaborator

tomkis commented May 13, 2016

Solved in #14

@tomkis tomkis closed this as completed May 13, 2016
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

3 participants