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.

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

charts-svelte: cannot read property 'removeChild' of null when using <svelte:component /> #799

Closed
1 of 5 tasks
danielboven opened this issue Sep 10, 2020 · 4 comments · Fixed by #800
Closed
1 of 5 tasks

Comments

@danielboven
Copy link

danielboven commented Sep 10, 2020

I am submitting a...

  • Feature request
  • Design defect
  • Source code defect
  • Demo/documentation defect
  • Other

charts version:

"@carbon/charts": "^0.36.2"
"@carbon/charts-svelte": "^0.36.2"

Issue description

With the Svelte version of Carbon charts everything seems to work as expected, except when using the special element <svelte:component this={componentName}>. The componentName variable refers to a reactive variable pointing to, for example, a PieChart, DonutChart, BarChartSimple import. When changing this variable I get an error:
Uncaught (in promise) TypeError: Cannot read property 'removeChild' of null

Here is (a simplified version of) my code:

<script>
  import { BarChartSimple, PieChart } from "@carbon/charts-svelte";

  const data = [...];

  let type = PieChart;

  // change chart type to different import after 5 seconds
  setTimeout(() => (type = BarChartSimple), 5000);
</script>

<svelte:component
  this={type}
  {data}
/>

Steps to produce the issue

Using <svelte:component this={ChartType} /> with a reactive value for ChartType.

I have also put the code above in a CodeSandbox, to reproduce this error. I'm getting a very similar error to the one I'm getting in my own project, probably a little different due to the nature and workings of CodeSandbox (compared to working with the Svelte template and Rollup):
Uncaught (in promise) TypeError: Cannot read property 'c' of undefined

Current behavior

When mounting the component and the chart everything works, untill changing the variable used in the svelte:component at this. In the console an error is thrown and the new chart type doesn't render, other parts of the page (component) also don't respond.

Expected behavior

When you change the variable connected to this at svelte:component, the new chart should render and replace the previous one, just like a component replaces another in the tutorial example.

Screenshot or recording

chartOnDestroyError

Possible fix to investigate

One of the reasons for this error seems to be relating to third party libraries interfering with Svelte's own DOM manipulation (see sveltejs/svelte#5038 (comment)). Removing the chart.destroy() call at the BaseChart.svelte file (line 33) has solved this issue for me. I'm not sure whether this call is necessary for the @carbon/charts package to function properly, or whether Svelte is able to handle destroying everything related to the component.
Without removing this call I have not been able to get the <svelte:component /> with a changing variable to work, as described in the Svelte tutorial.

@theiliad
Copy link
Member

@metonym could you please chime in here?

@metonym
Copy link
Contributor

metonym commented Sep 10, 2020

@danielboven Thank you for raising the issue and the detailed reproduction steps.

I was able to reproduce the error on my local machine with Rollup and the latest svelte, @carbon/charts-svelte versions.

You're absolutely right when you mention #5038, where the "Cannot read property 'removeChild' of null" occurs because of external/non-managed DOM manipulations . That issue has bitten me several times.

However, I think the underlying issue is that the chart instance in BaseChart is not re-instantiated when the Chart prop itself changes. In my local set-up, I was able to swap the charts using your example (although I still get the removeChild error).

$: if (element && Chart) {
    chart = new Chart(element, { data, options });
    dispatch("load", chart);
}

I'll need to dig deeper into this, but BaseChart probably requires significant adjustments.

@metonym
Copy link
Contributor

metonym commented Sep 11, 2020

Following up on this after drafting a PR (#800)


@danielboven correctly identifies chart.destroy() as the root cause of the "removeChild" error. The .destroy() method in chart.ts interacts with the DOM by removing the chart holder.

Instead of invoking the destroy method, the solution is to:

  • cherry pick behavior from the original method to ensure chart components are destroyed
  • set chart to null for garbage collection when the Svelte component is destroyed
- chart.destroy();
+ chart.components.forEach((component) => component.destroy());
+ chart = null;

@danielboven
Copy link
Author

The solution proposed by @metonym in #800 fixes the issue as described above. Thanks for looking into this!

changing graph

By the way: is it practice to keep this issue open until the PR is merged or could this issue be closed now?

theiliad pushed a commit that referenced this issue Sep 14, 2020
* fix(svelte): rewrite BaseChart to reload chart instance

Fixes #799

* refactor(svelte): remove unnecessary afterUpdate method

* chore(svelte): keep the PR scoped
theiliad pushed a commit to theiliad/carbon-charts that referenced this issue May 17, 2021
…ase-chart

* fix(svelte): rewrite BaseChart to reload chart instance

Fixes carbon-design-system#799

* refactor(svelte): remove unnecessary afterUpdate method

* chore(svelte): keep the PR scoped
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants