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

How should i update event (selection) function? #112

Closed
890f2151c2be69c51db72017546d00fd opened this issue Apr 11, 2019 · 13 comments
Closed

Comments

@890f2151c2be69c51db72017546d00fd

Hi,
when i get a new props, I pass new options to Highcharts. Chart is correctly updated (title, series etc.). Except of events functions. for example selection callback function is not recreated (updated). how should I update events functions?
chart: { events: { selection: (e) => { ... references to old const here }, },

@ppotaczek
Copy link
Contributor

ppotaczek commented Apr 11, 2019

Hello,

Thank you for contacting us!

This problem is a Highcharts bug, that is reported here: highcharts/highcharts#6538

You can use one of the suggested workarounds, for example addEvent and removeEvent methods.

API Reference:
https://api.highcharts.com/class-reference/Highcharts#.addEvent
https://api.highcharts.com/class-reference/Highcharts#.removeEvent

Best regards!

@bkniffler
Copy link

I think this issue should not be closed. As a react dev, I expected events to be rebound to new configs and old event handlers being removed automatically. I now found out that each time the config is mutated, all events will just pile up until they lead to weird bugs, which is the point where I found out of this issue.

At least you could detect if events are present in config and output a console warning about this. Better yet you could automatically cleanup events on each update.

@ppotaczek
Copy link
Contributor

Hi @bkniffler,

Thank you for your suggestion. I didn't want to duplicate threads, so I closed this topic. The problem is caused by the Highcharts code, not by the wrapper.

The chart.update method, which we use in this wrapper works in the same way with and without React.

Best regards!

@bkniffler
Copy link

I get that, but the issue has been open for quite a long time and should, in my opinion, at this point be mitigated within the react wrapper; or at least pointed out. Its not like this is about tons of changes. This is my current solution btw

const options = React.useMemo<Highcharts.Options>(() => {
    if (ref.current) {
      const { chart } = ref.current;
      Highcharts.removeEvent(chart.xAxis[0], 'afterSetExtremes');
    }
    return {
      xAxis: {
        events: {
          afterSetExtremes: function(e) {
            console.log(e)
          }
        }
      } ....,
    };
  }, [data]);

@ppotaczek
Copy link
Contributor

@bkniffler - I understand, but I would not like to add any workarounds to the wrapper code. There are many issues for which we could add a workaround, but then the code would get more and more extensive.

We have other wrappers like highcharts-vue or highcharts-angular that work very similar to highcharts-react-official, so to maintain uniformity this problem should be definitely solved on Highcharts level.

@bkniffler
Copy link

bkniffler commented Oct 28, 2019

I understand, though the very declarative nature of react makes this issue especially painful. On angular you would declare your events only once anyways within the controller, in react there is just no way around rebinding events each time your data changes. I'd really say this is a huge issue for the react wrapper especially, that does not apply to other wrappers in that extend. Which is maybe the reason why the issue wasn't resolved yet within highcharts, for imperative JS programming its just not a huge issue.

Thats the whole point of hooks like React.useEffect, to, for example, bind your events and cleanup on changes so no ghost events remain. I'd argue that this could and should be done inside the wrapper, which is probably not necesssarily the case for the other issues that you were mentioning.

@ppotaczek
Copy link
Contributor

Thanks for the explanation, but I still don't quite agree with you. The problem occurs only when you want to update events. Have you checked the information about optimal-way-to-update? You can update only the required part of a configuration object.

There is also the immutable option, which can be used to solve the problem.

@bkniffler
Copy link

Allright, I'll implement a save way within the app using the update mechanism. Thanks for the hints.

@annitya
Copy link

annitya commented May 4, 2021

Events piling up is a major pain point for us. They will add up to thousands after a short time causing terrible performance. Having a react-wrapper which causes these sorts of issues is worse than having none at all.

@ppotaczek
Copy link
Contributor

Hi @annitya,

You probably unnecessary update those events. As I mentioned above, you can update only the changed part of a configuration object.

Best regards!

@annitya
Copy link

annitya commented May 5, 2021

Thanks for your quick reploy @ppotaczek. The react-component containing the wrapper is rendered multiple times during it's lifetime, but that is to be expected. The events piling up aren't even created by me:

image

The legendItemClick is my event, but as you can see, show/hide events refers to highcharts-internals.

@ppotaczek
Copy link
Contributor

Thanks for the information @annitya. Could you reproduce the problem in codesandbox? You can start from: https://codesandbox.io/s/highcharts-react-demo-forked-yi81d?file=/demo.jsx
I am almost sure that we can do something with it!

@marcoripa96

This comment was marked as duplicate.

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

5 participants