Duplicate event handlers bound on every render

[mattsbennett]

Description

When event handlers contain react useState setters, duplicate event handlers are bound. The cause:

    if (onClick) {
      echartsInstance.on(echartsEvents.onClick, onClick);
    }

This results in event handlers being duplicated (an additional duplicate handler is bound every time the component is rendered). A simple solution would be the following:

    if (onClick) {
      echartsInstance.off(echartsEvents.onClick);
      echartsInstance.on(echartsEvents.onClick, onClick);
    }

Link to Reproduction

https://stackblitz.com/edit/react-xfumjc?file=package.json,src%2FApp.js,src%2FApp.js

Steps to reproduce

1. Go to https://stackblitz.com/edit/react-xfumjc?file=package.json,src%2FApp.js,src%2FApp.js
2. Open the preview pane console
3. Click any of the data points
4. With each subsequent click, and additional event is emitted
5. Comment out setStateData(params.value) in onChartClick
6. Click data points again
7. Now only a single event is emitted

JS Framework

React (JS)

Version

1.2.1

Browser

No response

Operating System

• macOS
• Windows
• Linux

Additional Information

No response

[hugocxl]

Hi @mattsbennett, great finding! Let me find some time to look into the issue.

[mattsbennett]

Thanks @hugocxl ! I think there's an additional bug here; on clicking any of the points (with the useState setter in the event handler), the chart zoom resets, which seems unexpected.

I worked around this by making x and y zoom start/end values stateful as well, storing the values in state and updating them via onDataZoom (and onRestore). Maybe that's the intent of the design? Not sure, though I found it unexpected.

[hugocxl]

@mattsbennett this issue is fixed in v1.3.0.

Could you check whether the last commented issue is fixed in this version? Thanks!

[mattsbennett]

@hugocxl I've updated my stackblitz example (https://stackblitz.com/edit/react-xfumjc?file=package.json,src%2FApp.js,src%2FApp.js) to 1.3.0, with no other changes and now it seems the chart doesn't load/render at all.

[hugocxl]

@mattsbennett absolutely. My bad, I based my previous solution in the Suspense API not taking into account that there might be projects where is not being used. I have reverted this change and it's working in your example properly with v1.3.1. Sorry for the incoveniences

[mattsbennett]

@hugocxl no problem, thank you! 1.3.1 resolves the render issue; however I've noticed one additional regression - now the charts don't dynamically resize on viewport width changes (though they still layout responsively on initial load). I've updated my stackblitz example to 1.3.1 (https://stackblitz.com/edit/react-xfumjc?file=package.json,src%2FApp.js,src%2FApp.js). To reproduce:

• Observe on resizing the stackblitz preview pane, the chart doesn't dynamically resize with the preview window.
• Now use the trash can in the dependences pane to remove version 1.3.1
• Install 1.2.1 again by entering @kbox-labs/react-echarts@1.2.1 in the "Enter package name" field
• After reloading the preview, the chart dynamically resizes on preview window width change

Edit: Added a new issue for this :-)