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

Highcharts prototype overrides can leak between charts #115

Open
pinkynrg opened this issue Jun 6, 2018 · 16 comments
Open

Highcharts prototype overrides can leak between charts #115

pinkynrg opened this issue Jun 6, 2018 · 16 comments

Comments

@pinkynrg
Copy link

pinkynrg commented Jun 6, 2018

When i hover on a curve the tooltip shows as expected.

But when I leave the chart though, the tooltip stays there. Is it normal?

@whawker whawker added the bug label Jun 6, 2018
@whawker
Copy link
Owner

whawker commented Jun 6, 2018

Good spot, that looks like a bug to me. Are you using React JSX Highcharts version 3?

@pinkynrg
Copy link
Author

pinkynrg commented Jun 6, 2018

yes, from package.json: "react-jsx-highcharts": "^3.0.1"

@whawker
Copy link
Owner

whawker commented Jun 6, 2018

By any chance are you overriding Highcharts.Pointer.prototype.reset anywhere in your app?

I've found that in my examples I'm overriding the reset method for the SynchronisedCharts example, however that seems to be bleeding through to other examples such as the SplineWithPlotBands example.

I had no idea Webpack would build the code in that way!

@pinkynrg
Copy link
Author

pinkynrg commented Jun 6, 2018

hehe yes i am, in a sync charts as you do :) I'll try to remove and see if it goes away.

@pinkynrg
Copy link
Author

pinkynrg commented Jun 6, 2018

yes that is it!

@whawker
Copy link
Owner

whawker commented Jun 6, 2018

Wondering if I should make withHighcharts take a third argument, where we provide the overrides as an object of paths.

i.e.

export default withHighcharts(MyComponent, Highcharts, {
  'Pointer.prototype.reset': () => {},

  'Point.prototype.highlight': function (func, event) {
    this.onMouseOver(); // Show the hover marker
    this.series.chart.tooltip.refresh(this); // Show the tooltip
    this.series.chart.xAxis[0].drawCrosshair(event, this); // Show the crosshair
  }
});

We can apply the overrides when the withHighcharts HOC mounts, and revert to standard functionality when the HOC unmounts.

Worth noting, in the second override function 'Point.prototype.highlight', the first argument is the original functionality, a lodash wrapped function.

@pinkynrg
Copy link
Author

pinkynrg commented Jun 6, 2018

IMHO that looks clean.

@whawker
Copy link
Owner

whawker commented Jun 6, 2018

Nice one.

@anajavi any opinions on this? (Sorry to keep bugging you)

Seems that overriding Highcharts functionality like this, bleeds through into other charts elsewhere in the app, that might not want the same overrides.

@anajavi
Copy link
Collaborator

anajavi commented Jun 7, 2018

It overrides class prototype methods, so it should bleed all instances created.

About the HOC:
Wouldn't that bleed to other charts mounted at the same time? If there are several charts mounted with different withHighcharts calls, it is quite unclear which pointer config wins the race.

Isn't there any other way to have that functionality than modifying the prototype? Events? Tooltip formatter code?

@anajavi
Copy link
Collaborator

anajavi commented Jun 7, 2018

Highcharts instantiates chart.pointer on first render.

The intance methods can be modified in some "after first render" event hook. That way the global Pointer prototype is not polluted.

Something like this (does not work, just an example):

<Chart onRedraw={ chart.pointer = myPointer //TODO run only once } />

@whawker
Copy link
Owner

whawker commented Jun 7, 2018

It overrides class prototype methods, so it should bleed all instances created.

Yeah good point

If there are several charts mounted with different withHighcharts calls, it is quite unclear which pointer config wins the race.

Ah yes, also correct. I was thinking about this too.

Isn't there any other way to have that functionality than modifying the prototype?

Potentially we could use the callback functionality that already exists, and only modify the current chart instance, but it requires a bit of reverse engineering of the Highcharts source to figure out how to do it.

getChart = function (chart) {
  chart.pointer.reset = () => {};

  // Not sure if this works
  Highcharts.addEvent(Highcharts.Point, 'afterInit', function () {
    const point = this;
    // Adding highlight fn to every point instance, not very efficient
    point.highlight = function (event) {	
      this.onMouseOver(); // Show the hover marker	
      this.series.chart.tooltip.refresh(this); // Show the tooltip	
      this.series.chart.xAxis[0].drawCrosshair(event, this); // Show the crosshair	
    };
  }
}

render () {
  return (
    <HighchartChart callback={this.getChart}>
      // omitted
    <HighchartChart />
  )
}

Although that requires a bit more

@ErnstWevers
Copy link

ErnstWevers commented Jun 11, 2018

I've encountered a similar bug, testing my application in IE11. I have a chart with multiple series with a tooltip that shows correctly on hover, and also disappears correctly when I leave the chart. In my case I have an onclick event tied to the charts' points, which when triggered has the unintended effect of creating a static tooltip which does not disappear. This can be repeated multiple times. It does not happen in chrome or FF.

Some info on the used versions:

  • react": "^15.6.2
  • react-jsx-highcharts": "^2.3.0-alpha.1

highcharts-jsx-tooltip

@whawker
Copy link
Owner

whawker commented Jun 11, 2018

@ErnstWevers would you be able to give a simplified reproducible example? Its difficult to debug without knowing what you're doing in the the click handler etc.

@ErnstWevers
Copy link

ErnstWevers commented Jun 13, 2018

@whawker I worked on it yesterday, but I am having some issues replicating the behavior outside our application. I have narrowed it down but there are still a number of options. A little recap: since I'm working in 2.3.0 the onClick prop has to be passed as an click event option. This works with a logger in the application but once I connect it to the actual functionality, i.e. make a fetch call to grab the raw data the graph represents, it shows the erroneous behavior. I haven't been able to replicate the error outside of the original application. So for now the tooltip is out, I will keep you informed if I manage to produce a reproduction.

@ErnstWevers
Copy link

I have replicated the problem here. It seems to be a side effect of using Preact instead of React.

@whawker
Copy link
Owner

whawker commented Jun 14, 2018

@ErnstWevers that is a strange way to pass plot options, the usual React JSX Highcharts way is just to pass plotOptions as a prop.

This seems to resolve the issue.

Highcharts.setOptions({
  lang: {
    noData: 'Geen berichten gevonden voor deze periode',
    resetZoom: 'Herstel zoom'
  }
});

const plotOptions = onClickAction => ({
  series: {
    cursor: 'pointer',
    point: {
      events: {
        click() {
          console.log('I got clicked! My category is: ', this.category);
          onClickAction();
        }
      }
    }
  }
});

const Chart = ({ title = '', onClickAction = () => {} }) => {
  const plotOpts = plotOptions(onClickAction);

  return (
    <div className="app">
      <HighchartsChart plotOptions={plotOpts}>
        <Title>{title}</Title>
        <Legend layout="vertical" align="right" verticalAlign="middle" />
        <XAxis>
          <XAxis.Title>X</XAxis.Title>
        </XAxis>
        <YAxis id="axis">
          <LineSeries name="testSeries1" data={[1, 2, 3, 2, 3, 4, 3, 4]} />
          <LineSeries name="testSeries2" data={[2, 3, 4, 3, 4, 5, 4, 5]} />
          <LineSeries name="testSeries2" data={[4, 3, 3, 2, 2, 3, 2, 1]} />
        </YAxis>
        <Tooltip shared />
      </HighchartsChart>
    </div>
  );
};

@whawker whawker changed the title tooltip doesn't fade out Highcharts prototype overrides can leak between charts Jun 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants