naver / billboard.js

📊 Re-usable, easy interface JavaScript chart library based on D3.js
https://naver.github.io/billboard.js/
MIT License
5.84k stars 353 forks source link

Calling .load() on a chart that is not rendered yet (due to lazy rendering) triggers an error #3106

Closed stof closed 1 month ago

stof commented 1 year ago

Description

Steps to check or reproduce

    const duration = 2500

    const options = {
      bindto: element,
      data: {
        columns: [
          ['fill', 0],
          ['background', 1]
        ],
        type: donut(),
        order: null
      },
      legend: {
        show: false
      },
      donut: {
        label: {
          show: false
        },
        width: 10,
        expand: false
      },
      interaction: {
        enabled: false
      },
      size: {
        height: 175,
        width: 175
      },
      transition: {
        duration
      },
    }

    const chart = bb.generate(options)

    setTimeout(function () {
      chart.load({
        columns: [
          ['fill', Math.min(score / 100, 1)],
          ['background', 1 - Math.min(score / 100, 1)]
        ]
      })
    }, 100)

If element is hidden at the time of running this code (in my case, it is inside a modal for which the opening animation has not run yet), the .load call fails because updateTargets tries to use $el.main which has not been initialized yet.

netil commented 1 year ago

IMO, this need to be handle by usage perspective. Basically, if the bound element isn't visible, loading new data has no sense, because it won't visualize loaded data when the element is hidden.

In this case, the call of .load() API, need to be called after the initialization.

// will be called, when chart element's visibility turn to be visible
onafterinit: function(){ 
    setTimeout(function () {
        chart.load({
            columns: [
               ['fill', Math.min(30 / 100, 1)],
               ['background', 1 - Math.min(30 / 100, 1)]
            ]
        })
    }, 100)
}
stof commented 1 year ago

@netil I indeed fixed the time when I loaded the data.

However, it would still be great to avoid triggering an error due to accessing an undefined internal property when calling this method when the chart is not initialized yet, especially given that lazy rendering is active by default if the element is initially hidden (and there is no way to opt-out).

netil commented 1 year ago

@stof, thanks for the suggestion. Basically, if lazy init is active, all of the APIs aren't able functioning normally.

What will be the "best" way handling this? Not throwing error? warning via console? or just staying silently not giving any notice? And expanding for every possible error cases, what policy is the "best"?

The way to handle this, will differ based on each one's perspective. I'll be taking others libraries as the reference on handling errors and then will consider apply some general consistent policy for error handling.

stof commented 1 year ago

@netil the issue is that there is no way to disable lazy init. Even putting an explicit lazy: false will still be lazy if the element is hidden at the time of rendering.

To me, the solution should look like this:

stof commented 1 year ago

side note: if the codebase was actually typechecked (instead of using any almost everywhere as done currently due to the way the code is organized), the type checker would force you to actually handle the case of uninitialized state.

stof commented 1 month ago

I found another case that triggered such error when calling the public API while a chart was not yet initialized due to the automatic lazy rendering:

const chart = bb.generate(options);

chart.legend.show(['first', 'second']);
netil commented 1 month ago
  • make it possible to disable the lazy rendering entirely in case lazy: false is set...

Will update to make a "forced" state to initialize, even when lazy:false with chart element isn't visible. But in this case, chart's composition elements can't render properly due to non visible state.

  • add an early error in any public API that cannot be called before the chart is rendered, to have clear error messages ...

Was trying to implement this, but not really sure. Giving warning msg is good for users, but in terms of library is adding some additional call execution. Will try consider in the future.