react-financial / react-financial-charts

Charts dedicated to finance.
https://react-financial.github.io/react-financial-charts/
MIT License
1.2k stars 210 forks source link

React 18 Flickering #684

Open markmcdowell opened 1 year ago

markmcdowell commented 1 year ago

When running under React 18 the current version of the charts is flickering on mouse wheel operations. This appears to be due to React 18 batching updates originating from native event handlers. See https://react.dev/blog/2022/03/08/react-18-upgrade-guide#automatic-batching

We've tried using the recommend method of FlushSync to avoid the batching, whilst this does remove the flickering, it also produces an unacceptable level of performance and jerk.

The only workaround at the moment is to use ReactDom.render instead of ReactDom.createRoot().render, this will mean the same behaviour as React 17 but with a warning in the console.

jsnns commented 1 year ago

Any updates on this?

Arnau-Gelonch commented 1 year ago

Has anyone been able to solve it?

jsnns commented 1 year ago

@Arnau-Gelonch I found a (very suboptimal) work-around for now. It's not perfect:

class StockChart extends React.Component<StockChartProps> {
  idFromProps(): string {
    return `candle-chart-${this.randomId}`;
  }

  componentDidUpdate(): void {
    if (this.props.data.length === 0) {
      return;
    }

    // if there is already a chart rendered, remove it
    const element = document.getElementById(this.idFromProps());
    if (this.hasRendered && element) {
      ReactDOM.unmountComponentAtNode(element);
      this.hasRendered = false;
    }
    if (!this.hasRendered) {
      ReactDOM.render(this.internalRender(), document.getElementById(this.idFromProps()));
      this.hasRendered = true;
    }
  }

  componentDidMount(): void {
    this.componentDidUpdate();
  }

  componentWillUnmount(): void {
    const element = document.getElementById(this.idFromProps());
    if (this.hasRendered && element) {
      ReactDOM.unmountComponentAtNode(element);
    }
  }

  public render() {
    // use ReactDom.render to render the component
    return <div id={this.idFromProps()} />;
  }

  public internalRender() {
    // render your chart
  }
}
avi2d commented 1 year ago

Leave this.clearThreeCanvas() only in getSnapshotBeforeUpdate and remove it from other places

carterjfulcher commented 11 months ago

Are there any updates on this? My company is looking to use this library in an upcoming product. We could possibly devote dev time to fixing.

electronicbits commented 11 months ago

@carterjfulcher I would've thought this is already fixed, there is no flickering anymore (under React 17 rendering method) However, I do notice the chart is not displayed when panning vertically. It does work horizontally. This can be checked on the current stories. I spent a few hours debugging the code, inspecting basically the draw functions in the ChartCanvas class but have not been able to identify the issue yet, as they "seem" to be ok to me so far. Anyone else having this issue ? Any idea ?

issue-charts

pencilcheck commented 8 months ago

On React 18, panning and zooming still flickers.

Tried avi2d method, doesn't work for me.

By changing

    shouldComponentUpdate() {
        return false;
    }

in "node_modules/.pnpm/@react-financial-charts+core@2.0.0_react-dom@18.2.0_react@18.2.0/node_modules/@reac t-financial-charts/core/lib/ChartCanvas.js" It makes the flickering a lot less, but there are still flickering at random intervals.

donkey-donkey commented 6 months ago

@carterjfulcher I would've thought this is already fixed, there is no flickering anymore (under React 17 rendering method) However, I do notice the chart is not displayed when panning vertically. It does work horizontally. This can be checked on the current stories. I spent a few hours debugging the code, inspecting basically the draw functions in the ChartCanvas class but have not been able to identify the issue yet, as they "seem" to be ok to me so far. Anyone else having this issue ? Any idea ?

this is exactly what we are experiencing

Cr1pter commented 5 months ago

Is there any solution to this in next.js?

ReCodee commented 5 months ago

Leave this.clearThreeCanvas() only in getSnapshotBeforeUpdate and remove it from other places

This fixed the problem, Does it have any consequences? If not why is someone not creating a PR as this fixes the issue?

ReCodee commented 5 months ago

Leave this.clearThreeCanvas() only in getSnapshotBeforeUpdate and remove it from other places

@Cr1pter I resolved the issue in next.js with this suggestion, You can do these changes in the ChartCanvas.js present in the module.