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

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

Open
stof opened this issue Feb 21, 2023 · 5 comments

Comments

@stof
Copy link
Contributor

stof commented Feb 21, 2023

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
Copy link
Member

netil commented Mar 2, 2023

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
Copy link
Contributor Author

stof commented Mar 2, 2023

@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
Copy link
Member

netil commented Mar 17, 2023

@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
Copy link
Contributor Author

stof commented Mar 17, 2023

@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:

  • make it possible to disable the lazy rendering entirely in case lazy: false is set (the automatic behavior would be kept for the case where the config is undefined). Rendering in an hidden element can cause some issues with some features, but that's something that can be documented as a warning in the doc for this option, and the automatic lazyness for hidden element is broken anyway if the hiding come from styles of a parent instead of hiding just that element as the automatic detection won't detect when it becomes visible again)
  • add an early error in any public API that cannot be called before the chart is rendered, to have clear error messages explaining what happens and how to fix it instead of getting weird errors about internal APIs not existing

@stof
Copy link
Contributor Author

stof commented Mar 17, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants