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.

Sign up for GitHub

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

Jump to bottom

Fixes #27292 - workaround for c3 destroy issue #435

Merged
merged 2 commits into from
Jul 15, 2019
Merged

Fixes #27292 - workaround for c3 destroy issue #435

merged 2 commits into from
Jul 15, 2019

Conversation

iNecas
Copy link
Member

@iNecas iNecas commented Jul 12, 2019

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:

c3.js:9223 Uncaught TypeError: Cannot read property 'data_types' of null
    at ChartInternal.c3_chart_internal_fn.isPieType (c3.js:9223)
    at ChartInternal.c3_chart_internal_fn.isArcType (c3.js:9234)
    at ChartInternal.c3_chart_internal_fn.getArc (c3.js:4473)
    at c3.js:4816
    at SVGPathElement.<anonymous> (d3.js:8775)
    at Object.tick [as c] (d3.js:8956)
    at d3_timer_mark (d3.js:2166)
    at d3_timer_step (d3.js:2147)

Delaying the actual destroy a bit seems to help with workarounding the
issue.

@iNecas
Fixes #27292 - workaround for c3 destroy issue
253162e 
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.
@iNecas
Copy link
Member Author

iNecas commented Jul 12, 2019

@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.

@adamruzicka
Copy link
Contributor

Fixes the issue for me

sharvit
sharvit reviewed Jul 15, 2019
View reviewed changes
webpack/ForemanTasks/Components/Chart/Chart.js
// 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;
Copy link
Contributor

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);

Copy link
Member Author

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

Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Member Author

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

sharvit
sharvit approved these changes Jul 15, 2019
View reviewed changes
Copy link
Contributor

@sharvit sharvit left a 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.

sharvit
sharvit approved these changes Jul 15, 2019
View reviewed changes
Copy link
Contributor

@sharvit sharvit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

adamruzicka
adamruzicka approved these changes Jul 15, 2019
View reviewed changes
Copy link
Contributor

@adamruzicka adamruzicka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APJ

@iNecas
Copy link
Member Author

iNecas commented Jul 15, 2019

Test failures unrelated

@adamruzicka adamruzicka merged commit 709bc87 into theforeman:master Jul 15, 2019
@adamruzicka
Copy link
Contributor

Thanks @iNecas , @sharvit !

adamruzicka pushed a commit to adamruzicka/foreman-tasks that referenced this pull request Jul 16, 2019
@iNecas @adamruzicka
Fixes #27292 - workaround for c3 destroy issue (theforeman#435)
e68ae2e 
* 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
iNecas added a commit that referenced this pull request Jul 16, 2019
@iNecas
Fixes #27292 - workaround for c3 destroy issue (#435)
d96b627 
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adamruzicka adamruzicka adamruzicka approved these changes

@sharvit sharvit sharvit approved these changes

Assignees
No one assigned
Labels
Needs testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@iNecas @adamruzicka @sharvit @theforeman-bot