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

React Advanced Chart Example in Documentation has bug when removing chart during resizing #1429

Open
patrickcook28 opened this issue Sep 25, 2023 · 1 comment
Labels
bug Unexpected problem or unintended behavior. documentation Improvements or additions to the documentation needs investigation Needs further investigation.
Milestone

Comments

@patrickcook28
Copy link

patrickcook28 commented Sep 25, 2023

Lightweight Charts™ Version: 4.0.1

Steps/code to reproduce:

Copy advanced react example from Lightweight Chart docs

export const ChartContainer = forwardRef((props, ref) => {
  const { children, container, layout, ...rest } = props;

  const chartApiRef = useRef({
    api() {
      if (!this._api) {
        this._api = createChart(container, {
          ...rest,
          ...layout,
          width: container.clientWidth,
          height: window.innerHeight * 0.8,
        });
        this._api.timeScale().fitContent();
      }
      return this._api;
    },
    free() {
      if (this._api) {
        this._api.remove(); // error occurs here
      }
    },
  });
  ...


export const Series = forwardRef((props, ref) => {
  const parent = useContext(ChartContext);
  const context = useRef({
    api() {
      if (!this._api) {
        const { children, data, ...rest } = props;
        this._api = parent.api().addCandlestickSeries(rest);
        this._api.setData(data);
      }
      return this._api;
    },
    free() {
      if (this._api) {
        parent.free(); // error occurs here
      }
    },
  });
  ...

Actual behavior:

When copying the Advanced React chart example that uses Refs and splits the Chart, Container, and Series into individual components, it doesn't work. An error is thrown when trying to remove the chart.

In development mode, React will run all useEffects and useLayoutEffects, then run their clean-up functions, then re-run the effects. There are some bugs in the clean-up functions which caused the example to not run properly.

The error occurred when chart.remove() was called in the useLayoutEffect hook that was responsible for handling resizing of the chart.

Expected behavior:

The example should work out of the box.

The example posted on the website should include proper clean-up functions. The solution was to add this._api = null; in the free() functions for both the ChartContainer and Series components.

@SlicedSilver SlicedSilver added bug Unexpected problem or unintended behavior. documentation Improvements or additions to the documentation needs investigation Needs further investigation. labels Sep 25, 2023
@SlicedSilver SlicedSilver added this to the 5.0 milestone Sep 25, 2023
@mishurov
Copy link

I would use this for Chart

if (this._api) {
  this._api.remove();
  this._api = undefined;
}

and this for Series

if (this._api) {
  parent._api && parent.api().removeSeries(this._api);
  this._api = undefined;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Unexpected problem or unintended behavior. documentation Improvements or additions to the documentation needs investigation Needs further investigation.
Projects
None yet
Development

No branches or pull requests

3 participants