Performance concerns related to translate()

[satazor]

Hello!

I've been using the translate() enhancer for quite some time and, after some experiments, I concluded that all components using translate are being re-rendered after any dispatch(action).

All my components extend PureComponent which goes well with redux because it avoids re-rendering components unless props have changed. The issue here is that the p object changes every time because of this line: https://github.com/Tiqa/redux-polyglot/blob/master/src/selectors.js#L52. A new object is being created in mapStateToProps function which causes p ref to change and, consequently, causes all translated components to re-render.

The user-land solution here is to implement a custom shouldComponentUpdate function that does a shallow compare for all properties except for p, which needs to be deeply compared. While it works, it feels awkward to delegate that responsibility to devs.

Am I missing something?

[guillaumearm]

Hi ! you're right ;-) But we probably should use memoization on 'getP' using 'createSelector' and composing with 'getLocale' and 'getPhrases'. so object returned will be the same until locale or phrases changes. thanks for reporting :) 2017-01-22 1:28 GMT+01:00 André Cruz <notifications@github.com>:

…

Hello! I've been using the translate() enhancer for quite some time and, after some experiments, I concluded that all components using translate are being re-rendered after any dispatch(action). All my components extend PureComponent which goes well with redux because it avoids re-rendering components unless props have changed. The issue here is that the p object changes every time because of this line: https://github.com/Tiqa/redux-polyglot/blob/master/src/selectors.js#L52. A new object is being created in mapStateToProps function which causes p ref to change which makes all translated components to re-render. The obvious solution here is to implement a custom shouldComponentUpdate function that does a shallow compare for all properties, except for p which needs to be deeply compared. While it works, it feels awkward to delegate that responsibility to devs. Am I missing something? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#71>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AQHWeqaFnnuJ8NUKy1MjpUcQMEqz-ec7ks5rUqKpgaJpZM4LqOoF> .

-- Guillaume ARM +33 6 71 50 10 11 www.redpelicans.com

[satazor]

@guillaumearm sounds good! I'm getting huge perf issues on inputs, where a dispatch is being issued on every keypress. Do you have some free time to solve this issue? I can step in but I guess it would take much more time to do it.

[guillaumearm]

ok i will publish a new release candidate tonight about this ;-)

[guillaumearm]

I just published a patch on next npm dist-tag.

try it with a npm install redux-polyglot@next
and tell me if it's better ;-)

[satazor]

@guillaumearm I've tested the next dist tag and I see no difference. :(

[guillaumearm]

hum too bad.
I will write more tests about react performance soon, and find a way to solve that.
stay tuned ;-)

[guillaumearm]

@satazor,
here: https://github.com/Tiqa/redux-polyglot/blob/optimize/getP/src/selectors.js#L69
I think it's because second parameter getP object is recreated all the time.

I will try to use a memoized merge object to fix this.
what do you think ?

another solution would be to use createSelectorCreator from reselect to create a custom createSelector.

[guillaumearm]

done here : d69a9af
I just published the 0.4.2-1 version on nextnpm dist-tag ;-)

Try it, and tell me if it's better ;-)
Can you provide me your tests ? if you got some.

[satazor]

I'm testing on a proprietary project but it's quite simple to test, just export a translated pure component and do a console.log inside render. You will see the print everytime you dispatch something (non related to polyglot).

Thanks for this, will test in a few hours.

[satazor]

@guillaumearm sorry for the delay. The behavior is still the same, will try to provide a fix.

[satazor]

@guillaumearm I've added a test case in #75, will investigate a fix. Want to do this together? :D

[guillaumearm]

Yup, sure ^^
But not tonight, I'm busy
anyway, thank you very much for your help :)

[guillaumearm]

found it ! just need to memoize all getTranslation* functions.
I will publish a new release soon ^^

[guillaumearm]

Merged, it is seems to be OK.

@satazor you can test it on next npm dist-tag ;-)

[satazor]

I'm currently on my phone, but will test asap.

[satazor]

I confirm it's working! Thanks for fixing this!