react-calendar icon indicating copy to clipboard operation
react-calendar copied to clipboard

Don't call callbacks from this.setState() to avoid double rendering

Open eldargab opened this issue 4 years ago • 2 comments

Hello, I noticed that react-calendar notifies parent component about various events via .setState() callback.

E.g.

https://github.com/wojtekmaj/react-calendar/blob/be7c443fbe39ce90036380dab727c1ca7072a073/src/Calendar.jsx#L310

The problem is that.setState() callback is called after react finishes real DOM updates. That means, changes from parent component will cause another update cycle. Also parent component might remove calendar in response to event entirely making previous update completely useless.

You can verify such behaviour via DevTools profiler

image

In the screenshot above all the work before Lifecycle hook scheduled a cascading update is completely useless and causes substantial delay.

Another reason not to use .setState() for parent callbacks is to avoid crashing entire app in case parent event handler erroneously raises exception.

eldargab avatar Dec 23 '19 18:12 eldargab

Hmmm, if we moved this to the beginning of the function then we would potentially start two asynchronous updates - one via onChange & props, one internal via state. That would be even worse, I think?

wojtekmaj avatar Dec 31 '19 14:12 wojtekmaj

No, there would be only one update incorporating both props and state changes.

This is what currently happens with react-calendar/sample/parcel app.

image

On other hand, immediate invocation of onChange callback yields the following picture

image

PR #302 illustrates proposed changes.

eldargab avatar Jan 02 '20 09:01 eldargab