-
Notifications
You must be signed in to change notification settings - Fork 98
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
Fixes #27292 - workaround for c3 destroy issue #435
Fixes #27292 - workaround for c3 destroy issue #435
Conversation
There is a known issue in react-c3js that can lead to the charts to disappear due to some race condition during component unmounting/destroy phase bcbcarl/react-c3js#22. Delaying the actual destroy a bit seems to help with workarounding the issue.
@sharvit it took me a while to find what was wrong. I can't think if any suitable workaround as this one, but if you had some thoughts, please feel free to experiment with this. |
Fixes the issue for me |
// to issue described in https://github.com/bcbcarl/react-c3js/issues/22. | ||
// Delaying the destroy a bit seems to resolve the issue. | ||
setTimeout(this.chart.destroy, 1000); | ||
this.chart = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you pass functions this way in javascript, you loose the this
reference. So if the destroy
function would use the this
keywork it will receive something different than the chart.
Therefore I want to suggest using:
setTimeout(() => {
this.chart.destroy();
this.chart = null;
}, 1000);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notice that we're actually not adding the this.chart = null
within timeout, which is on purpose (so that we make sure this component is not touching it after that point). I don't think we care about this
binding within the timeout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this.chart = null
will actually mean something because the component itself is already destroyed.
The this
binding work because of the implementation of chart.destroy
don't use this
. Once they will this code will break.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. Actually, the destroy
function is using this
already and it works thanks to the fact that c3
is binding the API functions explicitly https://github.com/c3js/c3/blob/062fc2a93915b94b36daa1e168334109f5edcf20/src/chart.js#L17-L24
On the other hand, I don't know how the component behaves after it's unmounted (and would be referenced later in after timeout). Therefore, I would probably just keep this is it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added an explicit comment though the mention that it's ok to do it this way in this case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @iNecas, the workaround is reasonable, LGTM.
please consider my suggestion about the this
keyword.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
APJ
Test failures unrelated |
* Fixes #27292 - workaround for c3 destroy issue There is a known issue in react-c3js that can lead to the charts to disappear due to some race condition during component unmounting/destroy phase bcbcarl/react-c3js#22. Delaying the actual destroy a bit seems to help with workarounding the issue. * Refs #27292 - Add note about no need for explicit bind
* Fixes #27292 - workaround for c3 destroy issue There is a known issue in react-c3js that can lead to the charts to disappear due to some race condition during component unmounting/destroy phase bcbcarl/react-c3js#22. Delaying the actual destroy a bit seems to help with workarounding the issue. * Refs #27292 - Add note about no need for explicit bind
There is a known issue in react-c3js that can lead to the charts to
disappear due to some race condition during component unmounting/destroy
phase bcbcarl/react-c3js#22, with this error procuder
in the console log:
Delaying the actual destroy a bit seems to help with workarounding the
issue.