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

The chart should be destroyed on unmount #34

Open
archfz opened this issue Feb 19, 2022 · 4 comments
Open

The chart should be destroyed on unmount #34

archfz opened this issue Feb 19, 2022 · 4 comments

Comments

@archfz
Copy link

archfz commented Feb 19, 2022

Uncaught DOMException: Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node. Related #20

diff --git a/node_modules/react-frappe-charts/build/index.js b/node_modules/react-frappe-charts/build/index.js
index 6b82382..0df800e 100644
--- a/node_modules/react-frappe-charts/build/index.js
+++ b/node_modules/react-frappe-charts/build/index.js
@@ -20,6 +20,7 @@ const ReactFrappeChart = forwardRef((props, parentRef) => {
                 onDataSelect(e);
             });
         }
+        return () => chart.current.destroy();
     }, []);
     React.useEffect(() => {
         if (initialRender.current) {
@archfz
Copy link
Author

archfz commented Feb 19, 2022

Unfortunately this still reproduces sometimes even with this patch, but considerably lower chance.

@dpnick
Copy link

dpnick commented Mar 14, 2022

I can confirm I have the same issue on each re-render I get the "Failed to execute 'removeChild' on 'Node'" error. Any news about it?

@archfz
Copy link
Author

archfz commented Apr 7, 2022

This has caused me a lot of headaches. The react-frappe-charts is not of blame here, but frappe-charts. It has a really bad architecture behind it, and quite bug prone with hard to understand bugs. Here is the workaround for now.

Patch react-frappe-charts

diff --git a/node_modules/react-frappe-charts/build/index.js b/node_modules/react-frappe-charts/build/index.js
index 6b82382..cf13bab 100644
--- a/node_modules/react-frappe-charts/build/index.js
+++ b/node_modules/react-frappe-charts/build/index.js
@@ -1,5 +1,5 @@
 import React, { useImperativeHandle, forwardRef } from "react";
-import { Chart } from "frappe-charts/dist/frappe-charts.min.esm";
+import { Chart } from "frappe-charts/dist/frappe-charts.esm";
 const ReactFrappeChart = forwardRef((props, parentRef) => {
     const ref = React.useRef(null);
     const chart = React.useRef(null);
@@ -12,7 +12,7 @@ const ReactFrappeChart = forwardRef((props, parentRef) => {
             }
         },
     }));
-    React.useEffect(() => {
+    React.useLayoutEffect(() => {
         chart.current = new Chart(ref.current, Object.assign({ isNavigable: onDataSelect !== undefined }, props));
         if (onDataSelect) {
             chart.current.parent.addEventListener("data-select", (e) => {
@@ -20,6 +20,7 @@ const ReactFrappeChart = forwardRef((props, parentRef) => {
                 onDataSelect(e);
             });
         }
+        return () => chart.current.destroy();
     }, []);
     React.useEffect(() => {
         if (initialRender.current) {

Patch frappe-charts

diff --git a/node_modules/frappe-charts/dist/frappe-charts.esm.js b/node_modules/frappe-charts/dist/frappe-charts.esm.js
index 80b7e3c..2c1f76d 100644
--- a/node_modules/frappe-charts/dist/frappe-charts.esm.js
+++ b/node_modules/frappe-charts/dist/frappe-charts.esm.js
@@ -1625,7 +1625,7 @@ class BaseChart {
 		this.height = height - getExtraHeight(this.measures);
 
 		// Bind window events
-		this.boundDrawFn = () => this.draw(true);
+		this.boundDrawFn = () => window.requestAnimationFrame(() => this.draw(true));
 		if (ResizeObserver) {
 			this.resizeObserver = new ResizeObserver(this.boundDrawFn);
 			this.resizeObserver.observe(this.parent);
@@ -1709,7 +1709,11 @@ class BaseChart {
 
 	makeChartArea() {
 		if(this.svg) {
-			this.container.removeChild(this.svg);
+			try {
+				this.svg.parentNode.removeChild(this.svg);
+			} catch (e) {
+				console.warn(e);
+			}
 		}
 		let m = this.measures;
 
@@ -1754,6 +1758,10 @@ class BaseChart {
 		if(this.config.showLegend) { this.svg.appendChild(this.legendArea); }
 
 		this.updateTipOffset(getLeftOffset(m), getTopOffset(m));
+		
+		if (this.container.getElementsByTagName('svg').length > 1) {
+			this.container.removeChild(this.container.getElementsByTagName('svg')[0]);
+		}
 	}
 
 	updateTipOffset(x, y) {

I don't understand why the things happen, but this resolves it, after trial and error.

@leephan2k1
Copy link

I have a same problem. Maybe i should pick another lib for the chart.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants