1

enter image description here

I've using "billboard.js": "^1.10.2", and react.js

I had searched billboard.js's documentation and found that onresize(), onresized() is attached on window and when I call chart.destroy() then It removes every events being attached on window that being related with this library.

So I tested it without state update on onresize(), onresized() it successfully deleted all events, but when I did it with state update on onreszie(), onresized() events were still attached on window. As a result of this I think this issue happens not because of billboardjs, but reactjs.

Why is it? Any ideas?

//...
const [isResize, setIsResize] = useState(false);

const options = {
    onresize(ctx) {
      // "resize" keep prints even chart.destory() is called.
      console.log("resize");
      setIsResize(true);
    },
    onresized(ctx) {
      setIsResize(false);
    },
//...
      <Chart
        className="timelineChart"
        options={options}
        isResize={isResize}
        ref={chartRef}
      />
//...
const options = {
    onresize(ctx) {
      // "resize" is no longer prints when chart.destory() is called.
      console.log("resize");
    },
    onresized(ctx) {

    },
//...

Edit billboardjs resize onresize bug

JillAndMe
  • 3,989
  • 4
  • 30
  • 57

2 Answers2

3

I believe you are attaching multiple event listeners. Each time the Page1 component re-renders, it attaches a new set of event listeners without cleaning up the old ones. What causes a re-render? State changes. That's why you are only seeing the issue once you add useState and setState.

You can verify this by checking the logs and noticing this not so helpful error:

Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in a useEffect cleanup function. in Page1 (at App.js:13)

You'll need to modify the code related to attaching/detaching event listeners to avoid this. I'm not familiar with Billboard so I can only tell you where the problem is, not the exact spot to fix it.

My gut says its the Chart.jsx here:

  renderChart = () => {
    console.log('render chart');
    const { current } = this.chartRef;
    if (current !== null) {
      this.chartInstance = bb.generate({
        ...this.props.options,
        bindto: this.chartRef.current
      });
    }
  };

Updated with full solution

I was correct in that the Chart.jsx is where the problem lies.

Listeners should be attached when the DOM is created and removed with the DOM is destroyed. You were not wrong when you first thought to use the React Lifecycles for this chore, however I find the Callback References to be more useful, especially when some of the DOM may be destroyed or created during update cycles (you do not have this problem).

Callback References can be tricky, do not use functions that get recreated each render (trust me its a headache). Callback References are called for two reasons. First, the DOM has been created and to hand you a reference to the element. Second, the DOM has been destroyed, so time to clean up. If React senses a change in the Callback Reference (i.e. you give it a new one) it will tell the first Callback Reference to clean up, and the second Callback Reference to initialize (this is the headache I mentioned). You can avoid this by using an instance method.

// Bind to 'this' otherwise 'this' is lost
  setChartRef = (ref) => {
    // Remove listeners
    if (!ref) {
      console.log('no reference');
      this.chartRef.current = null;
      this.destroy();
    }
    // Add listeners
    else {
      console.log('new reference');
      this.chartRef.current = ref;
      this.createChart();
    }
  };

Next piece of the puzzle, you only want to call bb.generate one time. This was causing multiple listeners to be created. So I've simplified and renamed your renderChart to createChart

  createChart = () => {
    this.chartInstance = bb.generate({
      ...this.props.options,
      bindto: this.chartRef.current
    });
  };

Finally, none of the lifecycle methods are necessary because Callback Reference tell us exactly when to create the chart and when to destroy it. You may be wondering what about resizing the chart? Seems like that is taken care of automatically? I could be missing something here, but in the event you need to update the chart, use this.chartInstance in an update lifecycle method.

My full modifications here:

Edit billboardjs resize onresize bug

Spidy
  • 39,723
  • 15
  • 65
  • 83
  • I should mention, you can absolute still use mount and unmount to do the exact same thing, if you find Callback References to be too confusing. The main problem was multiple calls to bb.generate, so put the createChart in the mount and destroy in the unmount, same fix different solution – Spidy Feb 28 '20 at 05:33
2

onresize is actually a DOM event, it's not part of billboard or React.

https://developer.mozilla.org/en-US/docs/Web/API/Window/resize_event

You'll need to cancel your function somehow, like making it run conditionally or defining it in a component that will be unmounted.

Sydney Y
  • 2,912
  • 3
  • 9
  • 15
  • removes all events when componentWillUnmount is called, but with state update on onresize() it is not deleted... – JillAndMe Feb 28 '20 at 04:29
  • 2
    Then it's not removing all event listeners, is it? Haha. If the listener is attached to the DOM it would have to be explicitly removed or overwritten, React wouldn't garbage collect it. – Sydney Y Feb 28 '20 at 04:34