unitaryfund / metriq-app

Web app for Metriq
https://metriq.info
Apache License 2.0
28 stars 19 forks source link

Remove Chart.js entirely (re: VQA feature) #905

Open WrathfulSpatula opened 6 months ago

WrathfulSpatula commented 6 months ago

We've run into an extremely weird series of bugs with Charts.js in the VQA feature. Firstly, the chart "randomly" reordered the association of bar chart labels to bar chart data values. Through experimentation, I found that if I alphabetized the labels (with the correct data points still associated), and so long as the values were numerical rather than strings (with the understanding that Charts.js usually can accept numerical data as strings, so this consideration shouldn't be necessary), the charts appear correctly on my screen. However, it's now been discovered that, seemingly at random, other users on the internet see random reordering of the data values' associations with labels, still!

The bar charts API in Charts.js is simply unsuitable and unfit, by this point. We should consider reporting the issue to the maintainers of that API. Regardless, we need to transition to another charting API entirely, probably D3.js.

@vprusso @nathanshammah I wanted to draw your attention, here. I don't have plans this weekend, so I hoped to get a jump start on this today and maybe tomorrow, over the weekend. The VQA feature in particular is affected. Since we were already in the process of transitioning to D3-based set of chart visualizations, I just need to speed up the timeline.

I apologize for the confusion this has caused, but it appears to be a basic failing the Charts.js API, which is usually relatively trusted and popular. Metriq might end up more secure, as a result, once we switch entirely to D3. This is top priority, for me.

WrathfulSpatula commented 6 months ago

It's worth adding: we previously used a secondary npm package for error bars on bar charts, and this plugin works "on top" of the Charts.js bar chart API. We weren't actually using the error bars functionality for bar charts, so I jettisoned this dependency. This did not fix the issue. Rather, the problem appears to be in "core" Charts.js, which is extremely worrisome.