tradingview / lightweight-charts

Performant financial charts built with HTML5 canvas
https://www.tradingview.com/lightweight-charts/
Apache License 2.0
8.77k stars 1.54k forks source link

Chart isn't updated when primitive is detached using plugins-example #1594

Open Rafael-Ramblas opened 1 month ago

Rafael-Ramblas commented 1 month ago

Lightweight Charts™ Version: 4.1.4

Importing the code from the band-indicator example and using in a React component there's an issue with detaching the primitive using a controlled state, as shown in the code below, the chart isn't updated after executing the function detachPrimitive.

The problem relies on the file plugin-base.ts that implements the method for detaching the primitive. The method doesn't call the requestUpdate function before destroying the instance data. If we add the line this.requestUpdate(); before this._requestUpdate = undefined; the feature works as expected.

const colors = {
  backgroundColor: "black",
  lineColor: "#2962FF",
  textColor: "white",
  areaTopColor: "#2962FF",
  areaBottomColor: "rgba(41, 98, 255, 0.28)",
};

const bandIndicator = new BandsIndicator();

const ChartComponent = ({
  data,
  showBands,
}: {
  data: LineData[];
  showBands: boolean;
}) => {
  const chartContainerRef = useRef<HTMLDivElement>(null);
  const lineSeries = useRef<ReturnType<IChartApi["addLineSeries"]>>(null);

  useEffect(() => {
    const { backgroundColor, textColor } = colors;
    const handleResize = () => {
      chart.applyOptions({ width: chartContainerRef.current.clientWidth });
    };

    const chart = createChart(chartContainerRef.current, {
      layout: {
        background: { type: ColorType.Solid, color: backgroundColor },
        textColor,
      },
      width: chartContainerRef.current.clientWidth,
      height: 300,
    });

    lineSeries.current = chart.addLineSeries();
    lineSeries.current.setData(data);

    chart.timeScale().fitContent();

    window.addEventListener("resize", handleResize);

    return () => {
      window.removeEventListener("resize", handleResize);
      chart.remove();
    };
  }, [data]);

  useEffect(() => {
    if (!lineSeries.current) return;
    if (showBands) {
      lineSeries.current.attachPrimitive(bandIndicator);
    } else {
      lineSeries.current.detachPrimitive(bandIndicator);
    }
  }, [showBands]);

  return <div ref={chartContainerRef} />;
};

const ChartWidget = (props: any) => {
  const [showBands, setShowBands] = React.useState(false);
  const sampleData = React.useMemo(() => generateLineData(), []);
  return (
    <>
      <ChartComponent data={sampleData} showBands={showBands} />
      <button onClick={() => setShowBands(!showBands)}>Toggle Bands</button>
    </>
  );
};

Actual behavior:

The chat isn't updated when the method detachPrimitive is executed.

Expected behavior:

The chart should update and remove the primitive draw.

CodeSandbox/JSFiddle/etc link:

https://codesandbox.io/p/sandbox/jolly-christian-lkmqnh

SlicedSilver commented 1 month ago

Thanks for the bug report with the helpful example.

I think it makes more sense if the library automatically refreshes the chart when a primitive is removed instead of relying on the Plugin code to do this within the detached lifecycle hook. So while it would be possible to fix this within the plugins code (in this case plugin-base), it would make more sense to implement this by adding this._series.model().fullUpdate(); to the end of detachPrimitive method.

https://github.com/tradingview/lightweight-charts/blob/0f7343e7934a063238c61a67ca9926248f7f1fbd/src/api/series-api.ts#L241-L246

Rafael-Ramblas commented 1 month ago

I found one more information about this. This issue only happens for line charts, other types of charts works as expected without having the need to call the requestUpdate method, and adding the line suggested actually causes a runtime error.