-
-
Notifications
You must be signed in to change notification settings - Fork 502
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
Don't call callbacks from this.setState() to avoid double rendering #298
Comments
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? |
No, there would be only one update incorporating both props and state changes. This is what currently happens with react-calendar/sample/parcel app. On other hand, immediate invocation of PR #302 illustrates proposed changes. |
Hello, I noticed that react-calendar notifies parent component about various events via
.setState()
callback.E.g.
react-calendar/src/Calendar.jsx
Line 310 in be7c443
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
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.The text was updated successfully, but these errors were encountered: