Duplicate event handlers bound on every render

mattsbennett commented 2 months ago

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

Go to https://stackblitz.com/edit/react-xfumjc?file=package.json,src%2FApp.js,src%2FApp.js
Open the preview pane console
Click any of the data points
With each subsequent click, and additional event is emitted
Comment out setStateData(params.value) in onChartClick
Click data points again
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 commented 2 months ago

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

mattsbennett commented 2 months ago

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 commented 2 months ago

@mattsbennett this issue is fixed in v1.3.0.

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

mattsbennett commented 2 months ago

@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 commented 2 months ago

@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 commented 2 months ago

@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 :-)

hugocxl / react-echarts