Clean up event listeners from previous renders (#618)

* Add second test series

This makes it possible to dispatch legend events for testing.

* Add test case

This verifies the bug by adding a failing test case. On re-render there
are two event listeners attached to the chart instance, there should be
only one!

* Correctly unbind the current event handlers

Keep track of the handlers added to the current instance. Unbind them
before binding new events.

* Rename onEvents to eventHandlerRefs for clarity

* Bump version from 3.0.5 to 3.0.6

---------

Co-authored-by: hustcc <i@hust.cc>

Author: gggritso

__tests__/charts/simple-spec.tsx

@@ -4,6 +4,9 @@ import type { EChartsOption } from '../../src/';
 import { render, destroy, createDiv, removeDom } from '../utils';
 
 const options: EChartsOption = {
+  legend: {
+    data: ['series1', 'series2'],
+  },
   xAxis: {
     type: 'category',
     data: ['Mon', 'Tue', 'Wed', 'Thu', 'Fri', 'Sat', 'Sun'],
@@ -13,10 +16,17 @@ const options: EChartsOption = {
   },
   series: [
     {
+      name: 'series1',
       data: [820, 932, 901, 934, 1290, 1330, 1320],
       type: 'line',
       smooth: true,
     },
+    {
+      name: 'series2',
+      data: [720, 832, 801, 834, 1190, 1230, 1220],
+      type: 'line',
+      smooth: true,
+    },
   ],
 };
 
@@ -216,4 +226,57 @@ describe('chart', () => {
       removeDom(div);
     });
   });
+
+  it('updates event handler on re-render', (done) => {
+    let instance;
+    const div = createDiv();
+
+    const handlerA = jest.fn();
+    const handlerB = jest.fn();
+
+    // First render, with first legend handler
+    const Comp1 = (
+      <ReactECharts
+        ref={(e) => (instance = e)}
+        option={options}
+        onEvents={{
+          legendselectchanged: handlerA,
+        }}
+        onChartReady={(chart) => {
+          // @ts-ignore - accessing internal property for testing
+          const handlers = chart._$handlers?.legendselectchanged;
+
+          expect(handlers).toBeDefined();
+          expect(handlers.length).toBe(1);
+
+          // Re-render with second legend handler
+          const Comp2 = (
+            <ReactECharts
+              ref={(e) => (instance = e)}
+              option={options}
+              onEvents={{
+                legendselectchanged: handlerB,
+              }}
+            />
+          );
+          render(Comp2, div);
+
+          // Wait a tick for re-render to complete
+          setTimeout(() => {
+            const updatedChart = instance.getEchartsInstance();
+            // @ts-ignore - accessing internal property for testing
+            const handlersAfterReRender = updatedChart._$handlers?.legendselectchanged;
+            expect(handlersAfterReRender).toBeDefined();
+            expect(handlersAfterReRender.length).toBe(1);
+
+            destroy(div);
+            removeDom(div);
+            done();
+          }, 0);
+        }}
+      />
+    );
+
+    render(Comp1, div);
+  });
 });

package.json

@@ -1,6 +1,6 @@
 {
   "name": "echarts-for-react",
-  "version": "3.0.5",
+  "version": "3.0.6",
   "description": " Apache Echarts components for React.",
   "main": "lib/index.js",
   "module": "esm/index.js",

src/core.tsx

@@ -26,12 +26,18 @@ export default class EChartsReactCore extends PureComponent<EChartsReactProps> {
    */
   protected echarts: any;
 
+  /**
+   * Currently attached ECharts event listeners
+   */
+  private eventHandlerRefs: Record<string, Function>;
+
   constructor(props: EChartsReactProps) {
     super(props);
 
     this.echarts = props.echarts;
     this.ele = null;
     this.isInitialResize = true;
+    this.eventHandlerRefs = {};
   }
 
   componentDidMount() {
@@ -62,7 +68,7 @@ export default class EChartsReactCore extends PureComponent<EChartsReactProps> {
     // 修改 onEvent 的时候先移除历史事件再添加
     const echartsInstance = this.getEchartsInstance();
     if (!isEqual(prevProps.onEvents, this.props.onEvents)) {
-      this.offEvents(echartsInstance, prevProps.onEvents);
+      this.unbindEvents(echartsInstance);
       this.bindEvents(echartsInstance, this.props.onEvents);
     }
 
@@ -163,15 +169,21 @@ export default class EChartsReactCore extends PureComponent<EChartsReactProps> {
 
   // bind the events
   private bindEvents(instance, events: EChartsReactProps['onEvents']) {
-    function _bindEvent(eventName: string, func: Function) {
+    const _bindEvent = (eventName: string, func: Function) => {
       // ignore the event config which not satisfy
       if (isString(eventName) && isFunction(func)) {
         // binding event
-        instance.on(eventName, (param) => {
+        const handler = (param) => {
           func(param, instance);
-        });
+        };
+
+        instance.on(eventName, handler);
+
+        // Store currently bound event handlers. This way we can unbind them
+        // on next component update, before binding the new handlers.
+        this.eventHandlerRefs[eventName] = handler;
       }
-    }
+    };
 
     // loop and bind
     for (const eventName in events) {
@@ -181,15 +193,16 @@ export default class EChartsReactCore extends PureComponent<EChartsReactProps> {
     }
   }
 
-  // off the events
-  private offEvents(instance, events: EChartsReactProps['onEvents']) {
-    if (!events) return;
-    // loop and off
-    for (const eventName in events) {
-      if (isString(eventName)) {
-        instance.off(eventName, events[eventName]);
-      }
+  /**
+   * Unbind all currently bound event handlers. Importantly, this does not
+   * unbind the `"finished"` event that is used for chart initialization.
+   */
+  private unbindEvents(instance: EChartsInstance) {
+    for (const [eventName, listener] of Object.entries(this.eventHandlerRefs)) {
+      instance.off(eventName, listener);
     }
+
+    this.eventHandlerRefs = {};
   }
 
   /**